perf: batch performance-metric writes into one SQLite round-trip - #1341
Conversation
record_metric opens a connection, INSERTs one row and commits, per metric. The background monitor emits 7 metrics every 30s and the /performance/report endpoint replays a whole client batch through the same path, so a browser sending 50 samples cost 50 connect+commit cycles. Add record_metrics()/_store_metrics(), which take the lock once, extend the buffer and fast-access deques in one critical section, and persist the whole batch with a single connect -> executemany -> commit. record_metric and _store_metric are unchanged for the ~15 genuine single-metric callers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@linear-code @coderabbitai review Two judgement calls I'd like a second opinion on, both spelled out in the PR body:
Prove-fail is recorded in the body: 7 of the 9 new tests fail against the pre-change source; the 2 that pass are deliberate control tests. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git diff --stat origin/main...HEAD
git diff --name-only origin/main...HEAD
fd -a 'performance_monitor\.py|router\.py|test_performance_monitor\.py' . \
| sort \
| xargs -r -n1 ast-grep outline --items all --view expanded
echo '--- changed diff ---'
git diff --find-renames --unified=80 origin/main...HEAD -- \
src/youtube_extension/backend/services/performance_monitor.py \
src/youtube_extension/backend/api/v1/router.py \
tests/unit/test_performance_monitor.pyLength of output: 59637 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- monitor initialization and schema ---'
sed -n '80,190p' src/youtube_extension/backend/services/performance_monitor.py
echo '--- batching implementation with line numbers ---'
nl -ba src/youtube_extension/backend/services/performance_monitor.py | sed -n '220,390p'
echo '--- report endpoint with line numbers ---'
nl -ba src/youtube_extension/backend/api/v1/router.py | sed -n '1140,1210p'
echo '--- record_metrics definitions and call sites ---'
rg -n -C 4 '\brecord_metrics\s*\(' \
src/youtube_extension tests/unit/test_performance_monitor.py
echo '--- endpoint tests and response-field assertions ---'
rg -n -C 5 'performance/report|metrics_recorded|ingest_performance_report_v1' \
tests src/youtube_extension/backendLength of output: 5733 🏁 Script executed: #!/bin/bash
set -u
echo '--- batching implementation ---'
sed -n '220,390p' src/youtube_extension/backend/services/performance_monitor.py
echo '--- report endpoint ---'
sed -n '1140,1210p' src/youtube_extension/backend/api/v1/router.py
echo '--- record_metrics definitions and call sites ---'
rg -n -C 4 '\brecord_metrics\s*\(' \
src/youtube_extension tests/unit/test_performance_monitor.py || true
echo '--- endpoint tests and response-field assertions ---'
rg -n -C 5 'performance/report|metrics_recorded|ingest_performance_report_v1' \
tests src/youtube_extension/backend || trueLength of output: 15795
I agree with the single-transaction design. For best-effort telemetry, atomic batch persistence is preferable to a partial prefix. The implementation uses one The in-memory update behavior remains consistent with the existing path. Both paths add metrics to the buffer before persistence. Both paths evaluate thresholds after the write operation returns. The batch writer also closes the connection in Keep Keep the legacy I recommend tracking both adjacent defects in a follow-up issue if they are not already tracked. The response count can mislead API clients. The legacy writer can leak a database handle when an operation fails before ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Adds batched SQLite metric persistence to reduce connection and commit overhead.
Changes:
- Introduces batch recording and persistence methods.
- Batches system and frontend metrics.
- Adds batch-path regression tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
performance_monitor.py |
Implements batched metric writes and system sampling. |
router.py |
Batches frontend performance reports. |
test_performance_monitor.py |
Tests persistence, buffering, alerts, and failures. |
| "value": float(value), | ||
| "unit": str(stats.get("unit", "ms")), | ||
| }) | ||
| await performance_monitor.record_metrics(samples) |
There was a problem hiding this comment.
This was addressed in a later commit on this PR. test_performance_report_error (tests/unit/test_v1_router_extended.py) now patches record_metrics — not the inert singular record_metric — and asserts the 500, with a docstring calling out exactly this pitfall. Verified locally against the current head: test_performance_report_error and test_performance_alert_error both pass. No further change needed here.
Generated by Claude Code
| component=entry["component"], | ||
| metric_name=entry["metric_name"], | ||
| value=float(entry["value"]), | ||
| timestamp=entry.get("timestamp") or now, |
There was a problem hiding this comment.
Confirmed real. Both batch callers — _monitor_system_resources() and POST /api/v1/performance/report — submit entries without a timestamp, so the single shared now gave every row in a batch an identical timestamp, diverging from the serial record_metric (line 244), which stamps each metric with its own datetime.now() at construction.
Fix — move the fallback into the per-record comprehension so the batch path matches record_metric exactly:
- now = datetime.now(timezone.utc)
records = [
PerformanceMetric(
component=entry["component"],
metric_name=entry["metric_name"],
value=float(entry["value"]),
- timestamp=entry.get("timestamp") or now,
+ timestamp=entry.get("timestamp") or datetime.now(timezone.utc),
unit=entry.get("unit", "ms"),
tags=entry.get("tags") or {},
)
for entry in metrics
]Verified: 128/128 in tests/unit/test_performance_monitor.py, including the serial-vs-batched parity test (test_batch_matches_serial_buffer_and_collections).
Delivery note: this session is scoped to push only to claude/determined-maxwell-rswptp, where the fix is committed as 3f15c27 — not this PR's perf/metrics-batch-ingest head. Applying that commit (or authorizing the cross-branch push) lands it here.
Generated by Claude Code
|
Independent review (automated routine, triggered by your review request) — 1 real blocker. I red-teamed the diff and adjudicated the two open Copilot threads. The batching design is sound and well-tested, but there is one genuine test regression that must be fixed before this is green. 🔴 Blocker — confirms Copilot's
|
Three tests failed only under the full suite with PYTHONPATH=src. Root cause: the tests patched `perf_mod`, a module-level alias that can bind to a *different* module object than the one PerformanceMonitor's methods read their globals from. This file's preamble re-imports the module, and CI's PYTHONPATH=src lets the package resolve under a second name, so `monkeypatch.setattr(perf_mod, "psutil", fake)` silently no-ops. The real psutil then ran, and on a loaded CI machine cpu/memory exceeded the 80% warning thresholds, firing an alert. `_store_alert` opens its own sqlite3 connection, so the "one cycle, one connection" assertion saw 2, and the process-metrics test saw all 7 samples instead of 4. Resolve the target as sys.modules[PerformanceMonitor.__module__] so the patch lands regardless of import identity, and clear the metrics buffer before each cycle so leftover samples cannot bleed across tests. Separately, the report endpoint now batches through `record_metrics`; its error-path test still patched the singular `record_metric`, which is inert and let the request succeed with 200 instead of 500. Repointed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
@linear-code @coderabbitai review — batching surfaced three latent test defects. All fixed in Why three tests brokeThey only failed under the full suite with All three trace back to one mechanism. Root cause: patching a module the code never readsThe tests did: monkeypatch.setattr(perf_mod, "psutil", fake_psutil)
When that happens the patch silently no-ops. No error — the fake is installed somewhere nobody looks, and the real What the real psutil then did
and That is the whole explanation for The same missed patch explains The fixsys.modules[PerformanceMonitor.__module__]Resolve the target from the class at runtime. Correct under every import identity, needs no knowledge of which test perturbs module state, and is a strict improvement even when the alias happens to be right. Also added a Third regression, unrelated mechanism
Repointed to VerificationLocal, mirroring Down from On this PR: Still open for a reviewer's judgementFlagging two things I chose not to change, both disclosed in the PR body:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- commit summary and changed paths ---'
git show --stat --oneline --find-renames d85abd866
git diff-tree --no-commit-id --name-only -r d85abd866
echo '--- changed hunks ---'
git show --format=fuller --find-renames --unified=50 d85abd866 -- \
tests/unit/test_performance_monitor.py \
tests/unit/test_v1_router_extended.py \
src/youtube_extension/backend/services/performance_monitor.py \
src/youtube_extension/backend/api/v1/router.py
echo '--- relevant production symbols ---'
ast-grep outline src/youtube_extension/backend/services/performance_monitor.py \
--items all --view expanded \
--match 'PerformanceMonitor|record_metrics|_store_metrics|_monitor_system_resources'
ast-grep outline src/youtube_extension/backend/api/v1/router.py \
--items all --view expanded \
--match 'performance_report|record_metrics'
echo '--- imports and test call contexts ---'
rg -n -C 5 \
'perf_mod|sys\.modules\[PerformanceMonitor\.__module__\]|metrics_buffer\.clear\(\)|test_system_resource_cycle|test_performance_report(_error)?|record_metrics' \
tests/unit/test_performance_monitor.py tests/unit/test_v1_router_extended.py \
src/youtube_extension/backend/services/performance_monitor.py \
src/youtube_extension/backend/api/v1/router.pyLength of output: 22985 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- repository state and candidate test files ---'
git rev-parse --short HEAD
git status --short
fd -a -i 'test_performance_monitor.py|test_v1_router_extended.py' . | sort
echo '--- test import/re-import preamble from the reviewed commit ---'
git show d85abd866:tests/unit/test_performance_monitor.py | sed -n '1,150p'
echo '--- all module-level sqlite patches in the batch-test class ---'
git show d85abd866:tests/unit/test_performance_monitor.py \
| nl -ba \
| sed -n '1140,1435p' \
| grep -E -C 4 'perf_mod\.sqlite3|_impl_module|patch\.object|monkeypatch\.setattr'
echo '--- production batching and system-cycle implementations ---'
nl -ba src/youtube_extension/backend/services/performance_monitor.py | sed -n '225,380p'
nl -ba src/youtube_extension/backend/services/performance_monitor.py | sed -n '510,575p'
echo '--- report endpoint input transformation ---'
nl -ba src/youtube_extension/backend/api/v1/router.py | sed -n '1165,1200p'Length of output: 6423 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- candidate test paths ---'
find . -type f \( -name 'test_performance_monitor.py' -o -name 'test_v1_router_extended.py' \) -print | sort
echo '--- sqlite and psutil patch sites in the batch-test class ---'
git show d85abd866:tests/unit/test_performance_monitor.py \
| sed -n '1140,1435p' \
| grep -n -E -C 4 'perf_mod\.sqlite3|_impl_module|patch\.object|monkeypatch\.setattr'
echo '--- record_metrics and _store_metrics ---'
sed -n '225,385p' src/youtube_extension/backend/services/performance_monitor.py
echo '--- system resource sampling ---'
sed -n '510,575p' src/youtube_extension/backend/services/performance_monitor.py
echo '--- report endpoint transformation ---'
sed -n '1165,1200p' src/youtube_extension/backend/api/v1/router.py
echo '--- timestamp fields supplied to record_metrics callers ---'
grep -RIn -E -C 4 'record_metrics[[:space:]]*\(' \
src/youtube_extension/backend tests 2>/dev/null || trueLength of output: 18761
The module-resolution fix is correct. The remaining The report error-path test now patches The shared timestamp is correct for the current callers. One system-resource cycle is one sampling instant. One frontend report is one payload. Neither caller supplies per-entry timestamps. Do not change this behavior in this PR. Keep the legacy 🐇🔎 ✏️ Learnings added
You are interacting with an AI system. |
|
Re-verified after The test regression my previous pass flagged is fixed.
All other PR gates green — Security Scan, CodeQL, Secret Scan, Dependency Review, PR Checks, PR Governance, Agent-completion; E2E skipped by config. The one remaining red, One residual, non-blocking (optional): the happy-path The two pre-existing defects you deliberately deferred — the Terminal state: HALTED(awaiting_merge_approval). Green on code and checks; the only step left is your merge to protected Generated by Claude Code |
Review follow-up. The happy-path report test patched the singular `record_metric`, but the endpoint calls `record_metrics`. The patch was therefore inert: the test still returned 200 and still asserted the count, while the request performed a live in-process SQLite write. It passed for the wrong reason and had silently lost its isolation. Repoint the patch to `record_metrics` and assert the batched call shape -- one await for the whole report, carrying both samples with the expected names, values and component. The await-count assertion is the substantive part. It pins the contract this endpoint exists to provide: a regression to one write per metric would preserve both the 200 and `metrics_recorded`, so nothing else in the suite would notice. Prove-failed by reverting the patch target to `record_metric`: the request still returns 200 and the new assertion fails `assert 0 == 1`, confirming the guard catches exactly the defect it was written for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@linear-code @coderabbitai — actioned the residual finding in The finding was correct, and worse than "hygiene"You flagged that the happy-path
That is the same defect class this PR already fixed twice elsewhere, and it fails in the quiet direction: no error, no slowdown large enough to notice, just a silently un-isolated test. FixRepointed the patch to assert mock_record.await_count == 1
(samples,) = mock_record.await_args.args
assert [s["metric_name"] for s in samples] == ["lcp", "fid"]
assert [s["value"] for s in samples] == [1200.0, 30.0]
assert {s["component"] for s in samples} == {"frontend"}The await-count line is the substantive part. It pins the contract this PR exists to deliver. A regression to one write per metric would keep the Prove-failReverted the patch target to The Verification
Still deferred, deliberatelyBoth remain out of scope as agreed — pre-existing, unrelated to batching, and better tracked separately than smuggled into a cleared PR:
I'll file these as a follow-up issue rather than expand this diff. Re-requesting a look at |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- commit and changed paths ---'
git show --stat --oneline --find-renames 69f277915
git diff-tree --no-commit-id --name-only -r 69f277915
echo '--- narrow commit diff ---'
git show --format=fuller --find-renames --unified=45 69f277915 -- \
tests/unit/test_v1_router_extended.py
echo '--- performance-report endpoint structure ---'
ast-grep outline src/youtube_extension/backend/api/v1/router.py \
--items all --view expanded --match 'performance_report|record_metrics'
echo '--- endpoint and relevant tests ---'
rg -n -C 18 \
'async def ingest_performance_report_v1|def test_performance_report\b|def test_performance_report_error\b|record_metrics|record_metric' \
src/youtube_extension/backend/api/v1/router.py \
tests/unit/test_v1_router_extended.pyLength of output: 15019 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- locate the test file in the worktree ---'
fd -a -i 'test_v1_router_extended.py' . || true
echo '--- test module imports, fixtures, and performance-report tests from reviewed commit ---'
git show 69f277915:tests/unit/test_v1_router_extended.py \
| nl -ba \
| sed -n '1,150p;640,735p;2150,2215p'
echo '--- router monitor construction and v1 router inclusion ---'
rg -n -C 6 \
'performance_monitor\s*=\s*PerformanceMonitor|include_router\(v1_router\)|from .*router import|import .*router' \
src/youtube_extension/backend/api/v1/router.py \
src/youtube_extension/backend/main.pyLength of output: 1107 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- test module imports and client setup from reviewed commit ---'
git show 69f277915:tests/unit/test_v1_router_extended.py \
| sed -n '1,170p'
echo '--- performance-report happy and error tests from reviewed commit ---'
git show 69f277915:tests/unit/test_v1_router_extended.py \
| sed -n '640,735p;2150,2215p'
echo '--- router monitor definition and application router inclusion ---'
rg -n -C 6 \
'performance_monitor\s*=\s*PerformanceMonitor|include_router\(v1_router\)|from .*router import|import .*router' \
src/youtube_extension/backend/api/v1/router.py \
src/youtube_extension/backend/main.pyLength of output: 16784
The test now patches the exact method that the endpoint awaits: The test application includes the same imported The new assertions match the endpoint transformation at Line 1183 through Line 1193:
This is a substantive regression test. It detects a return to per-metric writes even though the HTTP response and The error-path test also patches ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
`record_metrics` docstring promises "The observable behaviour is identical to calling `record_metric` once per entry". It was not: the batch hoisted a single `now = datetime.now(timezone.utc)` out of the comprehension and gave every record in the batch that same timestamp, while the serial `record_metric` stamps each metric at the moment it is recorded. Nothing tested the timestamp in either direction, so the divergence was free to persist. It was raised on #1341 (Copilot) and again on #1356, and has now survived two reviews unfixed. Consult the clock per record so the batch is a true drop-in. The `entry["timestamp"]` escape hatch is unchanged: an explicitly supplied timestamp is still honoured and the clock is only read for entries that omit it. Cost is one extra clock read per metric — the background monitor records 7 per 30s cycle, so it is not measurable against the SQLite commit the batch exists to collapse. Three tests pin it. Rather than assert timestamps merely differ — wall-clock resolution is coarse enough that several `now()` calls in a tight loop can legitimately return the same value — they patch the module clock to walk a known sequence, so the Nth record must carry the Nth instant. That holds only if the clock is consulted once per record, in order. Two of the three fail against the shared-`now` code and pass with this change; the third guards the explicit-timestamp path, which was never broken. Claude-Session: https://claude.ai/code/session_019baCDT5aP5Z66pLGBCE2Y6 Co-authored-by: Claude <noreply@anthropic.com>
Canonical issue
Closes #1339
Outcome
record_metric()opens a SQLite connection, INSERTs one row, and commits — onceper metric. Two callers submit metrics in groups, so they paid that cost N times
for what is logically one write:
_monitor_system_resources()emits 7 metrics every 30s, forever, from thebackground monitoring loop → ~20,160 connect+commit cycles/day.
POST /api/v1/performance/reportreplays an entire client-side batch throughthe same per-metric path; a browser posting 50 samples cost 50 cycles.
This adds
record_metrics(metrics)and_store_metrics(metrics), which:self._lockonce for the whole batch instead of once per metric,metrics_bufferand the three fast-access deques in one critical section,connect→executemany→commit.The background cycle drops from 7 connections per tick to 1 (~2,880/day), and
the report endpoint from N to 1.
record_metric()and_store_metric()are unchanged. The ~15 genuinesingle-metric callers across the codebase keep their existing behaviour; this
is purely additive plus two call-site rewrites.
Risk
Low, and deliberately bounded:
untouched.
test_serial_path_still_opens_one_connection_per_metricpins itscurrent behaviour so a future refactor can't silently reroute it.
_check_alert_thresholdsis per-metric and pure;it is still called once per metric, just after the batch write rather than
interleaved with it.
fail on 4. The batch is one transaction, so it is all-or-nothing. For
best-effort telemetry that is an improvement (no torn batches), but it is a
real semantic difference and is called out here deliberately.
asyncio.to_thread+try/finally._store_metricscloses its connectionin a
finally, so a failedexecutemanycannot leak the handle. (The existing_store_metricdoes not do this. That is a pre-existing bug and isintentionally left alone — fixing it here would put an unrelated correctness
change in a perf PR.)
returns
metrics_recorded: len(metrics), which counts all submitted entriesincluding non-numeric ones that get skipped. That was already true before this
change and the value is preserved verbatim, so this PR is not the place to
alter a response field. Flagging it so a reviewer doesn't read it as new.
Verification
tests/unit/test_performance_monitor.py— 128 passed (119 pre-existing + 9 new).Prove-fail — the 9 new tests were run against the pre-change source
(
git stashof both source files, test file retained):The 7 failures are the batching assertions. The 2 passes are intentional
control tests that must hold on both sides of the change:
test_serial_path_still_opens_one_connection_per_metric(pins the old path) andtest_system_resource_cycle_survives_process_metrics_failure(pins best-effortpsutil.Processhandling). A test that passes before and after is onlymeaningful as a guard, and both are here for that reason.
New coverage:
test_batch_opens_exactly_one_connectionsqlite3.connect, the core claimtest_serial_path_still_opens_one_connection_per_metrictest_batch_persists_every_row_with_correct_valuestest_batch_matches_serial_buffer_and_collectionstest_empty_batch_does_no_database_worktest_batch_evaluates_thresholds_for_every_metrictest_batch_swallows_database_errorstest_system_resource_cycle_uses_a_single_connectiontest_system_resource_cycle_survives_process_metrics_failurepsutil.Processraising still yields the 4 system metricsA real bug the parity test caught.
PerformanceMonitor.__init__callsstart_monitoring()whenever an event loop is running — which every async testprovides. That background task writes real system metrics into the same buffer
under assertion, so the strict serial-vs-batched comparison failed on a stray
cpu_usage_percentsample nobody recorded. The new tests build monitors througha
_quiesced()helper that cancels the task first. Looser assertions (len > 0)would never have surfaced this.
Lint:
ruff check --output-format=conciseover all three changed files reports25 findings, identical rule-for-rule to
origin/main(all pre-existingB904inrouter.py). Compared by rule+count against agit stashbaseline ofthe real paths, not by line number.
Production evidence
src/youtube_extension/backend/services/performance_monitor.py—_background_monitoring()loopswhile self.monitoring_enabled:withawait asyncio.sleep(30), calling_monitor_system_resources()each tick.That method emitted 7 serial
record_metriccalls. This is live in-processwork, not a test harness:
__init__starts the task automatically whenever anevent loop is present.
src/youtube_extension/backend/api/v1/router.py:118—performance_monitor = PerformanceMonitor()at module scope;@router.post("/performance/report")iterates the client's metric list andcalled
record_metricper entry.backend/main.py:35importsv1_routerandbackend/main.py:164callsapp.include_router(v1_router).Scope
Two call sites repointed; two new methods added. No change to the monitoring
cadence, the metric schema, the DB schema, the alerting rules, or any response
payload.
Agent handoff
@linear-code@coderabbitai— please review. Two things worth your attention:single transaction to be correct for best-effort telemetry, but if you'd
rather see per-row resilience, say so and I'll add it.
metrics_recorded: len(metrics)miscount or the missing
try/finallyin the legacy_store_metric, on thegrounds that neither belongs in a performance PR. Push back if you'd prefer
them folded in.