Prove the error-span contract across the real failure paths #94

Merged

coilysiren merged 1 commit from otel-error-span-path-coverage into main

2026-08-12 11:22:24 +00:00

coilyco-ops commented

2026-08-12 11:01:29 +00:00

Member

Closes #71.

The finding

The issue's central code ask — every manual record_exception also marks its span as an error — was already satisfied when I picked this up. The catch-all baseline (d6e9d10, referenced by #74) centralized both operations into app.obs.record_error, so no call site can set one without the other. app/ contains no other record_exception call. The issue's evidence section describes app/resilience.py and app/upstream.py calling record_exception on manually-created spans without setting status; that is no longer how the code is shaped.

What was genuinely missing was the evidence. tests/test_obs.py exercised the helper in isolation, which cannot show whether the real dispatch paths route their failures through it — nor answer the criterion that actually constrains the design: "retried attempts remain individually visible without incorrectly marking a later successful request as failed."

What changed

New tests/test_error_spans.py, six tests over an in-memory span exporter:

non-streaming transport failure — every attempt span carries both the exception event and StatusCode.ERROR
validation failure — a structurally invalid response records response_validation_failed, not a silent reroll
retry-then-recover — the load-bearing one. The failed attempt keeps its error span; the attempt that recovered is left clean
backend fallback — the dead primary is an error; the backend that served the request is not
streaming connect failure — driven through the real upstream.chat_stream against a failing transport
redaction — a secret placed in both the prompt and the upstream error message reaches no exception message, status description, or span attribute

Recording turns out to be at attempt granularity, which is the right answer for #71's criterion: a transient blip stays visible in Error Management without turning a request that ultimately succeeded into a false alert. docs/proxy.md now states that contract.

Two things worth the reviewer's attention

The streaming test does not stub chat_stream. The record_error call lives inside that function, so stubbing it would delete the code under test and leave the assertion passing vacuously. My first draft did exactly that and I caught it — it now drives a failing transport through the real function.

My first draft of two tests failed, and the tests were wrong, not the code. A fake UpstreamResult with prompt_eval_count=1 tripped the delivered-context detector (#33), which correctly recorded context_truncated and marked an otherwise-clean span as an error. The _ok() helper now derives prompt_eval_count from the proxy's own token count of the messages actually sent. Worth knowing because any future test that fabricates an UpstreamResult will hit the same edge.

Verification

pytest — 264 passed
ruff / black / mypy — clean
Mutation-tested: removing the set_status call in app/obs.py kills 5 of the 6 new tests. The redaction test correctly survives, since it does not depend on status. Reverted after the check.

Human checkpoint — not claimed here

This issue is labelled interactive, and its last acceptance criterion is an Ops verification: one synthetic transport failure appearing under service.name=agent-proxy in SigNoz Error Management, linked to the expected trace. That needs a deployed build and a person looking at the tenant, so I have not marked it done. Everything else in the acceptance list is covered above.

Closes #71. ## The finding The issue's central code ask — every manual `record_exception` also marks its span as an error — **was already satisfied** when I picked this up. The catch-all baseline (`d6e9d10`, referenced by #74) centralized both operations into `app.obs.record_error`, so no call site can set one without the other. `app/` contains no other `record_exception` call. The issue's evidence section describes `app/resilience.py` and `app/upstream.py` calling `record_exception` on manually-created spans without setting status; that is no longer how the code is shaped. What was genuinely missing was the **evidence**. `tests/test_obs.py` exercised the helper in isolation, which cannot show whether the real dispatch paths route their failures through it — nor answer the criterion that actually constrains the design: *"retried attempts remain individually visible without incorrectly marking a later successful request as failed."* ## What changed New `tests/test_error_spans.py`, six tests over an in-memory span exporter: - **non-streaming transport failure** — every attempt span carries both the exception event and `StatusCode.ERROR` - **validation failure** — a structurally invalid response records `response_validation_failed`, not a silent reroll - **retry-then-recover** — the load-bearing one. The failed attempt keeps its error span; the attempt that recovered is left clean - **backend fallback** — the dead primary is an error; the backend that served the request is not - **streaming connect failure** — driven through the real `upstream.chat_stream` against a failing transport - **redaction** — a secret placed in both the prompt and the upstream error message reaches no exception message, status description, or span attribute Recording turns out to be at **attempt** granularity, which is the right answer for #71's criterion: a transient blip stays visible in Error Management without turning a request that ultimately succeeded into a false alert. `docs/proxy.md` now states that contract. ## Two things worth the reviewer's attention **The streaming test does not stub `chat_stream`.** The `record_error` call lives *inside* that function, so stubbing it would delete the code under test and leave the assertion passing vacuously. My first draft did exactly that and I caught it — it now drives a failing transport through the real function. **My first draft of two tests failed, and the tests were wrong, not the code.** A fake `UpstreamResult` with `prompt_eval_count=1` tripped the delivered-context detector (#33), which correctly recorded `context_truncated` and marked an otherwise-clean span as an error. The `_ok()` helper now derives `prompt_eval_count` from the proxy's own token count of the messages actually sent. Worth knowing because any future test that fabricates an `UpstreamResult` will hit the same edge. ## Verification - `pytest` — 264 passed - `ruff` / `black` / `mypy` — clean - **Mutation-tested**: removing the `set_status` call in `app/obs.py` kills 5 of the 6 new tests. The redaction test correctly survives, since it does not depend on status. Reverted after the check. ## Human checkpoint — not claimed here This issue is labelled `interactive`, and its last acceptance criterion is an Ops verification: one synthetic transport failure appearing under `service.name=agent-proxy` in SigNoz Error Management, linked to the expected trace. That needs a deployed build and a person looking at the tenant, so **I have not marked it done**. Everything else in the acceptance list is covered above.

coilyco-ops added 1 commit

2026-08-12 11:01:29 +00:00

Prove the error-span contract across the real failure paths

ci / smoke (pull_request) Successful in 6s

Details

ci / quality (pull_request) Successful in 21s

Details

6ae15aa196

#71 asked that every manual record_exception also mark its span as an error.
The catch-all baseline already centralized both into app.obs.record_error, so
no call site can set one without the other, and no other record_exception call
exists in app/. The code criterion was met; the evidence was not.

tests/test_obs.py only exercised the helper in isolation. That cannot show
whether the real dispatch paths route failures through it, nor - the criterion
that actually constrains the design - whether a failure that is later retried
successfully leaks an error status onto the request that survived.

Add tests/test_error_spans.py covering non-streaming transport failure,
validation failure, retry-then-recover, backend fallback, streaming connect
failure, and redaction. Recording is at attempt granularity: the failed attempt
and the dead backend each keep an error span while the attempt that recovers or
serves is left clean, so a transient blip does not become a false alert.

The streaming case drives upstream.chat_stream against a failing transport
rather than stubbing chat_stream, since record_error lives inside that function
and stubbing it would remove the code under test.

Verified by mutation: dropping the set_status call in app.obs kills 5 of the 6
new tests. The redaction test correctly survives, as it does not depend on
status.

Ops verification of one synthetic transport failure in SigNoz Error Management
is the remaining human checkpoint and is not claimed here.

closes #71

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-12 11:01:29 +00:00