Fix/HYBIM-962 review follow ups - #209
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: approve — All three fixes are correct and the ownership refactor is semantically equivalent to the code it replaces; only minor edge-case and test-strength issues remain.
General Comments
-
🟡 minor (design): I traced all three replaced call sites against the new
SplunkAOLogger._is_current_rootand the refactor is semantically equivalent, including the corner cases: -
decorator._conclude_owned_trace: old code returned early whencurrent_parent()wasNone(rootNoneis nota validated non-NoneTrace); new code returnsFalse. Same. -
base_handler._conclude_owned_trace:tracecan beNonehere, becausestart_traceis wrapped in@nop_sync/@warn_catch_exceptionand returnsNonewhen logging is disabled or the call raises. Old code would then hitNone is None→Trueand callconclude(conclude_all=True), which loops zero times becausecurrent_parent()is alsoNone— an effective no-op. New code short-circuits ontrace is None. Behaviour preserved, and the new version is clearer about intent. -
openai_agents._conclude_current_trace_on_failure: identical guard sequence collapsed into one call.
No objection to the refactor itself; see the inline note on logger.py about the three root-walks that were left un-centralized.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/exporter/diagnostics.py:234-243: ApartialSuccessblock whoserejectedSpanscannot be parsed (e.g.3.5,"invalid","3.0") falls through to the Splunk-shapedvalid/invalidcheck and is then classified as a full success. A backend that really did reject spans but encoded the count unexpectedly would be reported as healthy, with nothing in the logs to explain it. Consider emitting a debug/warning log whenpartialSuccessis present but the count is unusable, so the blind spot is diagnosable in the field.tests/test_decorator_operation_ownership.py:110-127:test_logger_identifies_current_root_ownershipunit-tests a logger internal from a decorator-focused module. Consider moving it next to the other logger parent-tracking tests (e.g. tests/test_logger_otel_context.py) so ownership-semantics coverage lives with the class that owns the behaviour.
| def _is_current_root(self, trace: Trace | None) -> bool: | ||
| """Return whether trace owns the current proprietary parent chain.""" | ||
| if trace is None: | ||
| return False | ||
|
|
||
| root = self.current_parent() | ||
| if root is None: | ||
| return False | ||
| while root._parent is not None: | ||
| root = root._parent | ||
| return root is trace |
There was a problem hiding this comment.
🟡 minor (design): The PR's stated goal is to centralize the current-root ownership check, but only the boolean form was centralized — three copies of the same parent-chain walk remain:
logger.py:432-435(reset_parent_tracking)decorator.py:759-762(resolving the trace root in_prepare_call)handlers/agent_control/bridge.py:248-250(_active_context)
Each of those needs the root object rather than a boolean, so they can't call _is_current_root as written. Extracting the walk once and layering the predicate on top would actually finish the centralization and leave a single place to fix if the chain representation changes:
def _current_root(self) -> StepWithChildSpans | None:
root = self.current_parent()
while root is not None and root._parent is not None:
root = root._parent
return root
def _is_current_root(self, trace: Trace | None) -> bool:
"""Return whether trace owns the current proprietary parent chain."""
return trace is not None and self._current_root() is traceSeparately: _is_current_root is underscore-private but is now called from handlers/base_handler.py, handlers/openai_agents/handler.py, and decorator.py. Since base_handler accepts a caller-supplied splunk_ao_logger, this private method is effectively part of the contract that a substituted logger must satisfy. Consider dropping the underscore, or noting in the docstring that handlers depend on it.
| def _is_current_root(self, trace: Trace | None) -> bool: | |
| """Return whether trace owns the current proprietary parent chain.""" | |
| if trace is None: | |
| return False | |
| root = self.current_parent() | |
| if root is None: | |
| return False | |
| while root._parent is not None: | |
| root = root._parent | |
| return root is trace | |
| def _current_root(self) -> StepWithChildSpans | None: | |
| """Return the root of the current proprietary parent chain.""" | |
| root = self.current_parent() | |
| while root is not None and root._parent is not None: | |
| root = root._parent | |
| return root | |
| def _is_current_root(self, trace: Trace | None) -> bool: | |
| """Return whether trace owns the current proprietary parent chain.""" | |
| return trace is not None and self._current_root() is trace |
🤖 Generated by the Astra agent
| if isinstance(source_parts, list) and not source_parts: | ||
| parts = [] | ||
| elif source_parts is not None: | ||
| parts = _content_parts(source_parts) |
There was a problem hiding this comment.
🟡 minor (bug): When a message carries an explicitly empty parts and a non-empty content, this now silently drops the content entirely.
Input {"role": "user", "parts": [], "content": "hello"} maps to {"role": "user", "parts": []} — "hello" is gone. The old code was also lossy here (it emitted the bogus {"type": "text", "content": "[]"} and dropped content too), so this isn't a regression, but the fix is the natural place to decide the precedence. Since the point of the change is to stop losing/garbling content, having parts: [] win over real content seems like the wrong tiebreak — an adapter that initializes parts=[] by default and puts the payload in content would produce a message with no content at all.
Suggest only honouring the empty list when there is nothing else to fall back on:
if isinstance(source_parts, list) and not source_parts and content in (None, ""):
parts = []
elif source_parts is not None:
parts = _content_parts(source_parts)If parts: [] is meant to be authoritative regardless of content, that's a defensible call — worth a short comment saying so, plus a test pinning the both-present case so the precedence isn't accidentally flipped later.
| if isinstance(source_parts, list) and not source_parts: | |
| parts = [] | |
| elif source_parts is not None: | |
| parts = _content_parts(source_parts) | |
| if isinstance(source_parts, list) and not source_parts and content in (None, ""): | |
| parts = [] | |
| elif source_parts is not None: | |
| parts = _content_parts(source_parts) |
🤖 Generated by the Astra agent
| if isinstance(value, float) and math.isfinite(value) and value.is_integer(): | ||
| parsed = int(value) | ||
| return parsed if parsed > 0 else None |
There was a problem hiding this comment.
🟡 minor (bug): Integral floats above 2^53 are accepted and converted to a misleading exact integer. _positive_json_integer(1e30) is finite and is_integer(), so it returns 1000000000000000019884624838656 and the operator sees Rejected spans: 1000000000000000019884624838656. — digits the payload never contained. The 512-char message cap keeps the log line bounded, but the number itself is fabricated by float→int conversion.
The protobuf path is naturally bounded by int64, so requiring exact float representability keeps the JSON path consistent with it:
if isinstance(value, float) and math.isfinite(value) and value.is_integer() and abs(value) <= 2**53:The new parametrized test is a good place to pin this — adding 1e30 to test_invalid_json_rejected_span_counts_are_ignored would lock in the behaviour either way.
| if isinstance(value, float) and math.isfinite(value) and value.is_integer(): | |
| parsed = int(value) | |
| return parsed if parsed > 0 else None | |
| if isinstance(value, float) and math.isfinite(value) and value.is_integer() and abs(value) <= 2**53: | |
| parsed = int(value) | |
| return parsed if parsed > 0 else None |
🤖 Generated by the Astra agent
| # When: the operation is awaited | ||
| with pytest.raises(RuntimeError, match="async application failure"): | ||
| await failing_operation() | ||
|
|
||
| # Then: the original exception is re-raised and both telemetry contexts are released | ||
| logger = splunk_ao_context.get_logger_instance() | ||
| assert logger.current_parent() is None | ||
| assert splunk_ao_context.get_current_trace() is None |
There was a problem hiding this comment.
🟡 minor (testing): The test name promises owned_trace_is_concluded, but the assertions only check that both contexts were released. It does catch a skipped _conclude_owned_trace (current_parent() would be non-None), so it isn't vacuous — but it passes regardless of whether the trace was concluded with the failure status, which is the part the async path is most likely to get wrong.
test_commit_failure_concludes_handler_owned_trace in tests/test_openai_agents.py:196 sets the bar here by asserting owned_traces[0].status_code == 500. Suggest matching it, e.g. capture the trace via logger.traces[-1] and assert status_code == 500, or assert the exported span in logger._sink.spans carries the error status — otherwise a regression that concludes the trace with a success status would slip through.
(The sync sibling at line 79 has the same gap; no need to fix it here, but it would be worth strengthening both together.)
🤖 Generated by the Astra agent
|
addressed comments, except this one
can be addressed in https://splunk.atlassian.net/browse/HYBIM-939 |
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 — The handler refactor changes behavior when the owned trace is None (previously concluded a dangling chain, now silently no-ops), and the explicit-empty-parts precedence still silently drops non-empty content.
General Comments
- 🟡 minor (design): Centralization is incomplete. The PR's stated goal is to "centralize current-root ownership checks shared by decorators, synchronous/asynchronous handlers, and OpenAI Agents failure cleanup." Three of the four copies of the parent-chain walk were replaced by
_current_root(), but a fourth remains inlogger.py:2101-2105(add_control_span):
root_parent = current_parent
while getattr(root_parent, "_parent", None) is not None:
root_parent = root_parent._parent
if root_parent is not None:
trace_id = getattr(root_parent, "id", None)This is semantically identical to _current_root() (it starts from self.current_parent() and walks to the root). Replacing it with root_parent = self._current_root() would finish the job and leave exactly one place to fix if the chain representation ever changes — which is the whole point of the refactor.
Note the prior review identified three remaining copies (logger.py:432, decorator.py:759, bridge.py:248); this fourth one in add_control_span was not in that list and is still outstanding.
- 🟡 minor (design):
_current_root/_is_current_rootare underscore-private but are now part of a cross-module contract.
Both are called from handlers/base_handler.py:108, handlers/openai_agents/handler.py:113, handlers/agent_control/bridge.py:248, and decorator.py:760/904. SplunkAOBaseHandler.__init__ and SplunkAOTracingProcessor.__init__ both accept a caller-supplied splunk_ao_logger, so any substituted logger must now implement these two private methods or the handlers will raise AttributeError on their failure-cleanup paths.
The existing tests happen to pass because they use Mock(spec=SplunkAOLogger) (which auto-specs private attributes) or real SplunkAOLogger instances — so this gap is invisible in CI. This was raised in the prior review and appears unaddressed.
Either drop the underscore on both (they are effectively public SPI now), or add a note to each docstring stating that handlers depend on them so future refactors don't treat them as freely-changeable internals.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/exporter/diagnostics.py:235-244: Tracked in HYBIM-939 per the author's PR comment: emit a debug/warning log whenpartialSuccessis present butrejectedSpansis unparseable (e.g.3.5,"invalid","3.0", or now1e30), so the response does not fall through to the Splunk-shaped valid/invalid check and get classified as a full success with nothing in the logs. Noting here only for cross-reference — no action needed in this PR.src/splunk_ao/handlers/base_handler.py:99-105: Theexcept Exception/finallycleanup incommit()(and the identical block inbase_async_handler.py:66-72) clears_nodesand_root_nodebut never resets the logger's parent chain. If the handler's trace bookkeeping is ever left inconsistent, the open chain leaks into the nextcommit()on the same context, whereadd_traceraisesValueError("You must conclude the existing trace before adding a new one.")— surfacing as a confusing error far from the real cause. Consider areset_parent_tracking()fallback infinallywhen the handler knows it started the trace, so a failed commit cannot poison subsequent ones.tests/test_agent_control_bridge.py:243-252:bridge.py:_active_contextwas changed in this PR to use_current_root()and now also rejectsroot_parent is None(previously thegetattr(root_parent, "id", None) is Nonecheck covered that case implicitly). There is no direct unit test for_active_contextreturningNonewhen the parent chain is empty or the root lacks anid— existing coverage exercises it only indirectly through_dispatch_trace_context. Worth adding focused cases for bothNonebranches.
| def _conclude_owned_trace(self, trace: Any, output: Any, status_code: int | None) -> None: | ||
| current_parent = self._splunk_ao_logger.current_parent() | ||
| root = current_parent | ||
| while root is not None and root._parent is not None: | ||
| root = root._parent | ||
|
|
||
| if root is trace: | ||
| if self._splunk_ao_logger._is_current_root(trace): | ||
| self._splunk_ao_logger.conclude(output=output, status_code=status_code, conclude_all=True) |
There was a problem hiding this comment.
🟠 major (bug): This refactor changes behavior when trace is None, turning a cleanup path into a silent no-op.
The old code compared the computed root against trace directly:
root = current_parent
while root is not None and root._parent is not None:
root = root._parent
if root is trace:
self._splunk_ao_logger.conclude(..., conclude_all=True)When trace is None and the parent chain was also empty, root is trace evaluated to None is None → True, and conclude(conclude_all=True) ran (harmlessly, since its while self.current_parent() is not None loop body never executes). More importantly, when trace is None and a parent chain did exist, root is trace was False — so the old code was already correct there.
The new _is_current_root short-circuits on trace is not None first, so trace=None now always returns False. The behavioral difference is narrow, but it interacts badly with how owned_trace is produced:
commit() (line 79) assigns owned_trace = self._splunk_ao_logger.start_trace(...). start_trace is decorated with @nop_sync and @warn_catch_exception() (logger.py:1300-1301), so it returns None when logging is disabled or when it raises internally. In the @warn_catch_exception case, add_trace may have already run self._set_current_parent(trace) before a later step threw — leaving a live parent chain with owned_trace = None.
The except Exception: block at line 100 guards with if owned_trace is not None, so it skips cleanup; and finally only clears self._nodes / self._root_node, never the logger's parent chain. Net effect: the chain stays open and leaks into the next commit() on the same context, where add_trace will raise ValueError("You must conclude the existing trace before adding a new one.").
This is reachable today, is not covered by a test (test_commit_failure_concludes_only_handler_owned_trace only exercises the owned_trace is not None path), and the same shape exists in base_async_handler.py:67.
Suggested fix — make the failure path unconditionally reclaim a chain the handler owns, rather than relying on a possibly-None handle:
except Exception:
if self._start_new_trace:
self._conclude_owned_trace(owned_trace or self._splunk_ao_logger._current_root(), output="", status_code=500)
_logger.warning("Failed to commit handler telemetry", exc_info=True)Alternatively, keep _is_current_root strict and have commit()/async_commit() verify that start_trace actually returned a trace before proceeding, bailing out early (and resetting parent tracking) if it did not. Either way, please add a regression test that forces start_trace to return None after mutating parent state.
🤖 Generated by the Astra agent
There was a problem hiding this comment.
Thanks for the detailed trace, but I don't think this one holds up — I tried to reproduce the described state and couldn't reach it. Verified against fbb8e15.
The premise is that start_trace can return None after add_trace already ran _set_current_parent(trace). I don't think that state is reachable.
1. @warn_catch_exception() here does not catch Exception. start_trace uses the bare form (logger.py:1300-1301), so exceptions defaults to INFRASTRUCTURE_EXCEPTIONS:
httpx.HTTPError, httpx.TimeoutException, httpx.ConnectError, httpx.ReadError,
httpx.WriteError, ConnectionError, TimeoutError, OSError
Note the contrast with siblings like add_single_llm_span_trace, which explicitly pass exceptions=(Exception,). So "returns None … when it raises internally" only covers infrastructure errors, not arbitrary ones.
2. No exception of any kind can escape add_trace after the parent is set. The only three statements following _set_current_parent(trace) are:
self._record_otel_ids(trace)
self._sync_otel_context(trace)
return trace_record_otel_ids (logger.py:473) and _sync_otel_context (logger.py:492) each wrap their whole body in except Exception: → log-and-continue. return trace can't throw. Everything that can throw — the ValueError guard, LoggedTrace(...) validation, traces.append — happens strictly before the assignment, and leaves no parent state behind.
3. The logging-disabled path leaves no chain. nop_sync skips the body entirely, so no parent is ever set:
start_trace() -> None | current_parent() -> None
4. add_trace's ValueError propagates rather than returning None — it isn't an infrastructure exception. Confirmed: with a chain already open, start_trace raises ValueError("You must conclude the existing trace before adding a new one.") and current_parent() still is the caller's trace.
So owned_trace is None implies current_parent() is None, and the if owned_trace is not None guard is skipping cleanup that would be a no-op anyway. This matches the conclusion your earlier review reached on this exact call site ("Behaviour preserved, and the new version is clearer about intent") — I don't see new evidence that overturns it.
Separately: the suggested fix would introduce a real bug. owned_trace or self._splunk_ao_logger._current_root() falls back to whatever root is current — which, when a caller owns the chain and start_new_trace=True, is the caller's trace. I simulated it:
fallback target is the CALLER's trace: True
caller status_code after fallback: 500
current_parent now: None
That stamps someone else's trace as a 500 and clears their parent chain — exactly the invariant test_commit_failure_preserves_caller_owned_trace (tests/test_base_handler.py:156) exists to protect.
Leaving as-is. Happy to reopen if you can show a concrete path where start_trace returns None with a non-None current_parent() — that's the specific step I couldn't construct.
| # An explicitly supplied parts field is authoritative over legacy content. | ||
| if isinstance(source_parts, list) and not source_parts: | ||
| parts = [] | ||
| elif source_parts is not None: | ||
| parts = _content_parts(source_parts) |
There was a problem hiding this comment.
🟠 major (bug): parts: [] winning over a non-empty content silently discards the content — and the new test now pins that as intended.
This was raised in the prior review. The response was to add the comment on line 175 and test_orchestration_treats_explicit_empty_parts_as_authoritative (tests/test_attribute_mapping.py:418), which asserts that {"role":"user","parts":[],"content":"ignored input"} maps to {"role": "user", "parts": []}. That is a legitimate way to resolve the ambiguity — declaring the precedence and pinning it beats leaving it accidental — but I want to push back on the direction chosen, because it conflicts with the PR's own stated goal.
The changelog entry says the fix is so that empty parts are "preserved in OTLP telemetry instead of being serialized as the text \"[]\"" — i.e. the motivation is stop garbling content. But for the both-present case the new code still loses data, just more quietly than before: the old code emitted a bogus {"type": "text", "content": "[]"} part (visibly wrong, so diagnosable in the field), whereas the new code emits an empty parts list (indistinguishable from a genuinely empty message). Silent loss is worse than loud garbage for field debugging.
The realistic failure mode is an adapter that initializes parts=[] as a default and puts the payload in content. Under this change every such message exports with no content at all, and nothing in the span indicates anything was dropped.
The narrower rule only honours the empty list when there is genuinely nothing to fall back on, which still fixes the "[]" bug without introducing a new loss path:
if isinstance(source_parts, list) and not source_parts and content in (None, ""):
parts = []
elif source_parts is not None:
parts = _content_parts(source_parts)Note this preserves the three new tests' intent except test_orchestration_treats_explicit_empty_parts_as_authoritative, which would need to flip to assert the content is retained.
If parts: [] really is meant to be authoritative regardless of content — e.g. because a known producer emits both and the content is stale — please say so explicitly in the comment ("producer X emits stale content alongside authoritative parts"), since "legacy" alone doesn't justify discarding a non-empty payload.
| # An explicitly supplied parts field is authoritative over legacy content. | |
| if isinstance(source_parts, list) and not source_parts: | |
| parts = [] | |
| elif source_parts is not None: | |
| parts = _content_parts(source_parts) | |
| # An explicitly supplied parts field is authoritative over legacy content. | |
| if isinstance(source_parts, list) and not source_parts and content in (None, ""): | |
| parts = [] | |
| elif source_parts is not None: | |
| parts = _content_parts(source_parts) |
🤖 Generated by the Astra agent
There was a problem hiding this comment.
The design question here is fair, and I'll answer it directly below. But the suggested patch can't be applied as written — it reintroduces the exact "[]" bug this PR fixes.
I applied the suggestion verbatim and ran the mapper:
{'role': 'user', 'parts': [], 'content': 'hello'} -> {'role': 'user', 'parts': [{'type': 'text', 'content': '[]'}]}
The reason is the elif: with content="hello", the new first branch is False, so control falls through to elif source_parts is not None → _content_parts([]). [] is falsy, so the Sequence branch is skipped and it lands on return [_text_part(value)] → {"type": "text", "content": "[]"}.
So for the both-present case the suggestion doesn't preserve content — it produces the literal string "[]" and still drops "hello". That's strictly worse than both the current code and the pre-PR behavior. To actually retain the content it needs a third branch routing to _content_parts(content), not a guard on the empty-parts branch.
For the record, applying it fails only test_orchestration_treats_explicit_empty_parts_as_authoritative (42 passed, 1 failed) — so you were right that that's the one test that flips. The problem isn't the blast radius, it's that the replacement output is wrong.
On the actual design question — why parts: [] wins:
parts is the canonical OTel field; content is the legacy compatibility shim. The precedence rule is "canonical field wins when explicitly present," not a claim about which payload is more likely to be real. test_orchestration_keeps_missing_parts_and_empty_content_distinct (tests/test_attribute_mapping.py:437) pins the related distinction: absent parts and empty parts are different signals. An explicit parts: [] is a producer saying "this message has no parts" — deliberately, since the key had to be written to appear at all. Absent parts falls back to content and always will.
I take the point about silent-vs-loud loss for field debugging. But I'd rather not special-case the canonical field's precedence on the contents of the legacy one — that makes the rule "parts is authoritative, except when content is non-empty, in which case parts: [] is reinterpreted as absent," which is harder to reason about and means a producer can't express "empty" at all once it also sets content.
I don't have a named producer that emits stale content alongside authoritative parts, so I'll soften the comment to state the rule rather than imply a known offender. If a real adapter shows up defaulting parts=[] with the payload in content, that's a concrete bug report and I'll revisit the tiebreak then.
Keeping current behavior. Open to the narrower rule if you want to argue for it, but it'd need a correct patch — the posted one regresses the bug the PR is fixing.
| def _is_current_root(self, trace: Trace | None) -> bool: | ||
| """Return whether trace owns the current proprietary parent chain.""" | ||
| return trace is not None and self._current_root() is trace |
There was a problem hiding this comment.
🟡 minor (design): The trace: Trace | None annotation is narrower than the actual call sites, which weakens the type checking this signature is supposed to provide.
base_handler.py:107declarestrace: Anyand forwards it straight through.openai_agents/handler.py:113passesself._owned_trace, which is declaredAny(line 61) and assigned fromadd_trace(...).
So mypy cannot actually verify that callers pass a Trace. Meanwhile _current_root() returns StepWithChildSpans | None — the broader type — and the identity comparison works for any object, so the narrow annotation buys nothing at runtime either.
Consider widening to match what _current_root returns, which makes the contract honest and would let base_handler/openai_agents tighten their own annotations from Any in a follow-up:
def _is_current_root(self, trace: StepWithChildSpans | None) -> bool:| def _is_current_root(self, trace: Trace | None) -> bool: | |
| """Return whether trace owns the current proprietary parent chain.""" | |
| return trace is not None and self._current_root() is trace | |
| def _is_current_root(self, trace: StepWithChildSpans | None) -> bool: | |
| """Return whether trace owns the current proprietary parent chain.""" | |
| return trace is not None and self._current_root() is trace |
🤖 Generated by the Astra agent
| def _conclude_current_trace_on_failure(self) -> None: | ||
| if self._owned_trace is None: | ||
| return | ||
|
|
||
| current_parent = self._splunk_ao_logger.current_parent() | ||
| if current_parent is None: | ||
| return | ||
|
|
||
| root = current_parent | ||
| while root._parent is not None: | ||
| root = root._parent | ||
| if root is self._owned_trace: | ||
| if self._splunk_ao_logger._is_current_root(self._owned_trace): | ||
| self._splunk_ao_logger.conclude(output="", status_code=500, conclude_all=True) |
There was a problem hiding this comment.
🟡 minor (bug): The old code had an explicit if self._owned_trace is None: return guard, so _conclude_current_trace_on_failure was a documented no-op when the processor did not own a trace. That guard is now implicit inside _is_current_root, which reads fine — but it means the only way this method reclaims a chain is via the self._owned_trace handle.
_owned_trace is assigned at line 139 from add_trace(...), which is reached inside _log_node_tree(root_node, first_node=True) — itself called from _commit_trace inside the try at line 87. If add_trace succeeds (setting the logger's current parent) but the assignment's enclosing statement is interrupted, or if a later add_workflow_span throws before _owned_trace is observed, _conclude_current_trace_on_failure runs with _owned_trace = None and does nothing — then finally (line 98) resets _owned_trace = None, discarding the last handle to an open chain.
This is the same shape as the base_handler issue and is likely low-frequency in practice, but since the whole purpose of this method is failure cleanup, it would be worth either asserting the invariant or falling back to _current_root() when _owned_trace is unexpectedly None.
🤖 Generated by the Astra agent
Two of the findings don't hold up — replies are on the individual threads (#1, #2) rather than repeated here. Short version: the The rest hold up:
One caveat: |
2 participants
Summary
Harden OTLP content mapping, acknowledgement diagnostics, and operation trace ownership for several edge cases found during review.
Follow ups from reviews
What changed
Testing