Two pre-existing defects in the performance-metrics path, both surfaced during review of #1341 and deliberately left out of that diff to keep a cleared PR narrow. Neither is a regression from #1341; both predate it.
1. metrics_recorded overcounts
ingest_performance_report_v1 (src/youtube_extension/backend/api/v1/router.py) returns:
return {"status": "ok", "metrics_recorded": len(metrics)}len(metrics) counts keys submitted, but the ingest loop only appends a sample when the value is numeric:
value = stats.get("current") if isinstance(stats, dict) else None
if isinstance(value, (int, float)):
samples.append({...})A report mixing numeric and non-numeric values reports more written than were written. A client sending {"lcp": {"current": 1200}, "note": {"current": "n/a"}} is told 2; one row is persisted.
Suggested fix: return len(samples).
Worth noting this also makes the endpoint's own contract untestable from the response alone — the count cannot distinguish "batched correctly" from "wrote nothing", which is why #1341 added an explicit await_count assertion on the mock instead of relying on this field.
2. Legacy _store_metric lacks try/finally
_store_metric in src/youtube_extension/backend/services/performance_monitor.py opens a SQLite connection without a try/finally, so an exception between open and close leaks the handle. Its batched sibling added in #1341 does this correctly.
Suggested fix: mirror the batched path, or route the singular call through it with a one-element list.
Context
Both were raised in review of #1341 and confirmed as correctly out of scope for it. Filing here so they are tracked rather than lost.
Two pre-existing defects in the performance-metrics path, both surfaced during review of #1341 and deliberately left out of that diff to keep a cleared PR narrow. Neither is a regression from #1341; both predate it.
1.
metrics_recordedovercountsingest_performance_report_v1(src/youtube_extension/backend/api/v1/router.py) returns:len(metrics)counts keys submitted, but the ingest loop only appends a sample when the value is numeric:A report mixing numeric and non-numeric values reports more written than were written. A client sending
{"lcp": {"current": 1200}, "note": {"current": "n/a"}}is told2; one row is persisted.Suggested fix: return
len(samples).Worth noting this also makes the endpoint's own contract untestable from the response alone — the count cannot distinguish "batched correctly" from "wrote nothing", which is why #1341 added an explicit
await_countassertion on the mock instead of relying on this field.2. Legacy
_store_metriclackstry/finally_store_metricinsrc/youtube_extension/backend/services/performance_monitor.pyopens a SQLite connection without atry/finally, so an exception between open and close leaks the handle. Its batched sibling added in #1341 does this correctly.Suggested fix: mirror the batched path, or route the singular call through it with a one-element list.
Context
Both were raised in review of #1341 and confirmed as correctly out of scope for it. Filing here so they are tracked rather than lost.