-
Notifications
You must be signed in to change notification settings - Fork 994
[WIP] Adapt to the latest input and output semantic specifications #14613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🔧 The result from spotlessApply was committed to the PR branch. |
*/ | ||
@AutoValue | ||
@JsonClassDescription("Generic part") | ||
public abstract class GenericPart implements MessagePart { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jackson annotations or io.opentelemetry.api.common.Value
?
* @param chunkMessage the chunk message to append | ||
* @return a new OutputMessages instance with the merged content | ||
*/ | ||
public OutputMessages merge(int index, OutputMessage chunkMessage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, abstracting all StreamListener
types into a single interface might be a better choice. It should have an onChunk(OutputMessages)
method, an onEnd()
method, and an onError(Throwable)
method. We can complete the common collection logic here uniformly — such as the aggregation of OutputMessages, the captures of usage tokens and time per output chunk , etc.
How do you think about that? cc @anuraaga
import javax.annotation.Nullable; | ||
|
||
// as a field of Attributes Extractor | ||
// replace the 'ChatCompletionEventsHelper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By combining this instance into the GenAiAttributesExtractor
, we can uniformly manage various collection switches in the AttributesExtractor
— whether to collect chat history, whether to collect as span attributes or as logs, etc.
However, this also means that I might need to emit events in the AttributesExtractor
, and I'm not sure if this is the right approach cc @trask. But according to the current semantic specification, we need to record some necessary attributes in events — and these attributes can almost only be obtained in the AttributesExtractor
and SpanProcessor
.
If we don't do this in the AttributesExtractor
, the best approach might be to define an EventsExtractor
in the Instrumenter
and allow it to read the current attributes like:
public interface EventsExtractor {
public Context onStart(Attributes startAttributes, Context context);
public void onEnd(Attributes endAttributes, Context context);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OperationListener maybe another nice choice.
OutputMessage.create( | ||
Role.ASSISTANT, | ||
messageParts, | ||
choice.finishReason().asString())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each choice has a finish_reason
, so which one should be recorded in the span's gen_ai.response.finish_reasons
at this point?
} else if (msg.isUser()) { | ||
inputMessages.append(InputMessage.create(Role.USER, contentToMessageParts(msg.asUser().content()))); | ||
} else if (msg.isAssistant()) { | ||
ChatCompletionAssistantMessageParam assistantMsg = msg.asAssistant(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these parts be recorded in the same ChatMessage?
In multi-turn conversation scenarios, there may be multiple 'assistant messages' in the 'input messages' - should all of them be recorded? (I think we should truthfully record the input of each call, regardless of whether it's a multi-turn conversation or not.)
So far,
Lines 13 to 31 in 5307e20
|
No description provided.