feat: record telemetry span outcomes - #402
codeforester wants to merge 2 commits into
Conversation
| } | ||
| for key, value in attributes.items(): | ||
| _safe_span_call(session.span, "set_attribute", key, value) | ||
| if exception is not None: |
There was a problem hiding this comment.
Correctness (contradicts this PR's own stated goal): finish_telemetry calls record_exception() whenever exception is not None, without checking whether the outcome actually represents a failure — so successful invocations that exit via SystemExit(0)/click.exceptions.Exit(0) (an already-tested, legitimate early-success pattern; see tests/test_app_run_metadata.py lines 245-246) still get an exception recorded on their span. In _app_core.py's wrapper, except BaseException as exc: exception = exc catches this and still passes it to finish_telemetry(..., exception=exception) in the finally block, even though outcome.status == "ok". The span ends up simultaneously "OK" and "has a recorded exception" — misleading telemetry for a perfectly successful run. Suggest gating record_exception on outcome.status != "ok", matching the precedent in _record_lifecycle_diagnostic which already branches on outcome.kind before treating something as an error.
| state.attached_completion = True | ||
|
|
||
| def record_exception(self, exc: BaseException) -> None: | ||
| self.exception = exc |
There was a problem hiding this comment.
Correctness: same defect as the _app_core.py/integrations.py path, reproduced in the attached-lifecycle path. record_exception unconditionally sets self.exception = exc for every ctx.exit(code) call, including code=0 — so a normal, successful early exit from an attached Click tree also gets a fabricated exception recorded on its telemetry span once self.exception reaches finish_telemetry (see the exception=self.exception call at line 244 in _finalize).
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| result = invoke(app, [], home=Path(tmpdir)) | ||
|
|
There was a problem hiding this comment.
Test gap: this new test only exercises the failure path (asserts one recorded exception on a raised RuntimeError) and never asserts that a successful invocation leaves tracer.span.exceptions empty. So this PR's own test suite can't catch the unconditional-record_exception bug flagged on integrations.py/_attach.py — the exact regression this PR introduces for ctx.exit(0)/SystemExit(0) exits would pass this test unchanged, since nothing here asserts zero exceptions on a success run.
This branch has not been deployed
1 participant
Fixes #392
Summary
Validation
UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev --extra telemetry pytest -q tests/test_integrations.pyHosted checks are expected to run on this branch.