Skip to content

Prototype of span / event / metric deduplication and context passing for inference span - #663

Open
DylanRussell wants to merge 25 commits into
open-telemetry:mainfrom
DylanRussell:prototype_adding_on_context
Open

DylanRussell wants to merge 25 commits into
open-telemetry:mainfrom
DylanRussell:prototype_adding_on_context

Conversation

@DylanRussell

@DylanRussell DylanRussell commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Description

For InferenceInvocations the outermost instrumentation puts an empty dict onto the context to suppress duplicate downstream instrumentation from emitting spans / events / metrics.

All inner instrumentations (usually just 1) put all span attributes (except content attributes, which we disable completely from inner instrumentations to avoid extra processing and other issues) onto the context upon completion.

The outer instrumentation reads all those attributes at completion time, and sets them as the default attribute set to put on the span/event, and then sets the attributes it knows about (overwriting the context ones when there's a conflict).

We have a hardcoded a set of these attributes that should be applied on metrics -- we also look for token count attributes on the context.

There is a "metric_attributes" property on _invocation.. for now I'm not putting it onto the context, but maybe we want metric attributes to be passed similar to span/event attributes, we just need a way to differentiate them.. We could use a special prefix key on the attributes dict to do this.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

Unit tests

Checklist

  • Followed the style guidelines of this project
  • Changelog updated if the change requires an entry
  • Unit tests added
  • Documentation updated

Copilot AI lite review requested due to automatic review settings September 9, 2026 21:02
@DylanRussell
DylanRussell requested a review from a team as a code owner September 9, 2026 21:02
@DylanRussell
DylanRussell marked this pull request as draft September 9, 2026 21:02

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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds a GenAI inference context mechanism that allows nested inference invocations to deduplicate spans/events/metrics while still propagating/enriching shared inference attributes via OpenTelemetry context.

Changes:

  • Introduces opentelemetry.util.genai.context helpers to store/retrieve inference attributes in Context.
  • Updates invocation lifecycle to support “already started upstream” nested inference calls (dedup), including attribute/metric enrichment from context.
  • Adds/updates unit tests covering context propagation, nested inference dedup/enrichment, and workflow parent/child span relationships.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
util/opentelemetry-util-genai/tests/test_utils.py Updates parent/child relationship test to use workflow span as parent.
util/opentelemetry-util-genai/tests/test_context.py Adds tests for inference attribute context, nested dedup/enrichment, and metrics behavior.
util/opentelemetry-util-genai/src/opentelemetry/util/genai/context.py New module providing context key + get/set helpers for inference attributes.
util/opentelemetry-util-genai/src/opentelemetry/util/genai/_invocation.py Adds already_started flow, early-return start/finish behavior, and streaming changes for dedup scenarios.
util/opentelemetry-util-genai/src/opentelemetry/util/genai/_inference_invocation.py Implements publishing to context, context-based enrichment, and dedup finish behavior for nested inference.
util/opentelemetry-util-genai/src/opentelemetry/util/genai/init.py Re-exports context helpers as part of the public package surface.

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

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-09-21 16:45 UTC

Review the latest changes.

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.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@lmolkova lmolkova left a comment

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.

Great start!

Some ideas I was thinking of:

  • we should keep typed properties rather than raw attribute in a dict
  • we need to make it flexible and work for other invocations eventually
  • it should be easy to read and maintain, so having something like noop/nestedInferenceInvocation would help with it. It would just proxy setters to inner typed attribute representation
  • it should be completely invisible to instrumentation - it does not need to know about suppression like it does not care about parent spans.

Comment thread util/opentelemetry-util-genai/src/opentelemetry/util/genai/__init__.py Outdated
Comment thread util/opentelemetry-util-genai/src/opentelemetry/util/genai/_invocation.py Outdated
@DylanRussell

Copy link
Copy Markdown
Contributor Author

we should keep typed properties rather than raw attribute in a dict

My main concern with this one is it makes it hard to use for instrumentations outside this repo.. I see some benefit to this because we can pass unserialized data structures around the instrumentations, but I don't see a use case for that yet.

we need to make it flexible and work for other invocations eventually

SGTM

it should be easy to read and maintain, so having something like noop/nestedInferenceInvocation would help with it. It would just proxy setters to inner typed attribute representation

I don't follow this exactly -- can you clarify ?

it should be completely invisible to instrumentation - it does not need to know about suppression like it does not care about parent spans.

Mostly SGTM.. It might be interesting for instrumentations to know so they can avoid doing costly work (i'm thinking of the content-capture parsing, but maybe there's other stuff too)

@lmolkova

Copy link
Copy Markdown
Member

I did a small prototype to show noop/nestedinferenceinvocation to avoid fragile flag, PTAL:

lmolkova@e0de3d878

…_context

# Conflicts:
#	instrumentation/opentelemetry-instrumentation-genai-bedrock/tests/test_converse.py
#	instrumentation/opentelemetry-instrumentation-genai-bedrock/tests/test_invoke_model.py
@DylanRussell

DylanRussell commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

I took a look at your prototype. I like just having self.already_started (maybe a better name is needed) be the thing that differentiates inner/outer, I don't like having to have an entire new class.. I did steal a couple things from your prototype:

  1. set self.span to get_current_span when already_started is true
  2. Set content capture to false when already_started is true
  3. Skip parsing streaming chunks on inner

Comment thread util/opentelemetry-util-genai/src/opentelemetry/util/genai/_invocation.py Outdated
Comment thread util/opentelemetry-util-genai/src/opentelemetry/util/genai/_invocation.py Outdated
# Error attributes are not recorded on inner finish to isolate errors;
# the outer invocation records them only if the error escapes unhandled.
existing_attrs = get_inference_attributes()
if existing_attrs is not 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.

_finish_already_started() copies self.attributes (span/event-only) into context, but drops self.metric_attributes. Low-cardinality custom metric attributes set by inner instrumentations are lost when the outer invocation records metrics.

Failing test that should pass:

def test_nested_custom_metric_attributes_propagated(self) -> None:
    with self.handler.inference("outer"):
        with self.handler.inference("inner") as inner:
            inner.metric_attributes["custom.metric_tag"] = "low-cardinality-val"

    metrics = self._harvest_metrics()
    point = metrics["gen_ai.client.operation.duration"][0]
    self.assertEqual(point.attributes.get("custom.metric_tag"), "low-cardinality-val")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes i know but what's the solution ? I was thinking we could prefix the metric keys when they go into the context, so we know they should only be applied to metrics.. Or we could just leave out metric keys to begin with

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.

We can put an object on the context that contains multiple things such as span+event attributes and metric attributes. Or, we could create a dataclass for all invocation props and put that on the context - suppressed invocations would put things into that (as it already does) and outer would reconcile.

I'm really interested to figure out right design so we could codegen it - ptal at the #702 when you have a moment. Boilerplate and repetitions are not a concern at all for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright that way looks pretty good too.. Sent out #765

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To handle metric attributes the dictionary in the context now has this structure:

{ 
'spanevent_attributes': {}
'metric_attributes': {}
}

@DylanRussell

DylanRussell commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Made a bunch of changes, thanks for the review.. I still think putting already_started inside the baseclass and adding some branching logic there is not bad, there isn't that much additional logic to parse..

I added some clarifications to the code:

  • Change already_started to _already_started and move it to the base class, the child class doesn't need to know about it. All the child class has to do now is define _context_attributes_key, implement _finish_already_started and read the context in _apply_finish .
  • _context_token in the baseclass will be None for inner invocations, because it doesnt attach a new context, just uses the existing context.
  • I added a finished bool that we set inside finish in the base class so that we dont have to use context_token as a proxy for finished, just make an explicit class var.
  • Clarified that inside apply_finish in the child class we filter out start_attributes already applied by the outer instrumentation from the context_attributes because these have already been set on the span at start time and otherwise we might overwrite these. Also added a call here to self._invalidate_metric_attributes() because we may have picked up new metric attributes from the context..

…_context

# Conflicts:
#	util/opentelemetry-util-genai/src/opentelemetry/util/genai/_inference_invocation.py
#	util/opentelemetry-util-genai/src/opentelemetry/util/genai/_invocation.py

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.

3 participants