feat(logger): add OTel context lifecycle for DTB#81
Conversation
fercor-cisco
left a comment
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 — Code is well-tested and the context lifecycle logic is sound, but the new hard OTel dependency footprint and the coupling of OTel bookkeeping to core span creation warrant discussion before merge.
General Comments
- 🟡 minor (design): This PR promotes
opentelemetry-sdk,opentelemetry-api, andopentelemetry-exporter-otlp-proto-httpfrom the optionalotelextra to required base dependencies, while the actual conversion/export that needs them is explicitly deferred to follow-up work. That means every consumer ofsplunk-ao(including those who only use the proprietary logger and never export to OTel) now pulls the SDK + OTLP HTTP exporter purely to record identity that isn't yet emitted anywhere. The exporter dependency in particular seems unnecessary until export lands. Can you confirm this footprint increase is intended for this foundational-only PR, and whether the exporter could stay optional until the export step is implemented? Theallextra also silently loses OTel packages — fine given they're now base deps, but worth calling out for anyone who filtered on[all]. - 🟡 minor (other): GitHub reports this PR as
mergeable_state: dirty(merge conflicts againstmain). It will need a rebase/merge before it can land — please resolve conflicts and re-verify the lockfile.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
src/splunk_ao/otel.py:30-71: Now that the OTel SDK/API/exporter are required dependencies, theOTEL_AVAILABLEflag, the stub fallback classes, and theINSTALL_ERR_MSGin otel.py are effectively dead defensive code (the import can no longer fail in a normally-installed environment). Consider simplifying or removing this fallback machinery in a follow-up to reduce confusion about whether OTel is optional.
| def _record_otel_ids( | ||
| self, step: BaseStep, parent_step: BaseStep | None = None, parent_span_context: SpanContext | None = None | ||
| ) -> OtelIds: | ||
| """Assign stable OTel identity using the step's actual Galileo parent.""" | ||
| if parent_step is not None: | ||
| parent_ids = self._otel_ids.get(parent_step.id) | ||
| if parent_ids is None: | ||
| raise RuntimeError(f"Missing OTel context for parent step {parent_step.id}.") | ||
| parent_span_context = parent_ids.span_context | ||
| elif parent_span_context is None: | ||
| active_context = otel_trace.get_current_span().get_span_context() | ||
| parent_span_context = active_context if active_context.is_valid else None | ||
|
|
||
| if parent_span_context is None: | ||
| otel_trace_id = _otel_id_generator.generate_trace_id() | ||
| trace_state = TraceState() | ||
| else: | ||
| otel_trace_id = parent_span_context.trace_id | ||
| trace_state = parent_span_context.trace_state | ||
|
|
||
| ids = OtelIds( | ||
| span_context=self._assign_otel_context(otel_trace_id, parent_span_context, trace_state), | ||
| parent_span_context=parent_span_context, | ||
| ) | ||
| self._otel_ids[step.id] = ids | ||
| return ids |
There was a problem hiding this comment.
🟡 minor (design): OTel identity/context bookkeeping is now invoked in the middle of core span-creation methods (_record_otel_ids at add_llm_span L1612, add_retriever_span, add_tool_span L1787, _attach_parentable_span L1798, add_control_span L2048) and in conclude. These methods are wrapped in warn_catch_exception(exceptions=(Exception,)), which swallows the exception and returns None. Consequences if any OTel step raises (e.g. the RuntimeError("Missing OTel context for parent step") on L430, or an error inside _sync_otel_context's attach/detach logic): the span is already attached to the proprietary parent tree via add_child_span_to_parent, but the method returns None to the caller (breaking span = logger.add_llm_span(...) usage) and, in distributed mode, skips the subsequent _ingest_step_streaming. So a purely bookkeeping failure for a not-yet-exported feature can silently drop/short-circuit core logging. Recommend isolating the OTel calls in their own try/except (log-and-continue) so identity assignment can never degrade proprietary span creation.
🤖 Generated by the Astra agent
There was a problem hiding this comment.
Fixed by containing OTel identity, context synchronization and cleanup failures so they warn without changing proprietary span returns or skipping distributed egress.
| def _assign_otel_context( | ||
| self, otel_trace_id: int, parent_span_context: SpanContext | None, trace_state: TraceState | ||
| ) -> SpanContext: | ||
| """Create a sampled local SpanContext without changing the active context.""" | ||
| return SpanContext( | ||
| trace_id=otel_trace_id, | ||
| span_id=_otel_id_generator.generate_span_id(), | ||
| is_remote=False, | ||
| trace_flags=TraceFlags(TraceFlags.SAMPLED), | ||
| trace_state=trace_state, | ||
| ) |
There was a problem hiding this comment.
🔵 nit (other): _assign_otel_context accepts parent_span_context but never uses it in the body (parentage is recorded separately in _record_otel_ids). Either drop the parameter or, if it's kept only to give the recording test hook (test_single_llm_trace_assigns_both_contexts_and_cleans_up) something to assert on, add a comment noting it is intentionally unused by the implementation.
| def _assign_otel_context( | |
| self, otel_trace_id: int, parent_span_context: SpanContext | None, trace_state: TraceState | |
| ) -> SpanContext: | |
| """Create a sampled local SpanContext without changing the active context.""" | |
| return SpanContext( | |
| trace_id=otel_trace_id, | |
| span_id=_otel_id_generator.generate_span_id(), | |
| is_remote=False, | |
| trace_flags=TraceFlags(TraceFlags.SAMPLED), | |
| trace_state=trace_state, | |
| ) | |
| def _assign_otel_context(self, otel_trace_id: int, trace_state: TraceState) -> SpanContext: | |
| """Create a sampled local SpanContext without changing the active context.""" | |
| return SpanContext( | |
| trace_id=otel_trace_id, | |
| span_id=_otel_id_generator.generate_span_id(), | |
| is_remote=False, | |
| trace_flags=TraceFlags(TraceFlags.SAMPLED), | |
| trace_state=trace_state, | |
| ) |
🤖 Generated by the Astra agent
There was a problem hiding this comment.
removed unused param
|
@pradystar as we discussed during stand-up, please get a review from Sankar and other team members with more context about the OTel code. |
savula15
left a comment
savula15
left a comment
There was a problem hiding this comment.
The design is sound and overall it looks good to me. added few minor comments. PTAL.
| ) | ||
| ) | ||
| self.traces.append(trace) | ||
| self._record_otel_ids(trace) |
There was a problem hiding this comment.
This path records ids for the trace + llm span, sets parent to None (so the sync attaches nothing), then _release_otel_context(trace). i.e. the OTel context is never made current for the single-span case. If the exporter reads the active OTel span rather than _otel_ids, this path would emit nothing. Can you confirm this matches the intended export model?
There was a problem hiding this comment.
Both steps are complete at creation, so neither intentionally becomes active. Active OTel context is only used for open parent propagation and conversion/export for galileo handlers consumes each step’s stored span_context and parent_span_context from _otel_ids. The logger egress path (HYBIM-890) will emit the LLM child and trace from those IDs before releasing them.
| # Ensure base_url ends with / for proper joining | ||
| if not base_url.endswith("/"): | ||
| base_url += "/" | ||
| endpoint: str = urljoin(base_url, "otel/traces") |
There was a problem hiding this comment.
Lets change this to "otel/v1/traces" instead of "otel/traces" since thats the endpoint that will be available on new Splunk AO instances and also aligns well with OTLP spec to have /v1/traces.
the HTTP exporter will be used by the immediately following logger egress changes (HYBIM-890). Regarding the follow-up, I removed the optional-OTel availability flag, fallback stubs and unavailable dependency tests now that OTel is a required base dependency. |
846bd77 to
7e2a8df
Compare
3 participants
Summary
Establish the OpenTelemetry context foundation required to convert and export handler-based telemetry through DTB.
Every handler-produced step now receives stable OTel identity and explicit parentage at creation time. Active OTel context follows only the genuinely open execution chain, preserving correct parent-child relationships for nested spans, completed leaves, distributed traces, and concurrent requests.
Why
DTB and DTA need stable trace and span identity before proprietary steps are converted or exported. Assigning that identity only at flush time would lose the active distributed parent and could make completed sibling leaves appear nested. This change records identity when each step is created while keeping activity aligned with the logger's real open lifecycle.
What changed
otelextra and update repository examples that referenced it.Testing
env GALILEO_HOME_DIR=/tmp/splunk-ao-test-home poetry run pytest -q— 1,742 passed, 101 skipped; PR context suite — 13 passed; focused logger/handler/decorator/OTel regressions — 277 passed, 1 skipped; changed-file Ruff, configured mypy, lockfile validation, wheel build, and wheel dependency verification passed.Repository-wide Ruff continues to report three pre-existing import-order findings in two untouched files.
Compatibility and risk
There are no public logger API changes. The base package now installs the OTel dependencies previously exposed through the
otelextra, increasing the default dependency footprint. Existing repository references tosplunk-ao[otel]have been updated tosplunk-ao.This PR establishes identity, parentage, and context lifecycle only. Conversion, batching/export, and replacement of the legacy distributed tracing API remain separate follow-up work.