fix(telemetry): usage counters get canaries, task references, and the missing integration tests (#2663)
The deployed instance ran with every usage counter at zero while surfacing demonstrably fired. Every unit test was green because every unit test mocked either _schedule or the session — the two functions that touch the database ran against real Postgres nowhere. Both telemetry writers also held no reference to their fire-and-forget tasks (the loop keeps only weak ones), and swallowed every failure into logger.debug, so a total outage was indistinguishable from an unused corpus. - note_usage + retrieval_telemetry keep strong task references until done - failures log at WARNING; note_usage additionally drops one AppLog error row per process per site, so the admin UI shows the outage without host access - integration tests cover _insert_events -> usage_for_notes and the full record_pulled chain on a running loop, splitting the write and read halves so a failure names its side Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -15,8 +15,13 @@ Design notes (mirrors retrieval_telemetry, for the same reasons):
|
||||
- Writes are fire-and-forget. `record_surfaced` / `record_pulled` extract
|
||||
plain ints synchronously and schedule the insert as a background task, so
|
||||
telemetry never adds latency to — or can break — the surface it observes.
|
||||
- Every failure path is swallowed. Losing a usage row costs a data point;
|
||||
raising would cost the operator their retrieval.
|
||||
- Failures degrade, but they must not degrade SILENTLY. The original version
|
||||
swallowed everything into logger.debug, and the deployed instance ran with
|
||||
every counter at zero for weeks while surfacing demonstrably fired — a
|
||||
total outage indistinguishable from "nobody uses this" (#2663). A
|
||||
subsystem whose every failure mode is invisible cannot report its own
|
||||
death, so failures now log at WARNING and drop one AppLog error row per
|
||||
process per site, where the admin UI shows it.
|
||||
- Reads (`usage_for_notes`) are NOT fire-and-forget — a readout the caller
|
||||
awaits, aggregated in one round-trip for a whole page of snippets rather
|
||||
than per row.
|
||||
@@ -25,6 +30,7 @@ from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import logging
|
||||
import traceback
|
||||
|
||||
from sqlalchemy import case, func, select
|
||||
|
||||
@@ -33,26 +39,67 @@ from scribe.models.note_usage import PULLED, SURFACED, NoteUsageEvent
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Strong references to in-flight inserts. The event loop keeps only a WEAK
|
||||
# reference to a task, so a fire-and-forget create_task with no other holder
|
||||
# can be garbage-collected before it completes — a write that never errors and
|
||||
# never lands. The done-callback discard keeps the set from growing.
|
||||
_pending: set[asyncio.Task] = set()
|
||||
|
||||
# Sites that already dropped their once-per-process AppLog row. The readout
|
||||
# runs on every snippet list render — without this, a broken table would turn
|
||||
# the error log into a firehose that buries the finding it exists to surface.
|
||||
_reported: set[str] = set()
|
||||
|
||||
|
||||
async def _report_failure(site: str) -> None:
|
||||
"""Make a swallowed telemetry failure visible. Called from an except block.
|
||||
|
||||
WARNING to the process log every time; one AppLog error row per process per
|
||||
site so the admin UI shows the outage without host access. The AppLog write
|
||||
is itself guarded — when the whole database is down it fails too, and that
|
||||
is fine: the WARNING already said so, and a canary must never take down the
|
||||
surface it watches.
|
||||
"""
|
||||
logger.warning("note usage telemetry %s failed", site, exc_info=True)
|
||||
if site in _reported:
|
||||
return
|
||||
_reported.add(site)
|
||||
try:
|
||||
from scribe.services.logging import log_error
|
||||
|
||||
await log_error(
|
||||
endpoint="note_usage",
|
||||
error_type=f"note_usage_{site}_failed",
|
||||
error_message=f"note usage telemetry {site} is failing; "
|
||||
"usage counters will read zero until this is fixed",
|
||||
traceback=traceback.format_exc(),
|
||||
)
|
||||
except Exception:
|
||||
logger.debug("note usage canary write failed", exc_info=True)
|
||||
|
||||
|
||||
async def _insert_events(rows: list[dict]) -> None:
|
||||
"""Persist usage rows. Best-effort: all errors are swallowed."""
|
||||
"""Persist usage rows. Best-effort: failures degrade, visibly."""
|
||||
try:
|
||||
async with async_session() as session:
|
||||
session.add_all([NoteUsageEvent(**row) for row in rows])
|
||||
await session.commit()
|
||||
except Exception:
|
||||
logger.debug("note usage telemetry write skipped", exc_info=True)
|
||||
await _report_failure("write")
|
||||
|
||||
|
||||
def _schedule(rows: list[dict]) -> None:
|
||||
if not rows:
|
||||
return
|
||||
try:
|
||||
asyncio.get_running_loop().create_task(_insert_events(rows))
|
||||
task = asyncio.get_running_loop().create_task(_insert_events(rows))
|
||||
except RuntimeError:
|
||||
# No running loop (sync context outside the app) — skip rather than
|
||||
# block. Every app path runs on the loop.
|
||||
logger.debug("note usage telemetry skipped — no running event loop")
|
||||
return
|
||||
_pending.add(task)
|
||||
task.add_done_callback(_pending.discard)
|
||||
|
||||
|
||||
def record_surfaced(
|
||||
@@ -168,8 +215,10 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]:
|
||||
)
|
||||
).all()
|
||||
except Exception:
|
||||
# A telemetry readout must not be able to break the list it decorates.
|
||||
logger.debug("note usage readout failed", exc_info=True)
|
||||
# A telemetry readout must not be able to break the list it decorates —
|
||||
# but it must say it failed, or a broken readout is indistinguishable
|
||||
# from a corpus nobody uses (#2663).
|
||||
await _report_failure("readout")
|
||||
return out
|
||||
|
||||
for note_id, event, n, last_at, ambient in rows:
|
||||
|
||||
Reference in New Issue
Block a user