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. |
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: approve — Core semantics (retrieval CLIENT kind, data-source-id naming/attribute, opt-in remote-agent CLIENT) are correctly implemented and well tested; prior review rounds were addressed, and remaining items are minor consistency/observability nits plus an unresolved divergence from the ticket text.
General Comments
- 🟡 minor (question): HYBIM-961's acceptance criteria still reads: 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}and only then to bareretrieval(span_converter.py:47-51, asserted bytest_retriever_display_name_is_used_only_as_span_name_fallback).
I think the implemented behaviour is the right call — collapsing every un-tagged retriever to a single retrieval name would be a visible regression in the UI — and the PR body justifies it. But the ticket was last updated 2026-08-06 and still carries the contradicting text, so the acceptance criteria and the code disagree. Please get HYBIM-961 amended before merge so the next person reconciling the two doesn't "fix" the fallback away. Purely a docs/ticket action; no code change requested.
- 🔵 nit (other): A few hunks are unrelated reformatting rather than part of this change:
logger.py:368-372(rewrapping the existingSplunkAOLoggerExceptionraise) and three test bodies intests/test_attribute_mapping.py:570-640(collapsingControlSpan(...)calls onto one line). They look like a formatter pass over untouched code. Harmless, but they makegit blameon the control-span work point at this PR — worth splitting out if it's easy.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/schema/logged.py:86-102:LoggedRetrieverSpanis the only Logged* model declaringmodel_config = ConfigDict(from_attributes=True); its siblings (LoggedAgentSpan,LoggedWorkflowSpan,LoggedLlmSpan,LoggedControlSpan) do not. That means passing a coreAgentSpan/LlmSpaninstance intoLoggedTrace(spans=[...])does not widen the way a coreRetrieverSpaninstance now does — an inconsistency in the union's accepted input shapes. Worth either extendingfrom_attributes=Trueto the whole family (with a parity test asserting every union member widens from a core instance) or documenting that only the retriever path supports instance widening.src/splunk_ao/decorator.py:323-327:explicit_span_paramsis assembled without reference tospan_type, so mismatched combinations are accepted then silently discarded downstream:@log(span_type="llm", data_source_id="x")loses the ID during signature filtering in_complete_call(line 1128), and@log(span_kind=SpanKind.CLIENT)with nospan_typenever reaches thespan_type == "agent"branch in_prepare_call. This matches how the other auto-mapped span params already behave, but a debug log when a hint is supplied for a span type that cannot consume it would save a confusing round of "why is my attribute missing".src/splunk_ao/converter/attribute_mapping.py:30-65:test_every_alias_uses_an_explicit_destination_namespacechecks namespacing but not coverage, so there is no test asserting whichgen_ai.*keysbuild_span_attributesmay emit without asplunk_ao.*mirror. Now thatgen_ai.data_source.idis a deliberate standard-only attribute, an explicit allowlist test ("every emittedgen_ai.*key is either in the alias map or in this documented standard-only set") would make future additions a conscious decision rather than a silent omission, and would document the normalization layer's deprecation path.
| detail = getattr(span, field_name, None) | ||
| 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 reads data_source_id with a bare getattr, but set_retriever_attributes reads the same field via _field() (attribute_mapping.py:426), which falls back to model_extra. For any span carrying data_source_id only as a model extra — or a duck-typed source object like the SimpleNamespace(**fields, model_extra={...}) pattern already exercised in test_control_mapping_accepts_schema_compatible_control_span — the attribute gen_ai.data_source.id is emitted while the span name silently falls back to retrieval {display_name}. Name and identity then disagree for the same span, which is exactly the coupling this PR is trying to establish.
Not reachable through add_retriever_span today (it always constructs a real LoggedRetrieverSpan), so minor. Using the same accessor in both places removes the divergence by construction. Note this also means the returned value is no longer guaranteed to be a normalized string, so keep the .strip()/str() handling on it.
| detail = getattr(span, field_name, None) | |
| 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) | |
| def _span_name(span: BaseStep, step_type: StepType) -> str: | |
| prefix, field_name = _NAME_PARTS[step_type] | |
| detail = getattr(span, field_name, None) | |
| if step_type is StepType.retriever: | |
| detail = _field(span, field_name) | |
| if detail is None: | |
| detail = getattr(span, "name", None) | |
| return " ".join(part for part in (prefix, str(detail).strip() if detail is not None else "") if part) |
🤖 Generated by the Astra agent
| "retriever": [*common_params, "data_source_id"], | ||
| "tool": [*common_params, "tool_call_id"], | ||
| "workflow": common_params, | ||
| "agent": [*common_params, "agent_type"], | ||
| "agent": [*common_params, "agent_type", "span_kind"], |
There was a problem hiding this comment.
🟡 minor (bug): Adding data_source_id and span_kind to the auto-mapped lists means these are now populated from any decorated function parameter with a matching name (_prepare_input copies input_[param_name] into span_params at line 653-654), not just from the new decorator kwargs.
Two consequences worth weighing:
data_source_id: a coincidentally-named parameter of the wrong type — e.g.@log(span_type="retriever") def search(q: str, data_source_id: UUID)— makesLoggedRetrieverSpan(...)raiseValidationError(themode="before"validator passes non-str through untouched, and Pydantic won't coerceUUID/inttostr).add_retriever_spanis wrapped in@warn_catch_exception(exceptions=(Exception,)), so it returnsNoneand the entire span is dropped with only a warning. Previouslyretrieverauto-mapped onlycommon_params, so a same-named argument was simply ignored.span_kind: a user parameter namedspan_kindholding anything other than the literalSpanKind.CLIENTenum member is silently coerced toINTERNAL(see the validator thread).
This mirrors how model/temperature/agent_type already behave, so it's consistent rather than novel — but those are descriptive metadata, whereas these two drive OTel span identity and kind. Either coerce defensively in the validator (str(value) for non-None non-str), or restrict these two hints to explicit_span_params only and drop them from _get_span_param_names, so a name collision can't degrade or destroy a span.
🤖 Generated by the Astra agent
| self.add_child_span_to_parent(span) | ||
| span._parent = parent |
There was a problem hiding this comment.
🔵 nit (design): add_child_span_to_parent(span) runs before span._parent = parent, which is the inverse of both sibling helpers — _add_completed_leaf (line 650-651) and _attach_parentable_span (line 1874-1875) set _parent first. This preserves the old super().add_retriever_span(...) ordering so it isn't a behaviour change, but now that the construction is inlined here it's worth matching the local convention: any future logic in add_child_span_to_parent that reads span._parent would see it unset only on this one path.
| self.add_child_span_to_parent(span) | |
| span._parent = parent | |
| span._parent = parent | |
| self.add_child_span_to_parent(span) |
🤖 Generated by the Astra agent
|
|
||
| @pytest.mark.parametrize( | ||
| ("requested_kind", "expected_kind"), [(SpanKind.CLIENT, SpanKind.CLIENT), (SpanKind.SERVER, SpanKind.INTERNAL)] |
There was a problem hiding this comment.
🟡 minor (testing): This parametrization covers SpanKind.CLIENT and SpanKind.SERVER, i.e. only well-typed enum inputs. The path most likely to bite a user is the decorator auto-map one, where span_kind arrives as whatever the caller's function argument held — a string "CLIENT" or the int 2. test_agent_kind_allows_only_explicit_client_override in test_span_converter.py covers "CLIENT" at the model level, but nothing covers it end-to-end through @log(span_type="agent"), which is where the silent downgrade actually reaches users. Adding ("CLIENT", SpanKind.INTERNAL) and (2, SpanKind.INTERNAL) here would pin the intended contract at the egress boundary.
🤖 Generated by the Astra agent
Review verification summaryI verified all findings in the Astra review against the branch. All substantive findings hold up. Full suite: 2067 passed, 8 skipped; PR files clean under Confirmed by reproduction:
One pushback: the "unrelated reformatting" nit misdiagnoses cause. Main is genuinely unformatted at those lines under |
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.