-
Notifications
You must be signed in to change notification settings - Fork 827
feat: add extension point to add custom attributes to generate_content spans in google genai instrumentor.
#3961
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
…ontent {model.name}` spans
|
|
aabmass
left a comment
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.
LGTM, it could probably be generalized more to other GenAI instrumentations if this proves useful. We can also use this to set the gen_ai.conversation.id from ADK 🙂
| # Constant used for the value of 'gen_ai.operation.name". | ||
| _GENERATE_CONTENT_OP_NAME = "generate_content" | ||
|
|
||
| GENERATE_CONTENT_EXTRA_ATTRIBUTES = contextvars.ContextVar[Mapping[str, AttributeValue]]("GENERATE_CONTENT_EXTRA_ATTRIBUTES", default={}) |
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 do you think of using OTel context api instead of contextvars directly? It uses contextvars under the hood but it would then also work with code that does explicit context passing
def foo(context: Context):
with tracer.start_as_current_span("foo", context=context):
...(Admittedly it's a little clunkier/indirect)
| # Constant used for the value of 'gen_ai.operation.name". | ||
| _GENERATE_CONTENT_OP_NAME = "generate_content" | ||
|
|
||
| GENERATE_CONTENT_EXTRA_ATTRIBUTES = contextvars.ContextVar[Mapping[str, AttributeValue]]("GENERATE_CONTENT_EXTRA_ATTRIBUTES", default={}) |
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 think contextvars are usually named with snake case since the values aren't constants. Also, consider exposing it in __init__.py explicitly to be clear it's in the public API.
|
@RKest any blockers here? |
Description
Introduces ability to add additional attributes to
generate_content {model.name}spans to Google GenAI instrumentation.Motivation behind this change is to add (non-semconv)
gcp.vertex.agent.event_idattribute, togenerate_content {model.name}spans, in order to correlate events with spans in google/adk-python.Broader motivation is:
generate_contentand legacycall_llmspans.generate_contentspans and deprecatecall_llmspans.call_llmspans containgcp.vertex.agent.event_idattribute, and we still want to correlate events with spans.Note: Event in ADK is not synonymous with event in OTEL. In ADK, Event == LLM Request/Response or Tool Call/Result, in OTEL IIUC Event == Log.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested using:
opentelemetry-instrumentor-google-genaioff my fork in ADK, and successfully adding custom attributes to target spanDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.