feat(otel): add proprietary span converter#117
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: approve — Clean, well-tested stateless converter with no correctness/security issues; only minor observations about error-description mapping.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
tests/test_attribute_mapping.py:147-158: The fixture change frominput={"text": "question"}toinput='{"text":"question"}'makes the ControlSpan schema-valid (nativeControlSpan.inputisStepAllowedInputType, which excludes plain dicts). Correct fix, but note the test now passes a pre-serialized string that_content_valuereturns verbatim rather than a structured object it serializes — so it no longer really exercises 'structured content' JSON-encoding. Consider a separate case that feeds structured input through the serialization path if that behavior still needs coverage.
| def _span_status(span: BaseStep) -> Status: | ||
| exception = getattr(span, "exception", None) | ||
| status_code = getattr(span, "status_code", None) | ||
| if exception is None and (status_code is None or status_code < 400): | ||
| return Status(StatusCode.UNSET) | ||
| return Status(StatusCode.ERROR, str(exception) if exception is not None else "") |
There was a problem hiding this comment.
🟡 minor (design): _span_status reads getattr(span, "exception", None), but no Galileo span type defines an exception field — BaseStep and every subclass (LlmSpan, ToolSpan, etc.) only carry status_code. So for every real span this getattr returns None, meaning: (1) the ERROR status only ever fires via status_code >= 400, and (2) error spans always get an empty status description (Status(StatusCode.ERROR, "")) since the description would come from the non-existent exception.
Handlers do set status_code=400/500 on failures, so ERROR is set correctly — but the human-readable error message is lost. Is that intended? If the error text should propagate into the OTel status description, consider sourcing it from an existing field (e.g. the span output, which handlers populate with "Error: ..."). If the exception branch is truly speculative/future, a comment noting that would help, since as written it looks like a supported path but is dead for all current span types.
🤖 Generated by the Astra agent
There was a problem hiding this comment.
tests/test_span_converter.py:412-420 (line not in diff)
🟡 minor (testing): test_exception_sets_error_status_and_description verifies the ERROR-with-description path by defining ToolSpanWithException(ToolSpan) with an exception field that no real Galileo span type has. This test passes but exercises a type that never occurs in production, so the description-propagation behavior is not actually covered for any real span. Consider adding coverage for how error messages surface on real error spans (which today only have status_code), or align this test with the actual span contract.
🤖 Generated by the Astra agent
fercor-cisco
left a comment
There was a problem hiding this comment.
Please see the suggested follow-ups.
Summary
Add a stateless conversion layer that transforms completed proprietary SDK spans into OpenTelemetry
ReadableSpanobjects. This prepares handler-based integrations for the SDK's OTLP export pipeline without requiring those integrations to change how they create spans.The converter produces the same preliminary attribute contract used by SDK-native instrumentation while preserving the OpenTelemetry identity and routing Resource assigned when the proprietary span was created.
Why
Handler-based integrations currently produce Galileo span models rather than native OpenTelemetry spans. Moving their telemetry to OTLP requires a single conversion boundary that preserves existing instrumentation behavior, avoids duplicate attribute-mapping implementations, and produces standard completed spans that can be submitted to the SDK's existing batch processor.
Keeping conversion stateless and separate from logger lifecycle and export wiring makes the behavior independently testable and prevents trace buffering or a second batching implementation.
What changed
SpanConverterthat converts one completed Galileo span into one OpenTelemetryReadableSpan.SpanKind.CLIENTfor LLM calls andSpanKind.INTERNALfor the remaining supported operations.splunk_aoinstrumentation scope.gen_ai.*tosplunk_ao.*normalization at the common export boundary.DTB behavior
This change produces the completed
ReadableSpanobjects consumed by the existing DTBBatchSpanProcessor. It does not add batching, wait for an entire trace, force a flush, or export spans itself. Logger completion will submit these spans to the existing batch sink in the subsequent wiring change (HYBIM-890), allowing the standard batch processor to export completed spans on its configured schedule.Testing
Tests: 1,998 passed and 10 skipped; focused converter/export lifecycle suite: 84 passed; Ruff lint/format, converter mypy, Poetry lock validation, and
git diff --checkpassed.Compatibility and scope