-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Adopt OTel semantic conventions for agents and frameworks #2575
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
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Response from ADK Triaging Agent Hello @krisztianfekete, thank you for your contribution! It looks like a couple of automated checks are failing on this PR. To help move this forward, could you please address the following?
Addressing these items will help our reviewers proceed with the review process. Thanks! |
aa86026
to
e5ef820
Compare
@Jacksunwei, can you please take a look at this PR once you have a chance? |
Hey @Jacksunwei, can you please take a look at this? |
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.
Hi, thank you for the PR and apologies for the delay.
We've recently started working on GenAI semconv alignment. Our internal approach differs slightly: we tentatively plan to use logs for prompts/responses and use the opentelemetry-instrumentation-google-genai library for instrumenting LLM calls.
Because of these differences, your PR will likely by put on hold for the time being.
Still whenever you get a chance, please address the questions in this review, as it would give us valuable feedback to consider in our implementation.
if hasattr(llm_response, 'id') and llm_response.id: | ||
span.set_attribute('gen_ai.response.id', llm_response.id) | ||
|
||
# Set response model if different from request model | ||
if ( | ||
hasattr(llm_response, 'model') | ||
and llm_response.model | ||
and llm_response.model != llm_request.model | ||
): | ||
span.set_attribute('gen_ai.response.model', llm_response.model) | ||
|
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.
Where did you find the llm_response.model
and llm_response.id
field?
They are not a part of the pydantic model https://github.com/google/adk-python/blob/main/src/google/adk/models/llm_response.py#L26
if ( | ||
hasattr(llm_response, 'model') | ||
and llm_response.model | ||
and llm_response.model != llm_request.model | ||
): | ||
span.set_attribute('gen_ai.response.model', llm_response.model) |
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.
What's the reasoning for only setting the gen_ai.response.model
when it's different to gen_ai.request.model
?
Is this behavior specified somewhere, so it would be reasonable to assume both are the same if the response model is missing?
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.
SemConv marks gen_ai.request.model
as “Conditionally Required (if available)” and gen_ai.response.model
as “Recommended,” not required, so it might makes sense to only include it if it provides additional information. I have a vague recollection of this practice from one of the many semconv GH issues discussing this, but cannot find it now.
That's the discussion I am aiming to start. It was decided in OTel community that both span attributes AND logs should be a valid options to emit input/output messages and the end users should be able to use one or both depending on their requirements.
Do you have an ETA on this effort? There are also some missing attributes that would be a useful addition and I am happy to spin those out in a separate PR as I don't expect those to be contraversail, e.g. |
Ultimately instrumentation of LLM inputs/outputs will not be done in the ADK codebase, but instead in opentelemetry-instrumentation-google-genai (and TBD for non-gemini LLM requests).
You should see the first changes within one to two weeks.
I appreciate the offer and would be happy to review it. This effort would overlap with my current work, but I think having more pairs of eyes on this would be beneficial. |
This PR adopts the current OpenTelemetry Semantic Conventions for genAI agents and frameworks as per https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-agent-spans/
Fixes #1826
Sets
span.set_attribute('gen_ai.system', 'gcp.vertex_ai')
according to the well-known system names.gen_ai.operation.name
for Agent invocations toinvoke_agent
andchat
for LLM calls according to specsAdds missing attributes:
gen_ai.response.model
gen_ai.request.temperature
Adds new utility functions to support proper OpenTelemetry conventions for naming and event emission:
_create_span_name(operation_name: str, model_name: str) -> str
Creates span names following the OTel convention:
{operation_name} {model_name}
add_genai_prompt_event(span: trace.Span, prompt_content: str)
Adds GenAI prompt events using the standard
gen_ai.content.prompt
event nameadd_genai_completion_event(span: trace.Span, completion_content: str)
Adds GenAI completion events using the standard
gen_ai.content.completion
event nameLocal demo script: