[ps48]

Description

Agents served behind an HTTP handler produce traces whose root is an HTTP SERVER span (for example POST /runs), with the GenAI spans (invoke_agent, chat, execute_tool) below it. OpenSearch Dashboards Agent Traces lists a trace by a root that has gen_ai.operation.name, so these traces were missing from the Traces and Sessions tabs (#7191).

As proposed in #7191 (comment), otel_traces now marks the trace's AI root, not the HTTP root:

• Marker: each outermost GenAI span of a trace (a span with gen_ai.operation.name and no GenAI ancestor) gets derived.gen_ai.is_root = true. That's the root itself when it is a GenAI span, or the agent span below a non-GenAI root, so every agent trace carries one marker. The token usage of its descendants is aggregated onto it, unless it already has gen_ai.usage.input_tokens.

• Parallel agents: several top-level agents under one root are each marked.

• Late roots: spans flushed after trace_flush_interval without their root are still marked, because the marker only needs the spans below it. Root enrichment cannot help in that case.

• Source markers: if any span in the trace already carries derived.gen_ai.is_root (set by the GenAI SDKs or by hand, with any value), the processor marks nothing else in that part of the trace. Setting it to false opts a trace out.

• Known edge cases (documented):

  • a span that arrives after trace_group_cache_ttl may be marked as well;
  • if a trace's children are flushed before a GenAI root arrives, both the root and the outermost child can be marked. Consumers prefer the marked span with no parent, as Agent Traces does.

• Root propagation: gen_ai.agent.name, gen_ai.request.model and the other propagated keys now come from the outermost GenAI spans first (earliest start time), instead of whichever child came first in the batch.

• README: fixed the propagated-keys list (gen_ai.operation.name was listed but never propagated) and documented the marker.

• Token totals counted once: root enrichment used to sum token usage over every child, and the marker did the same over its descendants. Agent spans such as invoke_agent often report the total of their own LLM calls, so those calls were counted twice; on a real Strands trace the root showed about 2× the actual usage. Both now count only spans that have no ancestor reporting usage. Root enrichment also runs before marking, so the root doesn't count the marker's aggregate.

• Late spans: spans that arrive after their trace's root was processed are enriched on their own, without their ancestors, so a late chat span or a called service's spans looked outermost. The processor now remembers, per trace and for trace_group_cache_ttl, which span ids are covered by a GenAI span or a marker (bounded at 500,000 span ids), and checks there when a parent isn't in the batch.

• Agents calling agents: an agent that calls another agent over HTTP or RPC in the same trace (invoke_agent > HTTP client > HTTP server > invoke_agent) gets one mark, on the calling agent, because the called agent has a GenAI ancestor. Edge case: if the called service's spans are flushed after trace_flush_interval without the caller's spans, the called agent is marked too, and Agent Traces lists it as a separate row. The same happens if that service marks its own span with the SDK. This is documented in the README.

Why mark the span instead of copying gen_ai.operation.name to the root: copying would make the HTTP span look like a GenAI span (it would show up as an extra Agent span), the trace row would still have no input or output, and late roots would stay hidden. Datadog and OpenInference also treat the outermost GenAI span as the agent trace; details in the issue comment.

Related:

• OpenSearch Dashboards Agent Traces lists AI roots: [Agent Traces] List agents behind an HTTP handler by their marked AI root span OpenSearch-Dashboards#12977
• GenAI SDKs mark_agent_root() / markAgentRoot(): feat: add mark_agent_root() to mark a trace's agent root span genai-observability-sdk-py#48, feat: add markAgentRoot() to mark a trace's agent root span genai-observability-sdk-ts#88

Issues Resolved

Resolves #7191

Testing

• New GenAiAgentRootTest:

  • an HTTP root with shuffled children: only invoke_agent is marked, and it gets the summed tokens without double counting the root;
  • outer-agent propagation;
  • a GenAI root, which is marked itself;
  • parallel agents, which are both marked;
  • an orphan batch, where the outermost span is marked;
  • a source marker, which is kept;
  • a non-GenAI trace, which gets no marks;
  • the processor path where the root arrives last;
  • an agent calling another agent over HTTP (only the caller is marked), and the same trace with the caller's spans missing (the called agent is marked);
  • late spans: a chat arriving after its agent, a called service arriving after its caller, and a span below a source-marked span are not marked, both through the helper and through the processor;
  • an invoke_agent that reports the total of its chats, and a nested sub-agent that does, are each counted once, on the marker and on the root.

• ./gradlew :data-prepper-plugins:otel-trace-raw-processor:test plus checkstyle pass (53 tests, JDK 17).

• End to end on observability-stack, with a Data Prepper image built from this branch, OpenSearch 3.9.0, and a FastAPI app instrumented with OTel and the GenAI Python SDK:

  Case	Result in OpenSearch
  HTTP root POST /runs > invoke_agent > chat, execute_tool > chat	invoke_agent marked, tokens 150/30. Root not marked; root totals 150/30.
  The SDK marks the HTTP span	Only the HTTP span is marked.
  GenAI root	No marks (behaviour before this update; GenAI roots are now marked themselves).
  Two agents under one HTTP root	Both marked (150/30 each). Root totals 300/60; root agent name is the earliest agent.
  Root held open for 400s	Children flushed as orphans (There are 4 spans with missing trace groups). The late root has no enrichment, but invoke_agent is marked with 150/30.

• Replay of a real Strands on Bedrock AgentCore trace (389 spans: an outer invoke_agent, a nested sub-agent under execute_tool, and calls to other services whose parent spans weren't exported):

  • only the outer invoke_agent is marked, both with the trace root present and with the root removed. In the second case all 388 spans are flushed as orphans and the marker still applies;
  • the root's tokens now match the agent span (351,830 input); before this change the root showed 707,679.

Check List

• New functionality includes testing.
• New functionality has a documentation issue. Please link to it in this PR.
  • New functionality has javadoc added
• Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 242387f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible NPE on propagation In enrichRootSpan, when a propagated attribute value is an empty string, entry.getValue() == null remains true and iteration continues, but once set, value.toString() is called without null-checks on later children's values (fine). However, for the README documented propagation, strings are expected but attributes could be non-String Objects; toString() on complex values (e.g., Map/List from vendor normalization) would propagate a non-semantic representation to the root. Consider restricting propagation to String values only, matching the previous (String) attrs.get(...) cast behavior. for (final SpanInfo child : ordered) { for (final Map.Entry<String, String> entry : toPropagate.entrySet()) { final Object value = child.attributes.get(entry.getKey()); if (entry.getValue() == null && value != null) { entry.setValue(value.toString()); } } Potential overflow / huge TTL Duration.ofMillis(2 * traceFlushInterval).plus(...) uses long arithmetic on 2 * traceFlushInterval where traceFlushInterval is derived from getTraceFlushIntervalSeconds() * 1000. If a user configures a very large flush interval, 2 * traceFlushInterval could overflow or produce an unreasonably large cache TTL. Consider clamping/validating or using Duration.ofSeconds(2L * configSeconds) to be explicit, and defend against overflow. genAiCoverage = GenAiCoverage.create(GENAI_COVERAGE_MAX_SPAN_IDS, Duration.ofMillis(2 * traceFlushInterval).plus(otelTraceRawProcessorConfig.getTraceGroupCacheTimeToLive()));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 242387f

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix ancestor lookup on cyclic parent chains When the walk terminates because the next parent is not in the batch, parentId holds the id of the first out-of-batch parent and is checked against known. However, if the walk terminates due to a cycle (seen.add returning false), parentId still points to an in-batch span, and that id may spuriously be found in known, incorrectly reporting an ancestor match. Track termination reason explicitly and only consult known when the parent was actually absent from the batch. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [216-228] private boolean hasAncestor(final SpanInfo info, final Predicate<SpanInfo> matches, final Set<String> known) { final Set<String> seen = new HashSet<>(); String parentId = info.parentSpanId(); SpanInfo parent = bySpanId.get(parentId); - while (parent != null && seen.add(parent.spanId())) { + while (parent != null) { + if (!seen.add(parent.spanId())) { + return false; + } if (matches.test(parent)) { return true; } parentId = parent.parentSpanId(); parent = bySpanId.get(parentId); } return parentId != null && known.contains(parentId); Suggestion importance[1-10]: 6 __ Why: A valid edge-case concern: if a cycle exists in parent chains, falling through to check known.contains(parentId) with an in-batch id could cause a spurious match. Cycles are unlikely in real trace data but the correctness fix is reasonable.	Low
General	Parse token counts as integers Parsing token counts via Double.parseDouble silently truncates very large values (loss of precision beyond 2^53) and accepts non-integer forms like "NaN"/"Infinity", which cast to 0 or Long.MAX_VALUE and corrupt aggregates. Prefer Long.parseLong on the trimmed string, falling back to null on failure, since token counts are integral. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [393-405] -private Long toLong(final Object value) { +private static Long toLong(final Object value) { if (value instanceof Number) { return ((Number) value).longValue(); } if (value instanceof String) { try { - return (long) Double.parseDouble(((String) value).trim()); + return Long.parseLong(((String) value).trim()); } catch (final NumberFormatException e) { return null; } } return null; } Suggestion importance[1-10]: 5 __ Why: Valid point about Double.parseDouble accepting NaN/Infinity and causing precision loss. However, the existing test shows the current code intentionally tolerates numeric strings; switching to Long.parseLong would reject values like "12.0". The concern is real but the fix may be too strict.	Low
	Avoid overflow when computing TTL duration traceFlushInterval is a long of milliseconds derived from a user-configured seconds value; 2 * traceFlushInterval can overflow or produce a negative duration for large configured values, which would make Duration.ofMillis throw or create an invalid TTL. Compute the duration via Duration.ofMillis(traceFlushInterval).multipliedBy(2) or guard against overflow. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/OTelTraceRawProcessor.java [84-85] genAiCoverage = GenAiCoverage.create(GENAI_COVERAGE_MAX_SPAN_IDS, - Duration.ofMillis(2 * traceFlushInterval).plus(otelTraceRawProcessorConfig.getTraceGroupCacheTimeToLive())); + Duration.ofMillis(traceFlushInterval).multipliedBy(2).plus(otelTraceRawProcessorConfig.getTraceGroupCacheTimeToLive())); Suggestion importance[1-10]: 4 __ Why: Overflow is theoretically possible but highly unlikely with realistic trace flush interval configurations. The improved code is also not materially safer against overflow since multiplication still occurs.	Low
	Prevent external mutation of cached sets The returned Set is stored in the cache and then handed back to subsequent callers as the known set in later enrich invocations. Since downstream code calls containsAll on it but could also mutate it (e.g. via coveredSpanIds building new sets), wrap the merged set in Collections.unmodifiableSet to prevent accidental external mutation of cached state across concurrent trace updates. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [60-65] if (added.isEmpty() || known.containsAll(added)) { return existing; } final Set<String> merged = new HashSet<>(known); merged.addAll(added); -return merged; +return Collections.unmodifiableSet(merged); Suggestion importance[1-10]: 3 __ Why: The suggestion is in the wrong file (the code is in GenAiCoverage.java, not GenAiEnrichmentHelper.java). The hardening is minor and defensive; current callers do not appear to mutate the known set.	Low

---

Previous suggestions

Suggestions up to commit 96742f1

Category	Suggestion	Impact
General	Avoid overwriting partial token attributes The guard uses containsKey(GEN_AI_INPUT_TOKENS_KEY) to skip aggregation, but enrichRootSpan uses the same check for rootHasTokens. If only GEN_AI_OUTPUT_TOKENS_KEY is present on the agent root (an edge case), aggregation will overwrite nothing for input but still set input to 0 via the write path below. Consider checking both keys or symmetrically only writing the missing one to avoid overwriting partial pre-existing data. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [211-214] final Map<String, Object> attrs = agentRoot.getAttributes(); -if (attrs != null && attrs.containsKey(GEN_AI_INPUT_TOKENS_KEY)) { +if (attrs != null && (attrs.containsKey(GEN_AI_INPUT_TOKENS_KEY) + || attrs.containsKey(GEN_AI_OUTPUT_TOKENS_KEY))) { continue; } Suggestion importance[1-10]: 5 __ Why: This addresses a plausible edge case where only output tokens are present on the agent root, avoiding an inconsistent write. The impact is minor since this scenario is uncommon, but the fix improves symmetry with enrichRootSpan behavior.	Low
	Protect cached sets from external mutation The merge remap function on a Caffeine-backed asMap() must execute atomically, but reads via getIfPresent return the stored Set directly and callers iterate it (e.g., known.contains(parentId) in hasGenAiAncestor). If another thread concurrently calls record for the same trace, the returned merged set replaces the old one but readers holding a reference to the old set are fine; however, exposing the mutable HashSet risks accidental mutation. Wrap the stored set as unmodifiable (or return a copy from covered) to prevent callers from mutating cached state. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiCoverage.java [55-64] void record(final String traceId, final Collection<String> spanIds) { if (coveredByTrace == null || traceId == null || spanIds.isEmpty()) { return; } - coveredByTrace.asMap().merge(traceId, new HashSet<>(spanIds), (existing, added) -> { + coveredByTrace.asMap().merge(traceId, Set.copyOf(spanIds), (existing, added) -> { final Set<String> merged = new HashSet<>(existing); merged.addAll(added); - return merged; + return Collections.unmodifiableSet(merged); }); } Suggestion importance[1-10]: 4 __ Why: Making cached sets unmodifiable is a reasonable defensive measure to prevent accidental mutation by callers, though the current code doesn't appear to mutate them, so the practical impact is limited.	Low
	Verify opt-out semantics for existing markers The README states that a marker set at the source with any value (including false) opts the trace out of further marking, but the current short-circuit uses return, which only exits markAgentRoots. The caller still proceeds to call coveredSpanIds(...) which recognizes the marked span via isMarked, so coverage recording is consistent — however, if markAgentRoots is ever called standalone this still returns correctly. More importantly, verify this early-return occurs before any mutation; it does, so this is OK, but ensure consistent behavior across both overloads. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [197-202] +static void markAgentRoots(final Collection<Span> traceSpans, final Set<String> known) { + for (final Span span : traceSpans) { + final Map<String, Object> attrs = span.getAttributes(); + if (attrs != null && attrs.containsKey(DERIVED_GEN_AI_IS_ROOT_KEY)) { + return; + } + } - Suggestion importance[1-10]: 2 __ Why: This suggestion is largely a verification request that concludes the current code is OK, with no concrete code change proposed (the improved_code equals existing_code).	Low

Suggestions up to commit 5198311

Category	Suggestion	Impact
General	Make token-presence check symmetric The code checks only GEN_AI_INPUT_TOKENS_KEY but hasTokens and the aggregation include output tokens too. If a span reports only output tokens, this branch will skip aggregation inconsistently, and later sumTopmostTokens could still miss or double-count. Make the skip condition consistent with hasTokens (either both keys or either key) to avoid asymmetric behavior. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [157-160] final Map<String, Object> attrs = agentRoot.getAttributes(); -if (attrs != null && attrs.containsKey(GEN_AI_INPUT_TOKENS_KEY)) { +if (attrs != null + && (attrs.containsKey(GEN_AI_INPUT_TOKENS_KEY) || attrs.containsKey(GEN_AI_OUTPUT_TOKENS_KEY))) { continue; } Suggestion importance[1-10]: 5 __ Why: Reasonable consistency improvement: hasTokens treats either token key as present, so the skip condition on agentRoot should mirror that to avoid overwriting an existing output-only token value.	Low
	Guard ancestor walk against null span IDs If a parent span has a null spanId, seen.add(null) succeeds once but any subsequent null will stop the walk prematurely, and more importantly bySpanId lookups by null parent IDs may collide. Guard against null spanId to make the cycle-detection robust and consistent with how bySpanId is populated (which skips null span IDs). data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [125-135] private static boolean hasGenAiAncestor(final Span span, final Map<String, Span> bySpanId) { final Set<String> seen = new HashSet<>(); Span parent = bySpanId.get(span.getParentSpanId()); - while (parent != null && seen.add(parent.getSpanId())) { + while (parent != null && parent.getSpanId() != null && seen.add(parent.getSpanId())) { if (isGenAiSpan(parent)) { return true; } parent = bySpanId.get(parent.getParentSpanId()); } return false; } Suggestion importance[1-10]: 3 __ Why: Minor defensive hardening; bySpanId only stores non-null span IDs, so the risk of null-id collisions is low in practice, but the guard is a small robustness improvement.	Low
	Handle null span IDs in traversal The descendant collection does not exclude other outermost GenAI agent roots (e.g. parallel sibling agents are only siblings, but any nested agent reached transitively through a non-GenAI span would be counted). More importantly, if a descendant has a null spanId, seen.add(null) succeeds once then blocks all subsequent null-id spans. Skip null span IDs explicitly to avoid dropping unrelated nodes from traversal. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [162-172] final List<Span> pending = new ArrayList<>( childrenByParent.getOrDefault(agentRoot.getSpanId(), List.of())); final Set<String> seen = new HashSet<>(); while (!pending.isEmpty()) { final Span descendant = pending.remove(pending.size() - 1); - if (!seen.add(descendant.getSpanId())) { + if (descendant.getSpanId() == null || !seen.add(descendant.getSpanId())) { continue; } descendants.add(descendant); pending.addAll(childrenByParent.getOrDefault(descendant.getSpanId(), List.of())); } Suggestion importance[1-10]: 3 __ Why: Similar minor hardening against null spanId values in the DFS traversal; low practical impact since spans typically have non-null IDs, but adds consistency.	Low

Suggestions up to commit 3e5a02f

Category	Suggestion	Impact
General	Skip aggregation when any token already set agentRoot.getAttributes() is fetched after the put call above, so attrs may already reflect the newly set marker but that's fine; however, the check only skips when gen_ai.usage.input_tokens is present. If only output_tokens is pre-set on the agent root, the code will overwrite it with the aggregated sum, inconsistent with the "skip-if-present" semantics. Consider skipping if either token key is present, or document the asymmetry. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [155-160] for (final Span agentRoot : outermostGenAiSpans(traceSpans)) { agentRoot.put(ATTRIBUTES_PREFIX + DERIVED_GEN_AI_IS_ROOT_KEY, true); final Map<String, Object> attrs = agentRoot.getAttributes(); - if (attrs != null && attrs.containsKey(GEN_AI_INPUT_TOKENS_KEY)) { + if (attrs != null && (attrs.containsKey(GEN_AI_INPUT_TOKENS_KEY) + || attrs.containsKey(GEN_AI_OUTPUT_TOKENS_KEY))) { continue; } Suggestion importance[1-10]: 4 __ Why: Reasonable observation about asymmetric skip-if-present behavior for output_tokens, but it's a minor edge case and the current logic is likely intentional (checking only input_tokens as the primary indicator).	Low
	Guard cycle-detection set against null IDs The seen set is scoped per-agentRoot, so when there are multiple parallel top-level agents under a non-GenAI root, descendants shared by none but visited via one subtree are fine, but a descendant whose spanId is null will cause seen.add(null) collisions and skip legitimate siblings. Guard against null spanId before using it as a set key. data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/GenAiEnrichmentHelper.java [164-171] final List<Span> pending = new ArrayList<>( childrenByParent.getOrDefault(agentRoot.getSpanId(), List.of())); final Set<String> seen = new HashSet<>(); while (!pending.isEmpty()) { final Span descendant = pending.remove(pending.size() - 1); - if (!seen.add(descendant.getSpanId())) { + if (descendant.getSpanId() != null && !seen.add(descendant.getSpanId())) { continue; } Suggestion importance[1-10]: 3 __ Why: The concern about null spanId is minor; HashSet handles a single null fine, and the suggested change actually alters semantics (a non-null spanId no longer deduped when seen.add returns false is handled correctly, but a null spanId would skip the dedup entirely, potentially causing infinite loops if cycles exist). Low impact and arguably unnecessary.	Low

[kylehounslow]

Propagates gen_ai.system, gen_ai.provider.name, gen_ai.agent.name, and gen_ai.request.model from child spans to root (skip-if-present)

Please create a follow-up issue to remove this propagation. Once this change has stabilized

[ps48]

Agreed. I'll open a follow-up issue to remove the propagation once the marker has shipped and stabilized.

With the marker, consumers read agent data (name, model, input/output, tokens) from the agent span itself, so the copies on the root become redundant. They're also misleading: an HTTP or runtime span ends up carrying a model, an agent name and tokens it didn't produce.

Before removing it:

• the OpenSearch Dashboards trace flyout header has to read the AI root instead of the top span. That's now in [Agent Traces] List agents behind an HTTP handler by their marked AI root span OpenSearch-Dashboards#12977;
• the change needs a release note, since it changes the data on root spans for existing pipelines.

While checking this on a real Strands trace, I found that root enrichment double counted tokens. It summed every child with usage, including invoke_agent spans that already report the total of their own chat calls, so the root showed about 2× the real usage. The new marker aggregation had the same flaw. Both now count only spans that have no ancestor reporting usage. The fix is in this PR (5198311), with tests.

[kylehounslow]

on each outermost GenAI span: a span with gen_ai.operation.name and no GenAI ancestor.

How would this work when agents make http requests to services which result in no gen_ai attributes? E.g.

(1)Agent --> (2)http request --> (3)Agent --> (4)MCP tool --> etc... 

Would spans (1) and (3) be listed as separate agent traces in OSD front-end?

[ps48]

Good question. In the normal case they're listed as one agent trace:

POST /runs (root, no gen_ai)
└── (1) invoke_agent Planner            <- marked
    └── (2) HTTP client > HTTP server (other service, no gen_ai)
        └── (3) invoke_agent Weather    <- not marked: (1) is a GenAI ancestor
            └── (4) execute_tool (MCP)

All of these spans share the trace ID, and otel_traces holds the children until the root arrives, so the whole chain is usually in one batch. (3) has a GenAI ancestor through (2), so only (1) is marked. If (1) is the trace root, nothing is marked.

There is one edge case where (3) is listed separately. If the called service's spans are flushed after trace_flush_interval without (1) in the batch, the chain stops at the missing span, (3) gets marked too, and Agent Traces shows it as its own row. The same happens if that service marks (3) itself with the SDK.

I added tests for both (marksOnlyTheCallingAgentWhenAnAgentCallsAnotherAgentOverHttp, marksTheRemoteAgentWhenItsCallerIsMissingFromTheBatch), documented the edge case in the README, and added it to the PR description.

I also replayed a real Strands trace that includes calls to other services whose parent spans weren't exported. Only the outer invoke_agent was marked, both with the root present and with it removed.

[github-actions]

Persistent review updated to latest commit 5198311

[github-actions]

Persistent review updated to latest commit 96742f1

[kylehounslow]

LGTM! Thanks for this critical fix

[ps48]

@dlvenable / @srikanthpadakanti can you please help review and merge this PR Thanks!

[dlvenable]

Automated review (AI agent)

Note: This review was generated by an AI agent (Claude Code) run by @dlvenable. Candidate issues were surfaced by multiple review passes and each was then verified by an independent pass against the checked-out branch (not just the diff). Labels: CONFIRMED = every premise was checked against the code and holds; PLAUSIBLE = the mechanism is real in the code, but firing depends on inputs/environment that can't be proven from the repo alone. Please treat these as input for human judgment.

This is a well-tested and well-documented change — the marker semantics, edge cases, and the double-counting fix are carefully thought through, and the end-to-end validation in the PR description is thorough. The findings below are ranked most severe first; details are in the inline comments.

Correctness

1. CONFIRMED — GenAiCoverage TTL (trace_group_cache_ttl, default 15s) is disjoint from the orphan GC-flush window (trace_flush_interval, default 180s), so the late-span protection can never cover GC-flushed spans at default settings; every GC-flushed outermost GenAI orphan gets a duplicate derived.gen_ai.is_root mark.
2. CONFIRMED — the coverage.covered() → markAgentRoots() → coverage.record() sequence is a non-atomic check-then-act on state shared across pipeline worker threads; two concurrent batches of the same trace can both mark a root.
3. CONFIRMED — BY_START_TIME compares ISO-8601 start times lexicographically; Instant.toString() emits variable-length fractions, so ordering inverts across precision boundaries and the wrong agent's name/model can propagate to the root.
4. CONFIRMED — markAgentRoots skips aggregation only when the agent root has input_tokens; an agent reporting only output_tokens gets its own value overwritten.
5. PLAUSIBLE (pre-existing, widened) — unguarded (Number)/(String) casts on attribute values can throw ClassCastException and fail the whole batch.
6. CONFIRMED (narrow edge) — enrichRootSpan computes outermostGenAiSpans(children) without the known coverage set, so propagation and marking can disagree in late-root batches.

Efficiency

7. CONFIRMED — JacksonSpan.getAttributes() deserializes a fresh map per call, and the new predicates call it O(n·depth + n·log n) times per trace (previously O(n)), on the hot path for all traces including non-GenAI ones.

Design notes

8. PLAUSIBLE — the topmost-token rule can undercount nested sub-agents whose usage isn't included in the ancestor's reported total (documented trade-off; flagging for awareness).
9. Presence-based opt-out: an SDK emitting derived.gen_ai.is_root: false on every span silently disables marking for the whole trace.
10. The new 500k-span-id coverage cache has no metric gauge and no config knob; GenAiCoverage.estimatedSize() is currently dead code.

Not flagged: the README correction is accurate (gen_ai.operation.name was indeed never propagated), and all existing otel-trace-raw-processor tests pass on this branch.

[dlvenable]

[AI agent finding #1 — CONFIRMED] Coverage TTL is disjoint from the orphan-flush window, so the late-span protection never covers GC-flushed spans at defaults.

GenAiCoverage uses trace_group_cache_ttl (default 15s), but a child arriving more than 15s after its root misses traceIdTraceGroupCache, is parked in traceIdSpanSetMap, and is only flushed by getTracesToFlushByGarbageCollection after trace_flush_interval (default 180s, often 180–360s given the GC cadence). By then the coverage entry recorded when the agent was marked has always expired, so covered(traceId) returns empty and the orphan GenAI span is unconditionally marked derived.gen_ai.is_root: true.

So at default settings the window where the protection works (≤15s, the trace-group-cache-hit path) and the window where orphans are flushed (≥180s) are disjoint — the README's "a span that arrives later than that may be marked as well" is systematic for this path, not occasional.

Suggestion: give the coverage cache its own TTL of at least 2 × trace_flush_interval + trace_group_cache_ttl. The cache is already weight-bounded at 500k span ids, so a longer TTL is cheap.

[dlvenable]

[AI agent finding #2 — CONFIRMED] Non-atomic check-then-act on shared GenAiCoverage across pipeline workers.

OTelTraceRawProcessor is not @SingleThread, so a single instance is shared by all pipeline workers and doExecute runs concurrently (the class already uses ConcurrentHashMap/ReentrantLock for its other shared state). The sequence coverage.covered(traceId) → markAgentRoots(traceSpans, known) → coverage.record(...) has no synchronization, and spans of one trace can land in two concurrent batches: pass-through spans (trace group cached) race the GC flush, whose tryLock only guards its own path.

Both threads then read an empty covered set and both mark a root — e.g. the caller service's invoke_agent on worker A and the called service's invoke_agent on worker B — the exact duplication the coverage cache was added to prevent. The in-batch source-marker guard can't help across threads, and the new tests are single-threaded so they can't catch it.

Suggestion: serialize the covered-read → mark → record sequence per trace id (e.g. compute-style locking keyed by traceId, or a lock striped on traceId).

[dlvenable]

[AI agent finding #3 — CONFIRMED] Lexicographic comparison of ISO-8601 start times mis-orders across fractional precision.

Span.getStartTime() is a String produced via Instant.toString() (OTelProtoCommonUtils.convertUnixNanosToISO8601), which emits 0/3/6/9 fractional digits. "2026-01-01T00:00:01Z".compareTo("2026-01-01T00:00:01.500Z") > 0 because 'Z' (0x5A) > '.' (0x2E) — the later timestamp sorts first.

This breaks the "earliest first" contract of outermostGenAiSpans and the tiebreak in enrichRootSpan's sort: with two parallel top-level agents whose start times straddle a precision boundary (one landing exactly on a second/millisecond boundary), the trace root receives the wrong agent's gen_ai.agent.name/gen_ai.request.model.

Suggestion: compare parsed instants, e.g. Comparator.comparing(span -> Instant.parse(span.getStartTime()), ...) (with a null/parse-failure fallback), instead of raw strings.

[dlvenable]

[AI agent finding #4 — CONFIRMED] An agent root reporting only output_tokens gets its own value overwritten.

This guard checks only GEN_AI_INPUT_TOKENS_KEY, asymmetric with hasTokens() below, which treats either key as "reports usage". Both token attributes are only conditionally required by the OTel GenAI semconv, so an agent span carrying only gen_ai.usage.output_tokens (say 700) passes this guard, and the subsequent puts clobber its genuine value with the descendants' aggregate.

The same span would meanwhile count as a token-reporting ancestor in sumTopmostTokens, so totals become inconsistent between the root and the marker.

Suggestion: make this check symmetric with hasTokens() (skip if either key is present). The pre-existing one-key rootHasTokens check in enrichRootSpan has the same shape and could be fixed in passing.

[dlvenable]

[AI agent finding #5 — PLAUSIBLE, pre-existing but widened] Unguarded casts on attribute values can fail the whole batch.

(Number) attrs.get(...) here (and (String) attrs.get(entry.getKey()) in the propagation loop) throws ClassCastException on a string-typed token attribute. That's legal OTLP — the codec maps AnyValue string values to String, and normalizeAttributes copies vendor values (e.g. llm.usage.prompt_tokens → gen_ai.usage.input_tokens) verbatim without type conversion — so a vendor exporter emitting token counts as strings reaches this cast. The exception propagates out of doExecute and drops/DLQs the entire batch, including GC-flushed spans of unrelated traces.

The casts pre-exist on main, but this PR widens exposure: sumTopmostTokens now also runs per agent root via markAgentRoots, including when there is no trace root (the old path was gated on rootSpan != null).

Suggestion: an instanceof Number check (skip or attempt parse otherwise) makes enrichment degrade per-attribute instead of per-batch.

[dlvenable]

[AI agent finding #6 — CONFIRMED, narrow edge] Propagation ignores the known coverage set, so it can disagree with marking.

enrichRootSpan calls the outermostGenAiSpans(children) overload without known, while markAgentRoots passes it. When children were GC-flushed in an earlier batch and the root is later flushed with newly buffered children, this runs with a non-empty known it ignores — a covered descendant can win propagation of gen_ai.agent.name/gen_ai.request.model over the true outer agent.

Threading known through here (or computing the outermost list once per trace in enrichBatch and passing it to both consumers — see the efficiency comment) closes the gap.

[dlvenable]

[AI agent finding #7 — CONFIRMED] getAttributes() deserializes per call, and the new code calls it O(n·depth + n·log n) times per trace.

JacksonSpan.getAttributes() goes through JacksonEvent.get(..., Map.class) → mapper.treeToValue(...) — a full fresh deserialization on every call, including large prompt/completion payloads on GenAI spans. The previous code read each child's attributes once (O(n) per trace). The new code invokes it inside isGenAiSpan/isMarked/hasTokens, which run in per-span ancestor walks (3 passes: outermostGenAiSpans ×2 + coveredSpanIds), in enrichRootSpan's sort comparator (per comparison), and alongside 5–6 separately rebuilt bySpanId/childrenByParent indexes.

This is the hot path for all otel_traces traffic, and there's no fast-path bail-out for GenAI-free traces (markAgentRoots builds childrenByParent before discovering there's nothing to mark; enrichRootSpan copies and sorts all children even when nothing will propagate).

Suggestion: build one per-trace structure at the top of the loop in enrichBatch — bySpanId, childrenByParent, and a memoized per-span record {isGenAi, isMarked, inputTokens, outputTokens, startTime} — and thread it through. That restores O(1) deserializations per span, allows an early return for GenAI-free traces, and also collapses the duplicated hasGenAiAncestor/hasTokenAncestor walks into one parameterized helper.

[dlvenable]

[AI agent finding #8 — PLAUSIBLE, design note] Topmost-token rule can undercount nested sub-agents.

The "nearest token-reporting ancestor shadows the subtree" rule fixes the confirmed 2× Strands double-count, but in the agent-as-tool pattern a sub-agent runs its own event loop whose tokens are not included in the outer agent's counter — those now vanish from trace-level totals, with no opt-out.

This looks like a deliberate, documented trade-off, so flagging only for awareness: a README caveat ("an ancestor's reported usage is assumed to include its subtree") or a future refinement may be worth considering.

[dlvenable]

[AI agent finding #9 — design note] Presence-based opt-out may surprise SDKs that emit is_root: false everywhere.

isMarked (and the skip-check in markAgentRoots) uses containsKey, so a source that conscientiously emits derived.gen_ai.is_root: false on every non-root span — a natural reading of a boolean attribute — silently disables marking for all its traces, and nothing logs why.

Treating only value == true as a source marker, with a separate explicit opt-out mechanism, would be less surprising. If the presence-based contract is intentional (the README does document it), consider at least a debug log when a trace is skipped due to an existing marker.

[dlvenable]

[AI agent finding #10 — cleanup] estimatedSize() is dead code; the cache has no gauge and no config knob.

Nothing calls this method — it mirrors the gauge pattern used for traceIdTraceGroupCache/traceIdSpanSetMap (traceGroupCacheCount, spanSetCount), but no gauge is ever registered for genAiCoverage. Without it, operators can't see the 500k-span-id cache filling or evicting under weight pressure — and eviction produces the same late-span mismarking the cache exists to prevent.

Suggestion: register pluginMetrics.gauge(...) for it like the sibling caches (which appears to have been the intent), and consider making GENAI_COVERAGE_MAX_SPAN_IDS configurable alongside trace_group_cache_max_size.

[github-actions]

Persistent review updated to latest commit 242387f

[dlvenable]

Thank you @ps48 for this contribution!