Repository navigation
feat(telemetry): each captured payload stored in the background is one operation (#733) - #985
Merged
Merged
Conversation
…e operation (#733, slice 4) The resident capture worker and the spool drainer run outside any MCP call, so every sample they recorded had operation_id null: the remember sample of a capture, its capture_processed and the capture_skipped of its failure were joined only by op and ts. telemetry.begin_operation() starts a fresh operation in the current context, which lasts until the next call or the end of the context. The worker's callback (capture_store.remember) and the drainer (capture_drain.drain_pending) call it once per payload, before the store. The capture_skipped the server reports from the same task when the store raises, and the one the drainer reports when it retires a file, therefore carry the id of the payload they are about. The id is born where the payload is stored. It does not name the hook invocation that produced the payload: the capture payload has a fixed key set and no version, and a resident worker started before an upgrade keeps running the old code, so it would refuse a payload carrying a new key. That hand-off needs a decision on the frame and is left out; the audit says so. Tests drive the resident server's own consume loop and the real drainer, one of them through the real remember handler on SQLite. Mutation gate run locally on the changed functions with the CI plan and test selection: no survivor on a changed line. Not verified here: Windows (the drainer's platform), pyright, the full suite (torch cannot be installed in this container). Refs #733 Co-Authored-By: Claude Fable 5.2 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYrKg46KKYpvLPXcW4M Signed-off-by: Claude <[email protected]>
…le timeout (#733) The three worker tests fed the server's bounded admission queue from a thread and relied on the consume loop meeting the next payload within a 0.5 s idle timeout: on a slow runner the loop could stop first and the test fail. The first payload is now queued before the loop starts, and the callback queues the next one once a payload has been processed (the queue is empty then) or stops the server after the last. put_nowait fails, it cannot block, and the idle timeout is never waited for. The callback still awaits capture_store.remember in the server's own task, so the tests fail as before when either begin_operation() call is removed. Refs #733 Co-Authored-By: Claude Fable 5.2 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYrKg46KKYpvLPXcW4M Signed-off-by: Claude <[email protected]>
CHANGELOG.md conflict: both entries are kept, main's first. Co-Authored-By: Claude Fable 5.2 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYrKg46KKYpvLPXcW4M Signed-off-by: Claude <[email protected]>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @cdeust's task in 3m 31s —— View job PR #985 review (1 of 3)
I found no blockers, and the review lists three one-line notes. I could not run pytest or ruff in this job because shell commands were not approved. The Pipeline CI was still running when the review was posted. Verdict: approve |
Review 1 of 3: PR #985 (operation identity for background captures, #733 slice 4)I found no blockers. I ran three reviews (code quality, test coverage, documentation accuracy) and traced the context-variable paths myself:
I could not run pytest or ruff in this job (no approval for shell commands). The PR's Pipeline CI was still running when I posted this. Notes, no action before merge
View job run |
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.
Refs #733 (slice 4; the issue stays open, see "Still open").
What
The part of "operation identity across background processing" that needs no change to the capture frame.
telemetry.begin_operation()inmcp_server/core/telemetry.py: starts a fresh operation in the current context, lasting until the next call or the end of the context.capture_store.remember) and the spool drainer (capture_drain.drain_pending) call it once per payload, before the store.remembersample of a capture, itscapture_processed, and thecapture_skippedof a failed or refused payload share oneoperation_id; it wasnull. Two payloads get distinct ids.Proof
tests_py/hooks/test_capture_operation_identity_733.py(6 tests) and 3 tests added totests_py/core/test_operation_identity_733.py. With eitherbegin_operation()call removed, 3 of the 6 fail (the worker tests for one, the drainer tests for the other).The worker tests use no feeder thread and no idle-timeout margin: the first payload is queued before the consume loop starts, and the test callback queues the next one from the server's own task.
Checks
ruff check,ruff format --checkon the changed files: clean.scripts/check_craftsmanship.py: OK.scripts/check_doc_claims.py: OK.begin_operationandcapture_store.rememberhave no survivor;capture_drain.drain_pendinghas none on the changed lines (7 survivors on lines this change does not touch are not judged, ADR-1122). No equivalent registered.main: the 733 tests, the backup tests and the handler selection pass (70 passed).Not verified
Still open on #733
capture_worker_policy.validate_payload) and no version, and a resident worker started before an upgrade keeps the old code: it would acknowledge a new payload and then refuse it. This needs a decision: version the frame, restart the worker on code change, or accept the loss.consolidate) carry no id.total_cost_usd.Generated by Claude Code