feat(otel): add canonical attribute normalization#115
Conversation
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.
Verdict: request_changes — Solid, well-tested refactor, but the new finally-block in start_splunk_ao_span can mask user exceptions, and the ns precision round-trip loses data.
General Comments
- 🟡 minor (question): A2A span attribute keys were renamed from
a2a.task.id/a2a.context_id/a2a.rpc.method/a2a.task.stateto thesplunk_ao.a2a.*namespace, andgen_ai.system="a2a"was dropped entirely. The removed comment on these constants previously stated they "match API-side A2A extension expectations." Is the API/ingest side already updated to parse thesplunk_ao.a2a.*keys (and to no longer rely ongen_ai.system)? If not, this is a wire-contract break that would silently drop A2A task/context correlation on the backend. Please confirm the coordinated rollout.
| with tracer.start_as_current_span(galileo_span.name) as span: | ||
| yield span | ||
| # Set dataset attributes for ground truth/reference output support | ||
| _apply_dataset_attributes( | ||
| span, galileo_span.dataset_input, galileo_span.dataset_output, galileo_span.dataset_metadata | ||
| ) | ||
| if isinstance(galileo_span, RetrieverSpan): | ||
| _set_retriever_span_attributes(span, galileo_span) | ||
| elif isinstance(galileo_span, ToolSpan): | ||
| _set_tool_span_attributes(span, galileo_span) | ||
| elif isinstance(galileo_span, WorkflowSpan): | ||
| _set_workflow_span_attributes(span, galileo_span) | ||
| try: | ||
| yield span | ||
| finally: | ||
| for key, value in build_span_attributes(galileo_span, _session_id_context.get(None)).items(): | ||
| span.set_attribute(key, value) |
There was a problem hiding this comment.
🟠 major (bug): Moving attribute finalization into finally means it now also runs on the exception path (intended, per the PR description). But build_span_attributes can itself raise — TypeError for an unsupported span type (its own else branch), or an error while serializing a partially-populated span that failed mid-operation. When the body already raised, an exception here replaces the user's original exception; when the body succeeded, it turns instrumentation into a hard failure. Instrumentation must never throw out of the wrapped operation. Guard the finalization and log instead of propagating.
| with tracer.start_as_current_span(galileo_span.name) as span: | |
| yield span | |
| # Set dataset attributes for ground truth/reference output support | |
| _apply_dataset_attributes( | |
| span, galileo_span.dataset_input, galileo_span.dataset_output, galileo_span.dataset_metadata | |
| ) | |
| if isinstance(galileo_span, RetrieverSpan): | |
| _set_retriever_span_attributes(span, galileo_span) | |
| elif isinstance(galileo_span, ToolSpan): | |
| _set_tool_span_attributes(span, galileo_span) | |
| elif isinstance(galileo_span, WorkflowSpan): | |
| _set_workflow_span_attributes(span, galileo_span) | |
| try: | |
| yield span | |
| finally: | |
| for key, value in build_span_attributes(galileo_span, _session_id_context.get(None)).items(): | |
| span.set_attribute(key, value) | |
| with tracer.start_as_current_span(galileo_span.name) as span: | |
| try: | |
| yield span | |
| finally: | |
| try: | |
| for key, value in build_span_attributes(galileo_span, _session_id_context.get(None)).items(): | |
| span.set_attribute(key, value) | |
| except Exception: | |
| logger.warning("Failed to finalize Splunk AO span attributes", exc_info=True) |
🤖 Generated by the Astra agent
| if metrics.time_to_first_token_ns is not None: | ||
| attrs["gen_ai.response.time_to_first_chunk"] = metrics.time_to_first_token_ns / 1_000_000_000 | ||
| attrs["splunk_ao.llm.time_to_first_token_ns"] = metrics.time_to_first_token_ns |
There was a problem hiding this comment.
🟡 minor (bug): The exact splunk_ao.llm.time_to_first_token_ns set here is later clobbered by normalize_attributes_for_export, which derives it from gen_ai.response.time_to_first_chunk via _alias_value (int(float(seconds) * 1e9)). That round-trip is lossy for values that aren't exact multiples: e.g. ns=123456789 → 0.123456789 s → int(...*1e9)=123456788, degrading the precise value that was already available. Consider not overwriting splunk_ao.llm.time_to_first_token_ns when it's already present in the source attrs (only derive it from the seconds field when the ns field is absent, i.e. the user-OTel path). The existing tests use round values (0.125, 0.25) and don't exercise this.
🤖 Generated by the Astra agent
| SPLUNK_ALIAS_BY_GEN_AI: Mapping[str, str] = { | ||
| "gen_ai.system": "splunk_ao.provider.name", | ||
| "gen_ai.operation.name": "splunk_ao.operation.name", | ||
| "gen_ai.conversation.id": "splunk_ao.session.id", | ||
| "gen_ai.workflow.name": "splunk_ao.workflow.name", | ||
| "gen_ai.agent.name": "splunk_ao.agent.name", | ||
| "gen_ai.agent.id": "splunk_ao.agent.id", | ||
| "gen_ai.agent.description": "splunk_ao.agent.description", | ||
| "gen_ai.agent.version": "splunk_ao.agent.version", | ||
| "gen_ai.provider.name": "splunk_ao.provider.name", |
There was a problem hiding this comment.
🟡 minor (design): Both gen_ai.system and gen_ai.provider.name map to splunk_ao.provider.name. Which one wins when both are present depends solely on their insertion order in this dict literal (provider.name currently later, so it wins — as asserted by test_normalizer_gen_ai_wins_collisions). This is fragile: reordering the map silently flips the precedence. Consider making the precedence explicit (e.g. handle provider resolution in a dedicated step, or add a comment documenting the order dependency).
🤖 Generated by the Astra agent
Summary
Add a single canonical attribute mapping and normalization pipeline for telemetry exported through the Splunk Agent Observability SDK. SDK-native spans and spans from user-configured OpenTelemetry or OpenInference instrumentation now produce a consistent OTLP wire contract for both standalone and O11y deployments without mutating completed spans.
The exported attributes retain standard
gen_ai.*fields, add the correspondingsplunk_ao.*aliases, move input/output and other content fields to the Splunk AO namespace, and identify the source assplunk_ao.system=splunk_ao_python.Why
The OTLP export foundation preserved upstream span attributes but did not yet apply the canonical Splunk AO attribute contract. Centralizing this behavior avoids divergent mappings across instrumentation types and provides one reusable builder for the upcoming handler-based JSON-to-OTLP conversion path.
For DTB, normalization runs on an immutable copy of each completed span after
BatchSpanProcessorforms an export batch and immediately before OTLP serialization. This change does not add a second batching implementation.What changed
gen_ai.*tosplunk_ao.*mapping used consistently at the export boundary.gen_ai.*attributes while duplicating their mapped values undersplunk_ao.*.splunk_ao.*namespace so content is not duplicated.splunk_ao.system=splunk_ao_pythonsource marker to exported spans.SPLUNK_AO_DEV_ENABLE_ATTRIBUTE_NORMALIZATION=false, for comparing baselinegen_ai.*output with the normalized wire shape. Routing, authentication, batching, and export remain active when it is disabled.Testing
Root SDK suite: 1,965 passed, 10 skipped; bundled A2A suite: 68 passed; focused mapping/exporter/native suite: 95 passed; changed-source Ruff/format and mypy, A2A Ruff/format/mypy, and
git diff --checkpassed.Compatibility and scope
splunk_ao.*namespace.