Skip to content

[google-genai, util-genai] Decouple event emission and set event severity to DEBUG (#619) - #836

Open
somuai wants to merge 9 commits into
open-telemetry:mainfrom
somuai:fix-google-genai-emit-event-decoupled
Open

somuai wants to merge 9 commits into
open-telemetry:mainfrom
somuai:fix-google-genai-emit-event-decoupled

Conversation

@somuai

@somuai somuai commented Oct 2, 2026

Copy link
Copy Markdown

Which problem is this PR solving?

Fixes #619

This PR implements the long-term decoupled GenAI event emission architecture requested by @DylanRussell:

  1. opentelemetry-util-genai:

    • Updates InferenceInvocation._maybe_create_event() to check if the event is disabled via self._logger.enabled(context=self._span_context, severity_number=SeverityNumber.DEBUG, event_name=event_name) (with graceful fallback for custom loggers whose enabled() does not accept severity_number). If disabled, log record creation and emission are skipped.
    • Sets the emitted LogRecord's severity_number to SeverityNumber.DEBUG per GenAI semantic conventions.
  2. opentelemetry-instrumentation-google-genai:

    • Removes unconditional os.environ["OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT"] = "true" in instrument_generate_content(), eliminating global process environment mutation and preserving explicit user configuration and OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT defaults.
  3. Tests:

    • Adds unit tests in opentelemetry-util-genai verifying event DEBUG severity, suppression when logger.enabled returns False, emission when logger.enabled returns True, and signature backwards compatibility.
    • Adds unit tests in opentelemetry-instrumentation-google-genai verifying that instrument() does not mutate or overwrite OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT.
    • All tests passing cleanly.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • pytest util/opentelemetry-util-genai/tests/test_utils_events.py (15/15 passed)
  • pytest instrumentation/opentelemetry-instrumentation-google-genai/tests/test_instrumentor.py (3/3 passed)
  • pytest instrumentation/opentelemetry-instrumentation-google-genai/tests/generate_content/ (124 passed)
  • pytest instrumentation/opentelemetry-instrumentation-google-genai/tests/interactions/ (344 passed)

…rity to DEBUG (open-telemetry#619)

- In opentelemetry-util-genai:
  - Check logger.enabled() before emitting inference details event
  - Set event severity_number to SeverityNumber.DEBUG per semantic conventions
- In opentelemetry-instrumentation-google-genai:
  - Remove unconditional mutation of OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT in instrument_generate_content()
  - Preserve user environment and content capture defaults
- Add unit tests validating event suppression, severity, and environment preservation
Copilot AI balanced review requested due to automatic review settings October 2, 2026 15:03
@somuai
somuai requested a review from a team as a code owner October 2, 2026 15:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The fallback can mishandle internal TypeErrors, and new tests depend on ambient environment state.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Decouples GenAI event emission from Google GenAI instrumentation configuration and adds DEBUG severity/filtering support.

Changes:

  • Removes global event-enabling environment mutation.
  • Checks logger enablement before constructing events.
  • Adds severity, filtering, and configuration-preservation tests.
File Description
_inference_invocation.py Adds DEBUG severity and logger filtering.
test_utils_events.py Tests severity and logger enablement.
generate_content.py Removes environment mutation.
test_instrumentor.py Verifies environment preservation.
nonstreaming_base.py Explicitly enables an expected test event.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

event_name=event_name,
):
return None
except TypeError:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in commit d5c7fa4. Restricted the TypeError fallback specifically to unexpected keyword argument severity_number, ensuring that any internal TypeErrors from custom logger implementations propagate cleanly and avoid duplicate invocations.

Comment on lines +485 to +490
@patch.dict(
os.environ,
{
"OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT": "EVENT_ONLY",
},
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in commit 66f2aae. Explicitly isolated OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=true across all test decorators in test_utils_events.py, and added unit test coverage verifying that unrelated internal TypeErrors propagate without suppression.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-10-06 20:34 UTC

Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):

  • Inline threads: 1
Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
  • Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

)


def test_instrument_does_not_mutate_emit_event_env_var(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this test. default is tested in the other test anyway

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in commit 8c6abe5, relying on for default preservation.

return None

event_name = "gen_ai.client.inference.operation.details"
if getattr(self._logger, "enabled", None):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please wait for #836 to land or repeat the fix here (by calling API directly and raising otel api version). No try/catch is necessary as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in commit 8c6abe5:

  • Removed the try/catch block and replaced with direct self._logger.enabled(context=self._span_context, severity_number=SeverityNumber.DEBUG, event_name=event_name)
  • Bumped opentelemetry-api to ~= 1.45 in util/opentelemetry-util-genai/pyproject.toml
  • Removed the legacy fallback unit tests from test_utils_events.py

All unit tests pass cleanly against opentelemetry-api 1.45.0.

@somuai

somuai commented Oct 4, 2026

Copy link
Copy Markdown
Author

Pushed commit 6f39257 with the remaining CI fixes:

  • Added hasattr guard on self._logger.enabled to safely handle loggers across test environments without breaking DEBUG event emission.
  • Synchronized uv.lock with pyproject.toml.
  • Formatted test suites via ruff 0.16.1.
  • Scoped explicit OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT: "true" to completion hook tests in test_e2e.py with clean environment restoration on teardown.

All local pre-commit checks and unit tests pass cleanly.

/dashboard route:reviewers

@@ -0,0 +1 @@
Check if logger is enabled before emitting inference details event, and set event severity to DEBUG per GenAI semantic conventions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should also remove OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT entirely from the repo in this PR... WDYT @lmolkova ?

Since we now have this generic way of disabling the event, we shouldn't need it anymore...

It'd be good to document how users can disable the event somewhere (instructions here: #619 (comment)).. I'm just not sure where since we don't have any public documentation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that since the standard LoggerProvider/Logger filter can disable events generically, OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT becomes redundant. Awaiting @lmolkova's confirmation whether you would like that removed in this PR or in a separate follow-up PR—happy to remove it here if preferred.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support removing it! we can document in the changelog that it's now controlled via logger config. Is there an e2e example in python-logging for severity-based enablement we can link to, @DylanRussell do you know?

context=self._span_context,
severity_number=SeverityNumber.DEBUG,
event_name=event_name,
):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be cached inside init so we don't have to recompute it each time.. self._emit_event could be reused to be this value if we agree the emit_event env var can be completely removed..

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in commit 561ed40d:
self._emit_event is now cached during InferenceInvocation.__init__ by checking self._logger.enabled(context=self._span_context, severity_number=SeverityNumber.DEBUG, event_name=event_name), eliminating the check from the hot path in _maybe_create_event() entirely.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is #619 (comment) addressed by this PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug events are not filtered out by default, the end user has to configure something to filter them out. Do we want this event off by default ? I didn't realize this before but the event itself is marked as "opt-in": https://github.com/open-telemetry/semantic-conventions-genai/blob/main/docs/gen-ai/gen-ai-events.md#event-gen_aiclientinferenceoperationdetails .. "opt-in" is then defined as (https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/signal-requirement-level.md):

"Instrumentations SHOULD emit the signal if and only if the user configures the instrumentation to do so. Instrumentations that don't support configuration MUST NOT emit Opt-In signals.

This requirement level is recommended for signals that are expensive to retrieve, usually pose a security or privacy risk, or are not essential for most applications. These should therefore only be enabled deliberately by a user making an informed decision."

This brings up some issues:

Now that there is a generic event.enabled opt-in / opt-out feature, this should probably be re-worded to recommend that be used instead of per-instrumentation custom configuration...

Should the event be marked "opt=in".. IMO the only argument for that is that it isn't "essential for most applications".. I'd prefer it not be opt-in but i'm fine either way...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I don't think this PR satisfies the opt-in requirement right now.

Should the event be marked "opt=in".. IMO the only argument for that is that it isn't "essential for most applications"..

Agreed. Maybe we should update the semconv if the plan is to rely on the log level for opting in/out. But the default behavior is still duplicative of the span right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify the default behavior and opt-in requirement:

  1. The event is strictly opt-in by default (NOT emitted unless configured):

    • By default (when OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT is unset), get_content_capturing_mode() defaults to ContentCapturingMode.NO_CONTENT.
    • When unset, _should_emit_event() evaluates to False for both NO_CONTENT and SPAN_ONLY.
    • Therefore, by default self._emit_event is False, and _maybe_create_event() produces None. Zero events are emitted by default, avoiding any span/event duplication out of the box (verified by test_does_not_emit_llm_event_by_default_for_no_content and test_does_not_emit_llm_event_by_default_for_span_only).
  2. How opt-in is configured:

    • A user explicitly opts into events either by:
      a) Setting OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=EVENT_ONLY or SPAN_AND_EVENT, or
      b) Setting OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=true.
    • Setting SeverityNumber.DEBUG ensures semconv compliance, while checking self._logger.enabled(severity_number=SeverityNumber.DEBUG) allows standard logger filters and log processors to suppress or route emissions without mutating process-wide state.
  3. Issue google-genai: instrument_generate_content() unconditionally sets OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=true, overriding config and user setting #619 resolution:

    • In google-genai, instrument_generate_content() previously mutated global state via os.environ[OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT] = "true", which broke the opt-in invariant across all downstream invocations.
    • That global environment mutation is eliminated in this PR, restoring user configuration sovereignty and respecting the opt-in default.

Commit 12b5ed50 also restores opentelemetry-api ~= 1.43 in util/opentelemetry-util-genai/pyproject.toml (resolving dependency conflicts with opentelemetry-semantic-conventions 0.64b0) and ensures completion hooks are not starved when loggers are disabled.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

google-genai: instrument_generate_content() unconditionally sets OTEL_INSTRUMENTATION_GENAI_EMIT_EVENT=true, overriding config and user setting

5 participants