-
Notifications
You must be signed in to change notification settings - Fork 1k
[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
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
|
|
see #15064 |
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.
Really sorry for the delay, the notification made me realize I had a pending comment that never got sent. Basically the approach looks fine as long as Jackson is acceptable in the instrumentation API.
| api("io.opentelemetry:opentelemetry-api-incubator") | ||
|
|
||
| compileOnly("com.google.auto.value:auto-value-annotations") | ||
| compileOnly("com.fasterxml.jackson.core:jackson-databind") |
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.
I believe this would need to be implementation because the API doesn't imply using the javaagent. The extra dependency I guess would be a concern with doing JSON marshaling within the instrumentation API itself, though when conventions rely on JSON, maybe it's unavoidable
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.
Yes. It may simplify the construction by defining these structured messages with json properties, which can also be seen in python-instrumentation: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/util/opentelemetry-util-genai/src/opentelemetry/util/genai/types.py
Here I use compileOnly because openai has depended on it yet, I just want to show my intention. However, implementation may lead to dependency conflicts. Maybe we should define these messages object in 'java-tooling' module, which has a shaded jackson dependency.
No description provided.