Fix/HYBIM-961 retrieval and agent span semantics - #210
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-5). It may contain mistakes.
Verdict: request_changes — Core mechanism is sound, but the new gen_ai.data_source.id attribute is missing from the splunk_ao alias map, whitespace-only IDs bypass the empty-ID normalizer, and the CHANGELOG contradicts the implemented naming fallback.
General Comments
- 🟡 minor (design): The two new runtime-only hints are modeled inconsistently.
span_kindlives onLoggedAgentSpanand is coerced by abeforevalidator that silently discards anything that isn't literallySpanKind.CLIENT;data_source_idlives onLoggedRetrieverSpanand is coerced only for the exact empty string. Both are then read back out by the converter viagetattr, so neither has a single place that owns "what does a valid value look like". Consider a small shared mixin (or at least a shared_OTEL_HINT_FIELDhelper) that both classes use, so the exclude-from-payload behaviour and the normalization rules are declared once. This would also make the asymmetry (why does one normalize whitespace-ish input and the other not?) visible at the definition site instead of at two separate validators. - 🟡 minor (question): HYBIM-961 states the retrieval span "is named
retrieval {gen_ai.data_source.id}when an authoritative data-source ID is supplied or simply retrieval otherwise". The implementation instead falls back toretrieval {display_name}when no ID is supplied. The PR description acknowledges and justifies this (preserving the captured display name avoids collapsing every un-tagged retriever to a singleretrievalname, which would be a visible regression in the UI), and I think the implemented behaviour is the better one. But it is a deliberate divergence from the accepted ticket text — please confirm with the ticket owner and update HYBIM-961 so the story and the code agree, otherwise the next person reconciling the two will "fix" the fallback away.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/schema/logged.py:86-97:LoggedRetrieverSpaninheritsspans: List[Span]from coreStepWithChildSpans, i.e. the core span union rather than the SDK-localLoggedSpanunion thatLoggedAgentSpanandLoggedWorkflowSpanboth override. If a child were ever attached to a retriever span,validate_assignmentwould coerce aLoggedLlmSpan/LoggedAgentSpanchild down to its core type, dropping widened multimodal content and the newspan_kindhint. This is not reachable today (SplunkAOLoggernever makes a retriever the current parent, soadd_child_span_to_parentcannot target one) and it predates this PR — the oldLoggedSpanunion embedded plainRetrieverSpanwith the same gap. Now that a dedicated SDK-local class exists, addingspans: list["LoggedSpan"] = Field(default_factory=list)would close it cheaply. Same applies to the still-coreToolSpanmember of theLoggedSpanunion.src/splunk_ao/decorator.py:323-327:explicit_span_paramsis assembled without reference tospan_type, so mismatched combinations are accepted and then silently discarded downstream:@log(span_type="llm", data_source_id="x")loses the ID during signature filtering in_complete_call, and@log(span_kind=SpanKind.CLIENT)(nospan_type, i.e. the workflow path) never reaches thespan_type == "agent"branch in_prepare_call. Neither is a bug introduced here — it matches howparamsand the other auto-mapped span params already behave — but a debug/warning log when a hint is supplied for a span type that cannot consume it would save users a confusing round of "why is my attribute missing".tests/test_logger_otel_egress.py:148-150:test_agent_logger_allows_only_client_kind_overridecallsotlp_logger.conclude(output="answer")twice on consecutive lines. This is correct and necessary — the first closes the agent span, the second closes the trace envelope — but it reads exactly like a copy-paste duplication and the retriever test immediately above needs only one call. A one-line comment (# conclude the agent span, then the trace envelope) would stop a future reader from "cleaning up" the second call and silently changing what the test covers.
| attrs["gen_ai.retrieval.documents"] = _content_value(span.output) | ||
| attrs["splunk_ao.retrieval.documents.count"] = len(span.output) | ||
| attrs["db.operation"] = "search" | ||
| _set_if_present(attrs, "gen_ai.data_source.id", _field(span, "data_source_id")) |
There was a problem hiding this comment.
🟡 minor (bug): gen_ai.data_source.id is emitted but never registered in SPLUNK_ALIAS_BY_GEN_AI, so normalize_attributes_for_export will not produce a splunk_ao.* mirror for it. Every other gen_ai.* attribute this function emits has an alias — including the two immediate neighbours here, gen_ai.retrieval.query.text and gen_ai.retrieval.top_k. The result is that with normalization enabled (SPLUNK_AO_DEV_ENABLE_ATTRIBUTE_NORMALIZATION), the new authoritative retrieval identity is the only retrieval field with no Splunk-namespaced counterpart, so any backend or dashboard consuming the splunk_ao.retrieval.* set silently sees no data source ID.
Impact is limited today because normalization is opt-in and off by default, which is why I'm rating this minor rather than major — but it will become a silent data gap the moment normalization is turned on. Add the alias next to the other retrieval entries. test_every_alias_uses_an_explicit_destination_namespace only checks namespacing, not coverage, so nothing currently catches this; consider a companion assertion that every gen_ai.* key produced by build_span_attributes appears in the alias map.
| _set_if_present(attrs, "gen_ai.data_source.id", _field(span, "data_source_id")) | |
| _set_if_present(attrs, "gen_ai.data_source.id", _field(span, "data_source_id")) | |
| # and in SPLUNK_ALIAS_BY_GEN_AI, alongside the other retrieval entries: | |
| # "gen_ai.data_source.id": "splunk_ao.data_source.id", |
🤖 Generated by the Astra agent
There was a problem hiding this comment.
gen_ai.data_source.id must remain standard only, the normalization behavior will be removed fully in future
| if step_type is StepType.retriever: | ||
| if detail is not None: | ||
| return f"{prefix} {detail}" | ||
| detail = getattr(span, "name", None) | ||
| return " ".join(part for part in (prefix, str(detail).strip() if detail is not None else "") if part) |
There was a problem hiding this comment.
🟡 minor (bug): The retriever branch returns f"{prefix} {detail}" without stripping, while the shared fallback path below it does str(detail).strip(). Combined with normalize_data_source_id, which only maps the exact empty string to None, a whitespace-only or padded ID leaks straight through:
data_source_id=" "→ span name"retrieval "(trailing whitespace) and attributegen_ai.data_source.id == " ".data_source_id=" kb "→ span name"retrieval kb ".
That is the exact class of malformed name test_missing_optional_name_parts_do_not_leave_whitespace exists to prevent — it just never exercises the new data_source_id path. Two options:
- Strip in the converter only (below). Cheap, but the attribute still carries the untrimmed value, so name and attribute disagree.
- Preferred: strip in
LoggedRetrieverSpan.normalize_data_source_idand treat the result as absent when empty, so name and attribute stay consistent and the empty-vs-whitespace cases collapse into one rule:
@field_validator("data_source_id", mode="before")
@classmethod
def normalize_data_source_id(cls, value: object) -> object:
if isinstance(value, str):
stripped = value.strip()
return stripped or None
return valueNote option 2 changes the intent of test_retriever_name_and_attribute_use_explicit_data_source_id_byte_for_byte; that test's case ("knowledge base/v1") still passes since it has no leading/trailing whitespace, but please add a whitespace-only case either way.
| if step_type is StepType.retriever: | |
| if detail is not None: | |
| return f"{prefix} {detail}" | |
| detail = getattr(span, "name", None) | |
| return " ".join(part for part in (prefix, str(detail).strip() if detail is not None else "") if part) | |
| if step_type is StepType.retriever: | |
| if detail is not None: | |
| return f"{prefix} {str(detail).strip()}" | |
| detail = getattr(span, "name", None) |
🤖 Generated by the Astra agent
There was a problem hiding this comment.
Agreed whitespace only IDs are now absent. Padded IDs are trimmed consistently before use in both the span name and attribute.
| @field_validator("span_kind", mode="before") | ||
| @classmethod | ||
| def normalize_span_kind(cls, value: object) -> SpanKind: | ||
| """Allow only the explicit remote-agent client classification.""" | ||
| return SpanKind.CLIENT if value is SpanKind.CLIENT else SpanKind.INTERNAL |
There was a problem hiding this comment.
🟡 minor (design): The identity comparison (value is SpanKind.CLIENT) in a mode="before" validator rejects representations that Pydantic would normally accept for an Enum field, and does so with no diagnostic. SpanKind is a plain Enum with int values, so all of these silently become INTERNAL:
add_agent_span(span_kind=2)@log(span_type="agent", span_kind="CLIENT")— plausible, since the decorator also auto-maps aspan_kindfunction argument intospan_params, where it arrives as whatever the caller passedSpanKind.SERVER(genuinely unsupported, but the caller gets no signal that their request was dropped)
The first two are the concerning ones: the user asked for a remote-agent classification, got a local one, and has nothing in the logs to explain why. Compare by value so equivalent representations are honoured, and warn on genuinely unsupported kinds so the drop is observable:
The existing parametrized test asserts "CLIENT" → INTERNAL, so it encodes the current behaviour; it would need updating alongside this. Please also confirm that silently normalizing (rather than raising) is the intended contract — the PR description says "safely normalizing", which suggests it is, but silence and normalization are separable choices.
| @field_validator("span_kind", mode="before") | |
| @classmethod | |
| def normalize_span_kind(cls, value: object) -> SpanKind: | |
| """Allow only the explicit remote-agent client classification.""" | |
| return SpanKind.CLIENT if value is SpanKind.CLIENT else SpanKind.INTERNAL | |
| @field_validator("span_kind", mode="before") | |
| @classmethod | |
| def normalize_span_kind(cls, value: object) -> SpanKind: | |
| """Allow only the explicit remote-agent client classification.""" | |
| try: | |
| resolved = value if isinstance(value, SpanKind) else SpanKind[str(value)] | |
| except (KeyError, ValueError): | |
| resolved = None | |
| if resolved is SpanKind.CLIENT: | |
| return SpanKind.CLIENT | |
| if value not in (None, SpanKind.INTERNAL): | |
| _logger.warning("Unsupported agent span_kind %r; using SpanKind.INTERNAL.", value) | |
| return SpanKind.INTERNAL |
🤖 Generated by the Astra agent
There was a problem hiding this comment.
agent span kind can only be client or internal, only an actual SpanKind.CLIENT enables remote-agent classification
| class LoggedRetrieverSpan(RetrieverSpan): | ||
| """RetrieverSpan with SDK-local OTel data-source identity.""" | ||
|
|
||
| model_config = ConfigDict(from_attributes=True, validate_assignment=True) |
There was a problem hiding this comment.
🟡 minor (question): Why does LoggedRetrieverSpan need model_config = ConfigDict(from_attributes=True, validate_assignment=True) when no other Logged* model declares one? validate_assignment=True is already inherited from BaseStep.model_config, so it is redundant. from_attributes=True is not exercised anywhere in this diff or in the surrounding code — nothing calls model_validate on a non-dict source for this type, and LoggedAgentSpan carries an analogous new field without it. If it is load-bearing for a path I've missed, please add a comment naming that path; otherwise drop the whole line so the model stays consistent with its siblings and doesn't quietly widen validation for arbitrary objects.
| model_config = ConfigDict(from_attributes=True, validate_assignment=True) | |
| data_source_id: str | None = Field(default=None, exclude=True) |
🤖 Generated by the Astra agent
There was a problem hiding this comment.
Partially agree - Removed redundant validate_assignment=True, retained required from_attributes=True, documented why it is necessary, and strengthened the compatibility assertion.
| - Retriever spans exported over OTLP now use client operation semantics and | ||
| names derived only from an explicit data-source ID. |
There was a problem hiding this comment.
🟡 minor (documentation): "names derived only from an explicit data-source ID" contradicts the implementation. _span_name falls back to retrieval {name} when no data_source_id is present, and further to bare retrieval when neither is available — that fallback is deliberate, tested (test_retriever_display_name_is_used_only_as_span_name_fallback), and described correctly in the PR body. As written, the CHANGELOG tells users their existing un-tagged retriever spans will lose their display name in the OTLP name, which is the opposite of what happens.
| - Retriever spans exported over OTLP now use client operation semantics and | |
| names derived only from an explicit data-source ID. | |
| - Retriever spans exported over OTLP now use client operation semantics and are | |
| named `retrieval {data_source_id}` when an explicit data-source ID is supplied, | |
| falling back to the captured display name and then to `retrieval`. |
🤖 Generated by the Astra agent
The shared mixin and child-union/decorator follow-ups are unnecessary for this PR. I added the useful comment explaining the intentional double conclude() call. |
2 participants
Summary
Align path-1 retrieval and agent spans with OpenTelemetry GenAI operation semantics.
What changed
Testing
Full SDK suite: 2,148 passed, 4 skipped.