Resolve orphan provider span parents structurally, wire 152/153 - #277
Open
chris-colinsky wants to merge 2 commits into
Open
Resolve orphan provider span parents structurally, wire 152/153#277chris-colinsky wants to merge 2 commits into
chris-colinsky wants to merge 2 commits into
Conversation
A call issued from branch or fan-out instance middleware resolved its parent against whatever dispatch spans happened to be registered when the observer drained the event. Dispatch spans are synthesized from inner NODE events, and a wrapper-issued call is enqueued before the wrapper's first inner node starts, so the answer came down to whether anything yielded to the event loop in between. One `await asyncio.sleep(0)` in user middleware moved the span from its branch to the invocation root. Observability section 10 covers parentage, so that was non-conforming rather than untidy. The parent is now resolved structurally: a call issued from a wrapper is inside that wrapper, so its nearest enclosing wrapper is that dispatch span whether or not the observer has materialized it. Any dispatch span the calling lineage sits inside is synthesized on demand, triggered by the first event that needs it rather than only by an inner node event. Ruled in the release coord thread; the section 6 sentence pinning the span's start time to the inner started event changes with it, and that wording is spec's to land. Two details. The synthesis walks the full calling namespace rather than its proper ancestors, because a call from branch middleware sits AT the parallel-branches namespace, so its branch's prefix is that namespace and the existing ancestor walk never reaches it. And a provider event carries the lineage chains but no subgraph identities, so a span it synthesizes would read an empty subgraph name where one synthesized from a node event reads the real value; the identity is backfilled from the first node event so that attribute cannot depend on which event arrived first either. Applies to both dispatch span kinds. They share one synthesis path with one trigger, so neither has an eager arm the other lacks.
Both assert where a provider span lands when it is issued from a wrapper rather than the node body, so the calling node's span is not open and the span falls back to the nearest enclosing wrapper. 152's is the per-branch dispatch span; 153 nests a fan-out between them so its is the innermost instance span. THE WRAPPER YIELDS ON PURPOSE. An earlier version of this driver made both fixtures pass, and that passing was worthless: it depended on nothing yielding to the event loop between the provider call and the next node start. One `await asyncio.sleep(0)` moved 152's orphan to the invocation root and 153's to the branch dispatch span its own invariant forbids. The yield is the assertion, and the observer change it exercises lands separately. The routing invariant now checks routing. It previously counted orphans per dispatch span, which a full branch swap satisfies exactly as well as correct routing, and the span_tree cannot see the difference because the two orphan spans are name- and attribute-identical in the declared tree. The mock routes each branch's distinct request content to a distinct response id, so the id on the span identifies which branch issued it, and the expected mapping is derived from the fixture rather than hardcoded. Dispatch spans are told from node spans by the attributes only a synthesized dispatch span carries. Matching on name or on branch_name misfires both ways: an instance dispatch span reuses its fan-out node's name, and every node span inside a branch carries branch_name, so the sibling and absence claims accepted the guard node span they forbid. Subgraph references are collected in one helper covering all three spellings. Missing the parallel-branches one compiled a host subgraph before its branch targets and died on a bare KeyError from the builder.
There was a problem hiding this comment.
Pull request overview
This PR makes OpenTelemetry span parent resolution deterministic for provider/tool spans emitted from wrapper middleware by synthesizing any enclosing dispatch spans on-demand (based on lineage structure rather than observer drain timing). It also activates and wires conformance fixtures 152 and 153 with a dedicated driver that asserts orphan-parenting invariants that cannot be expressed in the existing span_tree matcher.
Changes:
- Add structural (lineage-derived) dispatch-span synthesis on orphan-parent resolution so wrapper-issued provider/tool spans consistently parent under the correct enclosing dispatch span.
- Backfill
openarmature.subgraph.namefor already-synthesized parallel-branches dispatch spans once a node event supplies subgraph identities. - Wire and activate observability fixtures 152/153 with a new driver that validates routing, absence, count, and ordering invariants beyond
span_treeexpressiveness.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/openarmature/observability/otel/observer.py |
Adds on-demand dispatch span synthesis for wrapper-issued provider/tool events to remove schedule-dependent parentage; adds subgraph identity backfill for synthesized dispatch spans. |
tests/conformance/test_observability.py |
Activates fixtures 152/153 and introduces a driver + invariants to validate deterministic orphan-parent resolution under yielding middleware. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+7105
to
+7125
| def _handler(request: httpx.Request) -> httpx.Response: | ||
| # Routed by REQUEST CONTENT, not FIFO. Under concurrent branches the | ||
| # queue order is nondeterministic, so a FIFO mock cannot support the | ||
| # routing invariant: whichever branch called first would get the first | ||
| # id regardless of which one it was. | ||
| payload = cast("dict[str, Any]", json.loads(request.content.decode())) | ||
| sent = " ".join( | ||
| str(cast("dict[str, Any]", m).get("content", "")) | ||
| for m in cast("list[Any]", payload.get("messages") or []) | ||
| ) | ||
| for mock in mocks: | ||
| body = cast("dict[str, Any]", mock["body"]) | ||
| if any(_mock_matches_content(body, word) for word in sent.split()): | ||
| return httpx.Response( | ||
| 200, content=json.dumps(body).encode(), headers={"Content-Type": "application/json"} | ||
| ) | ||
| assert mocks, "mock_llm queue exhausted" | ||
| body = cast("dict[str, Any]", mocks[0]["body"]) | ||
| return httpx.Response( | ||
| 200, content=json.dumps(body).encode(), headers={"Content-Type": "application/json"} | ||
| ) |
Comment on lines
+7158
to
+7160
| subgraph_specs = _merged_subgraph_specs(case, spec) | ||
| instance_mw: dict[str, dict[str, list[Any]]] = {} | ||
| branch_mw: dict[str, dict[str, list[Any]]] = {} |
Comment on lines
+2951
to
+2954
| namespace = cast("tuple[str, ...]", event.namespace) | ||
| fan_out_index_chain = cast("tuple[int | None, ...]", event.fan_out_index_chain) | ||
| branch_name_chain = cast("tuple[str | None, ...]", event.branch_name_chain) | ||
| branch_name = cast("str | None", event.branch_name) |
Comment on lines
+2630
to
+2634
| # Already synthesized, possibly from a provider event, which | ||
| # carries the lineage but not the subgraph identities. Fill | ||
| # the identity in from this node event so the attribute does | ||
| # not depend on which event triggered synthesis. | ||
| _backfill_subgraph_identity(open_branch, event, len(prefix)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An orphan provider span's parent was a function of thread scheduling. This makes it structural, and activates the two fixtures that pin it.
The defect
A call issued from branch or fan-out instance middleware resolved its parent against whatever dispatch spans happened to be registered when the observer drained the event. Dispatch spans are synthesized from inner node events, and a wrapper-issued call is enqueued before the wrapper's first inner node starts. So the answer came down to whether anything yielded to the event loop in between:
One
await asyncio.sleep(0)in user middleware moved the span from its branch to the invocation root. Yielding is ordinary for real middleware: any HTTP call, any lock.Observability §10 scopes determinism to the event stream and excludes only timestamps, span ids and trace ids. Parentage is structure derived from that stream, so this was non-conforming rather than untidy.
The fix
The parent is resolved structurally: a call issued from a wrapper is inside that wrapper, so its nearest enclosing wrapper is that dispatch span whether or not the observer has materialized it. Any dispatch span the calling lineage sits inside is synthesized on demand, triggered by the first event that needs it rather than only by an inner node event.
Two details worth review attention:
range(1, len(namespace))) never reaches it.openarmature.subgraph.namewhere one synthesized from a node event reads the real value — trading schedule-dependent parentage for a schedule-dependent attribute. The identity is backfilled from the first node event. Currently unobservable, since every dispatch span in the corpus has an empty identity, which is exactly why it would have gone unnoticed.Applies to both dispatch span kinds. They share one synthesis path with one trigger; neither has an eager arm the other lacks.
The fixtures
152 and 153 are activated, and their wrapper yields on purpose. An earlier version of this driver made both pass, and that passing was worthless — it depended on nothing yielding. The yield is the assertion.
The routing invariant now checks routing. It previously counted orphans per dispatch span, which a full branch swap satisfies exactly as well as correct routing, and the
span_treecannot see the difference because the two orphan spans are name- and attribute-identical in the declared tree. The mock now routes each branch's distinct request content to a distinct response id, and the expected mapping is derived from the fixture rather than hardcoded.Dispatch spans are distinguished from node spans by the attributes only a synthesized dispatch span carries. Matching on name or on
branch_namemisfires both ways: an instance dispatch span reuses its fan-out node's name, and every node span inside a branch carriesbranch_name, so the sibling and absence claims accepted theguardnode span they forbid.Verification
That last one is a predicate-level proof, not end-to-end: three attempts at a source-side swap all failed incidentally, either because
span_treecaught them first or because the shared lookup also mis-parented the guard nodes.Spec status
Built against the ruling in the release coord thread (msgs 33/35), which is direction, not contract. It changes §6's sentence pinning a dispatch span's start time to the inner started event, so it needs a proposal, and the binding wording arrives when that is accepted. Nothing here touches the pinned spec.
Corpus
One observability fixture now remains deferred for a real gap: 119, which needs YAML
middleware:translation in the adapter and is unrelated.Full suite: 2155 passed, 497 skipped.