GenAI Conversation Root#116
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: needs_discussion — Well-tested change, but the shared gen_ai.conversation_root bridge is emitted as a string on the native path and a boolean on the OTel path, which the downstream span converter may not treat uniformly.
General Comments
-
🟠 major (design): This PR introduces an interim
gen_ai.conversation_rootmetadata bridge that is meant to be consumed by the downstream Galileo span converter (epic HYBIM-714). However, the value type of that key differs between the two ingestion paths: -
Native path (
logger.py):span.user_metadata["gen_ai.conversation_root"] = "true"— a string. -
OTel path (
otel.py):span.set_attribute(GEN_AI_CONVERSATION_ROOT, value=True)— a boolean.
If the converter keys off gen_ai.conversation_root and does an exact-value or type-sensitive comparison (e.g. == True vs == "true"), one of the two paths will be mishandled. The tests bake in this divergence (test_logger_batch.py asserts the string "true", test_otel.py asserts boolean True), so it would not be caught. Please confirm the converter normalizes both, or align the two paths on a single representation.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
src/splunk_ao/otel.py:374-374:span.set_attribute(GEN_AI_CONVERSATION_ROOT, value=True)passesvalueas a keyword while every otherset_attributecall in this module uses positional args. This forced the test helper to special-casekwargs["value"](test_otel.py lines 442-443, 476-477). Consider using the positional formspan.set_attribute(GEN_AI_CONVERSATION_ROOT, True)for consistency and to simplify the tests.
| ) | ||
| if isinstance(self.current_parent(), LoggedTrace): | ||
| span.conversation_root = True | ||
| span.user_metadata = {**(span.user_metadata or {}), "gen_ai.conversation_root": "true"} |
There was a problem hiding this comment.
🟡 minor (design): Two concerns with this bridge write:
- It unconditionally overwrites any user-supplied
gen_ai.conversation_rootmetadata key. Unlikely, but a caller passingmetadata={"gen_ai.conversation_root": "..."}would be silently clobbered. - This exact 2-line block is duplicated verbatim in
add_agent_span(lines 1938-1940). Consider extracting a small helper (e.g._mark_conversation_root(span)) so the field name, value type, and theisinstance(current_parent(), LoggedTrace)rule live in one place — this also reduces the risk of the two paths drifting apart later.
| span.user_metadata = {**(span.user_metadata or {}), "gen_ai.conversation_root": "true"} | |
| if isinstance(self.current_parent(), LoggedTrace): | |
| self._mark_conversation_root(span) |
🤖 Generated by the Astra agent
There was a problem hiding this comment.
Updated the native bridge behavior so a caller-supplied gen_ai.conversation_root metadata value is preserved. The SDK still sets the canonical conversation_root=True field based on topology; the metadata bridge now supplies "true" only when the caller did not provide a value.
There was a problem hiding this comment.
@fercor-cisco The differing types are intentional and route-specific: OTel uses the semantic-convention attribute as a boolean, while native user_metadata is a string-only interim bridge.
I don't think we should convert the OTel attribute to a string, I was under the impression that the user_metadata is a temporary measure until the backend supports the native OTel attribute. Thoughts?
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 — Correct, well-scoped, and well-tested conversation-root marking across native and OTel paths; only minor maintainability/consistency notes remain.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
src/splunk_ao/logger/logger.py:1852-1854: The conversation-root marking block (conversation_root = True+ thegen_ai.conversation_rootmetadata bridge) is duplicated verbatim inadd_workflow_span(1852-1854) andadd_agent_span(1938-1940). Extract a small helper such as_mark_conversation_root(span)so the field name, the string value, and theisinstance(current_parent(), LoggedTrace)rule live in one place and the two paths can't drift apart. Non-blocking maintainability cleanup.src/splunk_ao/otel.py:366-374: Consider a brief comment noting the deliberate type divergence: the OTel path emitsgen_ai.conversation_rootas a boolean (True), while the native logger'suser_metadatabridge emits the string"true". This is intentional (interim bridge vs. semantic-convention attribute) but non-obvious to a future reader.
fercor-cisco
left a comment
There was a problem hiding this comment.
Please see the suggested follow-ups.
Mark invocation-level conversation roots across native and OTel ingestion paths.