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
| 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
|
@pradystar as we discussed during stand-up, please get a review from Sankar and other team members with more context about the OTel code. |
2 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.