Prototype of span / event / metric deduplication and context passing for inference span - #663
DylanRussell wants to merge 25 commits into
Conversation
There was a problem hiding this comment.
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.contexthelpers to store/retrieve inference attributes inContext. - 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.
Pull request dashboard statusWaiting on reviewers · refreshed 2026-09-21 16:45 UTC Review the latest changes. Status above doesn't look right?
|
lmolkova
left a comment
There was a problem hiding this comment.
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.
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.
SGTM
I don't follow this exactly -- can you clarify ?
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) |
|
I did a small prototype to show noop/nestedinferenceinvocation to avoid fragile flag, PTAL: |
…_context # Conflicts: # instrumentation/opentelemetry-instrumentation-genai-bedrock/tests/test_converse.py # instrumentation/opentelemetry-instrumentation-genai-bedrock/tests/test_invoke_model.py
|
I took a look at your prototype. I like just having
|
| # 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: |
There was a problem hiding this comment.
_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")There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alright that way looks pretty good too.. Sent out #765
There was a problem hiding this comment.
To handle metric attributes the dictionary in the context now has this structure:
{
'spanevent_attributes': {}
'metric_attributes': {}
}
|
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:
|
…_context # Conflicts: # util/opentelemetry-util-genai/src/opentelemetry/util/genai/_inference_invocation.py # util/opentelemetry-util-genai/src/opentelemetry/util/genai/_invocation.py
Description
For
InferenceInvocationsthe 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
How has this been tested?
Unit tests
Checklist