test(exact-sql): pin the split begin/lock spans - #713
Conversation
perf(observation) added an ObservationApplicationError variant and a HostAdmissionOutcome field without updating their consumers, and the hooks crate extraction left the observation benchmark reading fields it can no longer see. The pushed branch therefore does not compile, so every branch cut from it fails before its own work is built. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
perf(exact-sql): separate dispatch and lock timing (8bb12ad, already on this branch) already split the caller round trip (rusqlite.exact_sql.begin_immediate) from the worker's own SQLite lock acquisition (rusqlite.exact_sql.write_lock) and documented what each half measures. This adds the falsifiable proof that requirement asked for: a Hotpath-feature-gated test that drives a real begin_immediate() through a real writer thread, reads back the emitted JSON report, and asserts both span names are recorded with call counts >= 1 and are distinct labels. Never asserts on elapsed time, since these timings swing double digits run over run. Verified against current code: - rusqlite.exact_sql.begin_immediate (exact_sql/mod.rs) wraps the whole caller round trip: try_send + response.recv() blocking on the writer's reply channel, including the worker's own lock acquisition. - rusqlite.exact_sql.write_lock (exact_sql/command.rs, begin_transaction_with_busy_retry) measures only the worker-side busy retry loop that takes SQLite's write lock, on the worker thread, inside run_writer_command. - The bare rusqlite.begin_immediate name now appears exactly once, in writer/worker/mod.rs's run_incremental_vacuum -- an unrelated maintenance path that begins its own transaction directly on the already-dequeued worker thread (no channel wait folded in there). No writer scheduling, select_auxiliary_work, prefer_auxiliary, or drain_fair behavior touched. New test module is hotpath-feature-gated so the default build is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…plan-reopened' into HEAD
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bed9ac6006
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unsafe { | ||
| std::env::remove_var("HOTPATH_OUTPUT_FORMAT"); | ||
| std::env::remove_var("HOTPATH_OUTPUT_PATH"); |
There was a problem hiding this comment.
When feature-enabled tests run with an ambient HOTPATH_REPORT that excludes functions-timing (for example, a profiling invocation selecting only threads), clearing only the format and path leaves that selector active, so the JSON lacks functions_timing.data and this test panics even though both spans are correct. Clear or explicitly override the report selector as well, while restoring any prior environment afterward, to keep the test isolated.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
| #[cfg(feature = "hotpath")] | ||
| mod hotpath_spans; |
There was a problem hiding this comment.
In the inspected PR workflow, .github/workflows/ci.yml:224-225 runs workspace tests without hotpath, while lines 240-243 only run cargo check with that feature. Consequently this module is excluded from every PR test run, and a span rename or report-shape regression can merge green before first being exercised by the all-features release workflow; add this narrow test to a feature-enabled PR lane.
AGENTS.md reference: AGENTS.md:L126-L128
Useful? React with 👍 / 👎.
| /// Readiness counters the root-crate observation benchmark folds. The | ||
| /// struct moved into this crate, so its `pub(crate)` fields are no longer | ||
| /// reachable from there; these expose exactly the five it reads. | ||
| pub fn source_event(&self) -> &str { |
There was a problem hiding this comment.
These new pub getters are used only by baseline::validate_hook_telemetry_readiness, whose sole caller is the benchmark test module; production benchmark paths continue to serialize the aggregate without these methods. This creates a test-only production API even though the previous JSON assertions already checked the same fields, so retain serialization-based validation or expose access only when a real production contract needs it.
AGENTS.md reference: AGENTS.md:L82-L84
Useful? React with 👍 / 👎.
|
Closing after review. The feature-gated test is not exercised in PR CI, inherits ambient HOTPATH_REPORT state, and adds benchmark-only public accessors. The production span split already exists in PR 707. |
1 participant
Reopened against
codex/tracedecay-total-redesign-plan-reopened. The span split already existed inperf(exact-sql): separate dispatch and lock timing— my brief was stale. This adds only the missing falsifiable test provingrusqlite.exact_sql.begin_immediateandrusqlite.exact_sql.write_lockare recorded and distinct, driven through a real hotpath guard and asserted from parsed JSON, never on elapsed time.