HYBIM-887 feat(otel): normalize native and user-wired paths#110
HYBIM-887 feat(otel): normalize native and user-wired paths#110pradystar wants to merge 1 commit into
Conversation
|
will rebase and point to main once #106 is merged |
9538c19 to
2a43fbe
Compare
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: needs_discussion — Clean, well-tested refactor, but two intentional behavior changes (routing frozen at construction; session/dataset no longer on Resource) alter runtime semantics and the ingest contract, and the ticket has no spec to confirm intent.
General Comments
- 🟠 major (design): Routing is now captured once at
SplunkAOOTLPExporter.__init__time and frozen into the request headers and the copied-span Resource. Previouslyon_start()read_project_context/_log_stream_context/_experiment_id_contextper span andexport()refreshed the headers per batch, so a single processor could route different spans to different projects/log-streams/experiments based on the activesplunk_ao_context(...). Because the processor (and thus its exporter) is normally constructed once at application startup — before anysplunk_ao_contextis entered — dynamic per-request/per-context routing via the context manager is now silently ignored for the OTLP export path; every span goes to whatever was resolved at construction time (typically the default project). The PR body states routing is 'captured once per exporter', so this looks intentional, but HYBIM-887 has no description and this is a real semantic regression for multi-tenant callers that switch project/log-stream per request. Please confirm this tradeoff is acceptable and, if so, document it prominently (e.g. 'to route to a different project/log-stream/experiment, construct a separate processor/exporter'). - 🟠 major (design): The old
export()promotedsplunk_ao.session.idandsplunk_ao.dataset.input/output/metadatafrom span attributes into each span's Resource before serialization. The new implementation only puts routing keys (project/log-stream/experiment) into the Resource and leaves session/dataset content on span attributes (ROUTING_ATTRIBUTE_KEYSdeliberately excludes them). Semantically span-level is arguably more correct, but this is a silent change to the data contract the backend ingests. If Splunk AO ingestion keys session grouping or dataset/ground-truth scoring off Resource attributes, this breaks those features with no test coverage proving the backend accepts them at span level. Please confirm the ingestion side (PR #106/PR 3) reads session.id and dataset.* from span attributes rather than Resource.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
splunk-ao-a2a/src/splunk_ao_a2a/instrumentor.py:34-40: The class docstring example still importsSplunkAOSpanProcessorand callsadd_splunk_ao_span_processor(provider, SplunkAOSpanProcessor()). It still works, but the README/quick-start was simplified toadd_splunk_ao_span_processor(provider); update this docstring to match the new preferred form.
| events=span.events, | ||
| links=span.links, | ||
| kind=span.kind, | ||
| instrumentation_info=span.instrumentation_info, |
There was a problem hiding this comment.
🟡 minor (performance): span.instrumentation_info is a @deprecated property (typing_extensions) that emits a DeprecationWarning on every access. The SDK's filterwarnings suppression in get_tracer targets the message "You should use InstrumentationScope..." while this property's message is "You should use instrumentation_scope..." (different casing), so the warning is NOT suppressed — it fires once per exported span on the hot path. Since you already copy instrumentation_scope (which carries the same data), drop the deprecated argument to avoid the per-span warning and lookup.
| instrumentation_info=span.instrumentation_info, | |
| instrumentation_scope=span.instrumentation_scope, | |
| ) |
🤖 Generated by the Astra agent
| self._exporter = ( | ||
| _exporter | ||
| if _exporter is not None | ||
| else SplunkAOOTLPExporter( | ||
| project=project, | ||
| project_id=project_id, | ||
| logstream=logstream, | ||
| log_stream_id=log_stream_id, | ||
| experiment_id=experiment_id, | ||
| _exporter_factory=_exporter_factory, | ||
| **kwargs, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🟡 minor (bug): When a prebuilt _exporter is supplied, any **kwargs (e.g. timeout=...) are silently dropped rather than applied, because kwargs are only forwarded on the SplunkAOOTLPExporter(...) branch. A caller passing both _exporter=... and extra OTLP kwargs would get no error and no effect. Consider rejecting that combination explicitly, mirroring the guard you added in add_splunk_ao_span_processor.
🤖 Generated by the Astra agent
|
@pradystar see the comments above. Also I think this PR should be reviewed by Sankar. |
Summary
Make the SDK-native and caller-wired OpenTelemetry paths use the same deployment-aware standalone/O11y exporter configuration introduced by PR 3. Routing is captured once per exporter, represented in OTLP request headers and copied-span Resource attributes, and never written to routing span attributes or private OTel state.
What changed
SplunkAOOTLPExporterinto a deployment-aware wrapper around the OTLP HTTP exporter.ReadableSpanexport copies, remove stale routing keys from copied span/Resource attributes, and preserve every other source-span field.SplunkAOSpanProcessor; keep session/dataset content as span attributes and stop writing routing attributes inon_start().add_splunk_ao_span_processor(provider, **kwargs)construct, register, and return the default processor while retaining prebuilt-processor support.gen_ai.system="galileo-otel"injection while preserving upstreamgen_ai.*attributes and existing type-specific native attribute setters.Verification
1822 passed, 101 skipped.42 passed, 1 skipped.68 passed.113 source files, no issues.poetry check --lockandgit diff --checkpassed.