fix(telemetry): fail loudly when collection stops, and detect a drained feed#24
Merged
Merged
Conversation
The collector had three separate silent `exit 0` paths — REGISTRY_URL unset, feed fetch failure, feed validation failure. Each was individually defensible (a transient feed must not fail the daily workflow, and must never write zeros over a good snapshot), but together they made an outage indistinguishable from a healthy no-op. Registry #283 moved the ingest route on 2026-06-26 and live CLI telemetry was dropped until the #299 alias on 2026-07-03 — seven days, no signal, clean logs throughout. It was also the only collector behaving this way: npm, docker, huggingface and pypi all exit 1 on missing config. The rule now, in classifyCollectionSkip (pure, no db/network/clock, so it is actually testable): unset + never collected -> warning, exit 0 (legitimately unprovisioned) unset + previously collected -> error, exit 1 (config removed; not a blip) set + never collected -> error, exit 1 (broken feed or wrong URL) set + failed <= 2 days -> warning, exit 0 (transient) set + failed > 2 days -> error, exit 1 (outage; dashboard is stale) A single bad run stays tolerated. A sustained one fails. Failures also emit GitHub Actions annotations so they surface in the run summary rather than in step output nobody opens. The workflow step is now continue-on-error. The step runs before generate-md, generate-summary and the commit step, so a hard exit 1 there would silently cost a full day of npm/pypi/docker/hf/chrome data for every other collector — the fix must make failure loud without making it destructive. No behaviour change in today's real state: REGISTRY_URL is unset and no snapshot has ever been stored, so this warns and exits 0 exactly as before, but now says so. Verified end to end: unconfigured -> exit 0 + ::warning::; configured with an unreachable feed -> exit 1 + ::error::. 39 tests pass, including a regression test for the #283 shape (a 7-day gap must exit 1). Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
README:104 claimed the collector 'skips gracefully when unset', which is no longer the whole truth: it warns when unprovisioned, but errors when the variable is removed after telemetry was previously collected, and when a failure becomes sustained. Adds the escalation table, the TELEMETRY_STALE_AFTER_DAYS knob, and states the two properties the original silence existed to protect and which still hold: a failed run never writes (zeros must not overwrite a good snapshot), and a telemetry outage never costs the other collectors their data for the day. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
… outages
Adversarial review found the previous two commits shipped a false premise and
two real defects. Correcting all of it.
THE PREMISE WAS WRONG. I justified this change with registry #283, an incident
it cannot detect. #283 moved the INGEST path (CLIs POSTing heartbeats in). This
collector reads the ADOPTION FEED, a different endpoint that stayed up and would
have answered HTTP 200 with a valid all-zeros payload — normalizeAdoptionFeed
accepts that (correctly; it is well-formed), persist() writes it,
lastSuccessDate advances, and classifyCollectionSkip is never called. The
collector was also created 2026-07-02, one day before #283 was fixed, so it did
not exist during the gap. The claim appeared in three comments, a test name and
the README.
Rather than just delete the story, add the detector that actually catches that
shape: classifyFeedHealth compares against the previous snapshot and errors when
a live fleet reports mau=0 AND installs=0. Users do not all vanish overnight.
The zeros are still recorded as reported — we do not suppress what the feed
said; raising the alarm is the job. Only a total collapse of both counts
triggers: a decline, or MAU-only churn, is not assumed to be an outage.
HIGH — log injection. httpGetJson embeds up to 200 bytes of the registry's raw
response body into the message; a JSON parse error echoes body bytes even on a
200. That message reached an ::annotation:: on stdout AND console.error on
stderr, and the runner scans both. Only annotate() scrubbed, and its /\r?\n/
missed a lone \r — .NET line readers break on \r, \n and \r\n alike. Verified
against a hostile server: the old code emitted three attacker-controlled
commands including ::stop-commands::, letting a compromised registry suppress
the outage alert this code exists to raise. Now sanitized once at the boundary
(classifyCollectionSkip sanitizes its own output, so every consumer is safe by
construction), covering \r, \n, U+2028/29/0085, defanging `::`, and bounding
length. Re-verified: 0 injected commands, 0 surviving CRs.
HIGH — the exit 1 was swallowed. continue-on-error means the job goes green, and
a green scheduled run notifies nobody: the change relocated the silence rather
than ending it. The telemetry step now has an id, and a post-commit step
re-raises the failure so the run goes red and the notification fires — after the
day's npm/pypi/docker/hf/chrome data is safely committed.
MEDIUM — a FUTURE last-success date (clock skew, bad row) made age negative,
which never exceeds the threshold, pinning the collector to "warning" forever
and silently disabling escalation. Now errors.
MEDIUM — lastSuccessDate() mapped every db error to null, so a corrupt or
unreadable database asserted "has never succeeded" and pointed the operator at
the feed. Now three-valued: known / never / unknown, with unknown getting its
own honest message. A missing table is still legitimately 'never'.
MEDIUM — Number('') === 0 and Number.isInteger(0) is true, so my first guard let
an empty `${{ vars.UNSET }}` set the tolerance to zero and error on the first
blip. Empties are rejected before coercion.
Also: the impure glue was at 0% coverage and every real bug above lived in it.
collect-telemetry-stats.js now exports its internals for test, mirroring
collect-chrome-stats.js which already does this for parseListing.
Also: raw U+2028 bytes pasted into source made `file` and `grep` classify
lib/telemetry.js as binary data. Written as escapes now.
52 tests pass (was 39). New tests fail against the previous commit.
Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
The README justified the loud-failure work with registry #283, an incident this collector structurally cannot detect: #283 broke the INGEST path, while this reads the ADOPTION FEED — a different endpoint that stayed up and would have answered 200 with zeros. Replaced with what is actually true, and documented the zero-collapse detector that does catch that shape. Also softened the same claim in classifyFeedHealth's docstring: the collector was written 2026-07-02, after the gap closed, so it never observed the feed's behaviour during #283. Stated as the mechanism, not as an observation. Documents the log-injection boundary while here — untrusted feed text is sanitized before reaching any log line, because the runner parses ::command:: on both stdout and stderr. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
Verification of the previous commit found the detector I added to catch an
ingest outage would not have caught one, and would then have alerted once.
WRONG ENVELOPE. It required mau === 0 AND installs === 0. Those are different
windows: total_installs is distinct installs over the 90-day retention, mau is
30 days (setup-database.js). When ingest breaks, MAU empties at T+30 while
installs coasts down until T+90:
T+30 mau=0 installs=333 healthy <- fleet provably dead, no alert
T+60 mau=0 installs=167 healthy
T+90 mau=0 installs=0 ERROR <- first alert, 90 days late
Sixty days of zero monthly actives against a live install base passed as
healthy — and a test of mine encoded that blind spot as intentional ("could be
real churn"). Zero actives against 500 known installs is not churn. Now keyed on
MAU alone; installs surviving is stated as corroboration, because it is exactly
what a drained ingest looks like.
SELF-SILENCING. It compared against yesterday, so day one's zeros persisted and
became the baseline: every later day saw zero-following-zero and reported
healthy. One red run, then green forever while serving zeros — inverting this
module's own escalate-on-sustained rule. Now compares against the last snapshot
that actually had users, so the alarm keeps ringing and states the growing gap.
Also:
- Added a feed request timeout (30s, TELEMETRY_FEED_TIMEOUT_MS). A registry that
accepts the connection and never answers would otherwise hang the daily
workflow until the job limit killed it — a silent failure of a different shape
that also takes the other collectors' commit down with it. Found while
debugging my own test deadlock.
- dbPath is overridable via TELEMETRY_DB_PATH so the glue is testable. Every
defect three review rounds found lived in that glue, and it had 0% coverage
while the path was hardcoded.
- New test/telemetry-collector.test.js drives the REAL script as a subprocess
against fixture databases and a hostile HTTP server: injection on both streams,
corrupt store, drained feed, sustained drain, healthy run, brand-new
deployment, and the require.main guard. The previous commit's export comment
claimed it existed "so it needs to be reachable from a test" — no such test
had been written. Now it has.
61 tests pass (was 52).
Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
…n rule `git push || echo "No changes to push"` reported the run green whenever the push failed for ANY reason — non-fast-forward, a rejected hook (this repo has a local pre-push hook that blocks data/analytics.db), an auth error — with the day's data never pushed. That directly undercuts the premise of the new Surface step, which re-raises a telemetry outage on the grounds that the data is "safely committed". Now distinguishes nothing-to-push from a failed push. README: describes the drain rule as it now works — keyed on MAU alone (installs is a 90-day window against MAU's 30, so requiring both hid ~60 days of a dead pipeline) and compared against the last snapshot with users (comparing to yesterday made the alert fire once and then go quiet forever). Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
The final gate caught data/telemetry-badge.json in the diff. My own integration
test wrote it: I made dbPath injectable but left writeBadge's path hardcoded, so
the healthy-feed case wrote a real badge into data/ — one reading
"label": "active CLI users (MAU)", "message": "0"
That file is committed by the daily workflow and rendered as a shield, so a test
fixture would have published a fabricated "0 active users" claim as if it were
measured. It was introduced by becf327 and is removed here; it does not exist on
main.
writeBadge now honours TELEMETRY_BADGE_PATH, and the tests point it at a temp
dir. Production is unchanged — unset, it resolves to the committed path, and the
workflow still commits the real badge.
Verified: the full suite leaves data/ clean.
Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Nothing ran it. The only workflow in this repo was the daily collector, so every test in test/ was decoration: it passed on whoever last remembered to run it locally, and a change that broke it would merge green. That is the same shape as the silent-collector bug the suite exists to guard — a check that never fires looks identical to a check that passes. Runs on pull_request and on push to main: npm test, then the build. Also asserts the suite leaves the working tree clean. The tests drive the real collector against fixture databases and write a badge; the badge path was hardcoded until this branch, so a fixture wrote "active CLI users (MAU): 0" into data/ — a file the daily workflow commits and a shield renders. That was caught by hand during review. This makes it fail the build instead. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A
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
1 participant
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.
Why
Three silent
exit 0paths inscripts/collect-telemetry-stats.js— unsetREGISTRY_URL, fetch failure, validation failure. Each defensible alone (a transient feed must not fail the daily workflow, and zeros must never overwrite a good snapshot); together they made a broken collector look exactly like a healthy no-op. It was also the only collector doing this — npm, docker, huggingface and pypi all exit 1 on missing config.What this does
Retrieval failures escalate. A blip warns; a sustained one fails:
REGISTRY_URLunset, never collectedTELEMETRY_STALE_AFTER_DAYS(default 2)A drained feed is detected. This is the failure a retrieval check structurally cannot see, and the one a broken ingest actually produces: the feed answers
200with a well-formed payload whose active counts have emptied.normalizeAdoptionFeedaccepts it (correctly — it's valid),persist()writes it, last-success advances, and the retrieval classifier is never called.Two details, both learned from review:
total_installsis a 90-day window,mauis 30-day, so a broken ingest empties MAU at T+30 while installs coasts to T+90. Requiring both zero waved through ~60 days of a dead pipeline.The zeros are still recorded as reported. We don't suppress what the feed said; raising the alarm is the job.
Security: log injection (found in review, fixed, verified)
httpGetJsonembeds up to 200 bytes of the registry's raw response body into the message, and a JSON parse error echoes body bytes even on a200. That message reached an::annotation::on stdout andconsole.erroron stderr — the runner scans both. Onlyannotatescrubbed, and its/\r?\n/missed a lone\r(.NET line readers break on\r,\nand\r\nalike).Verified against a hostile server: the old code emitted three attacker-controlled commands including
::stop-commands::— letting a compromised registry suppress the very outage alert this PR adds. Now sanitized once at the boundary (the classifier sanitizes its own output, so every consumer is safe by construction). Re-verified: 0 injected commands, 0 surviving CRs.The premise was wrong, and is corrected
The first version justified all this with registry #283 — an incident it cannot detect. #283 broke the ingest path; this collector reads the adoption feed, a different endpoint that stayed up and would have answered
200with zeros. The collector was also created 2026-07-02, one day after the gap closed. That false story was in three comments, a test name and the README.Rather than just delete it, this adds the detector that catches the real shape. The remaining
#283reference is explicitly flagged as the mechanism, not an observation.Also fixed
exit 1was being swallowed.continue-on-errormeant the job went green — a scheduled run notifying nobody. The step now has anidand aSurface telemetry outagestep re-raises after the commit, so the run goes red without a telemetry outage costing the other collectors their data.git push || echo "No changes to push"swallowed every push failure and reported green with nothing pushed — undercutting the premise that the data is safely committed.Number('') === 0— my first guard let an unset${{ vars.X }}set the tolerance to zero.writeBadge's path was hardcoded, so a fixture wrote"active CLI users (MAU)": "0"intodata/— a file the workflow commits and a shield renders. Caught by the gate; both paths are now injectable.lib/telemetry.jsregister as binary tofileandgrep.Verification
test/telemetry-collector.test.jsdrives the real script as a subprocess against fixture databases and a hostile HTTP server: injection on both streams, corrupt store, drained feed, sustained drain, healthy run, brand-new deployment, therequire.mainguard.TELEMETRY_DB_PATH/TELEMETRY_BADGE_PATHexist so it is testable at all.data/untouched · the suite leaves the repo clean.Behaviour today
REGISTRY_URLis unset and no snapshot exists, so this warns and exits 0 exactly as before — but now says so out loud.https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A