-
Notifications
You must be signed in to change notification settings - Fork 62
[google-genai] Do not unconditionally set OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT during instrumentation #622
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
Open
somuai
wants to merge
1
commit into
open-telemetry:main
Choose a base branch
from
somuai:fix-google-genai-emit-event-override
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+32
−1
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
1 change: 1 addition & 0 deletions
1
instrumentation/opentelemetry-instrumentation-google-genai/.changelog/619.fixed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Do not unconditionally set OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT during instrumentation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 this was added because we want to emit the event regardless of the CONTENT_CAPUTRE mode ( we want CONTENT_CAPTURE to just impact whether content is captured on events/spans) where as I think other instrumentations were using the content_capture mode / env var to determine whether an event is emitted at all -- can you check if that is still true or not ?
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 @DylanRussell, thank you for the feedback. I investigated the entire codebase across all instrumentations and
opentelemetry-util-genai. Here is what I verified:How other instrumentations operate:
All instrumentations using
InferenceInvocationandTelemetryHandler(openai,bedrock,langchain,smolagents,portkey, andgoogle-genai) delegate event emission entirely toInferenceInvocation._maybe_create_event(), which callsshould_emit_event(). None of the other instrumentations setos.environ["OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT"].The contract in
opentelemetry-util-genai/src/opentelemetry/util/genai/utils.py:should_emit_event()explicitly implements:OTEL_INSTRUMENTATION_GENAI_EMIT_EVENTis set, user preference takes highest priority ("true"-> True,"false"-> False).OTEL_INSTRUMENTATION_GENAI_EMIT_EVENTis not set, it defaults based onOTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT:NO_CONTENTorSPAN_ONLY: defaults toFalse.EVENT_ONLYorSPAN_AND_EVENT: defaults toTrue.In
InferenceInvocation._maybe_create_event(), message content serialization is decoupled viaget_content_attributes():OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=trueis set,should_emit_event()returnsTrue, so thegen_ai.client.inference.operation.detailsevent is emitted.CONTENT_CAPTUREisNO_CONTENT,get_content_attributes()omitsinput_messages,output_messages, andsystem_instructionfrom the event payload, while preserving top-level operation attributes.OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=trueandOTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=NO_CONTENT.os.environ["OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT"] = "true"was problematic:generate_content.py, mutatingos.environdirectly at instrumentation time overrides any explicit user setting (such asOTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=false) and mutates the global process environment for all instrumentations.google-genaiinto exact alignment with all other instrumentations and restores respect for user-configured environment variables.Let me know if you would prefer
google-genaito retain a specific default when no environment variables are set, or if aligning with the sharedshould_emit_event()contract across all instrumentations is the desired direction.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.
yeah that will be a breaking change for users of this instrumentation.. i don't like that the CAPTURE_CONTENT env var is responsible for event emission.. that is not intuitive.. i'd prefer we got rid of that logic instead...
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 @DylanRussell,
Thank you for the review and for calling this out. You have raised two critical points:
Semantic Separation of Concerns & Public Documentation:
You are completely right that OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT should not be governing event emission. To answer your question from the issue: this behavior is NOT documented or required anywhere in the public OpenTelemetry Semantic Conventions specification. The official GenAI semconv defines payload privacy (capturing prompt/completion content) and event emission (gen_ai.client.inference.operation.details) as orthogonal concepts. An event can and should record operational telemetry (model name, token counts, response status) even when content capture is off. The fallback was introduced internally in Python PR #3994 (opentelemetry-util-genai) as a local heuristic.
Backwards Compatibility for google-genai:
Removing the hardcoded os.environ assignment exposes that utility fallback, causing google-genai to stop emitting events by default for unconfigured users, which is indeed a breaking change for existing users of this package. At the same time, the original issue (google-genai: instrument_generate_content() unconditionally sets OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=true, overriding config and user setting #619) arose because unconditionally writing os.environ["OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT"] = "true" clobbers explicit user opt-outs (e.g. when an operator or framework explicitly sets OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=false) and leaks across the entire process.
To resolve this cleanly, there are two paths:
Path A (Scoped, non-breaking fix for google-genai in this PR):
We preserve default event emission for google-genai when OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT is unset (for example, using os.environ.setdefault("OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT", "true") or resolving defaults in-memory), so existing users continue getting events without any disruption, while strictly honoring explicit user opt-outs like OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT="false".
Path B (Decoupling in opentelemetry-util-genai):
As you suggested, we remove the CAPTURE_CONTENT fallback logic from should_emit_event() in opentelemetry-util-genai. If we do that, what should the global default for should_emit_event() be when OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT is unset? Should it default to True (events on by default across all instrumentations, with message content gated by CAPTURE_CONTENT), or False?
Please let me know which direction you would prefer, and I will be happy to update this PR or submit a PR for opentelemetry-util-genai.
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.
My preference is to default it to
Trueand remove the capture content fallback logic.. Someone else should weigh in on this thoughThere 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.
Thanks @DylanRussell, that makes total sense. Defaulting event emission to
Truealigns well with standard telemetry expectations while keeping content payload capture decoupled underCAPTURE_CONTENT. Let's see if other maintainers / semconv WG members want to chime in on the global default. In the meantime, I can prepare a PR againstopentelemetry-util-genaiimplementing that decoupling if you'd like, or wait for consensus here.