feat(telemetry): measure which lint rules fire, cost, and fail to converge - #3367
Conversation
…verge Lint rule changes are currently argued from anecdote. This adds the three measurements needed to argue them from data. `lint_report`, once per `hyperframes lint` or `hyperframes check`: - `code_counts` / `codes` — which rules actually fire, and how often - `rule_group_ms` — milliseconds per rule-source module (core, gsap, media, ...) - `slowest_rule` / `slowest_rule_ms` — slowest single rule as `<group>#<index>` - `rule_count` — how many rules this build ran `lint_rule_streak`, once per finding that survives an edit to its file: - `edits` — how many edits the finding survived - `cleared` — whether it eventually went away The streak event is the one that matters. A lint pass costs about 5ms, so per-rule CPU is not what makes the authoring loop slow; a rule an agent cannot satisfy is, because every failed attempt costs a full edit-and-relint cycle. A single run cannot see that, so `lint_rule_streak` reconstructs it across runs: high `edits` with `cleared: false` is a rule nobody can fix, and the `cleared: true` distribution is the baseline to judge it against. An iteration is counted only when the file's content digest CHANGED and the finding is still there. Re-linting an untouched project is not an attempt, which is what stops `check` (which lints on every invocation) from inflating the numbers. Rule identity is the source module plus an index within it. Naming all 86 rules would make the timings prettier but it is a refactor this measurement does not need: the group locates the file, and the index locates the rule. Version, agent runtime, CI flag, and invocation id are already attached to every event by `trackEvent`, so lint pain can be split by CLI version and by which agent produced it without adding anything here. Privacy: only rule codes, counts, and timings are sent. Streak state lives in ~/.hyperframes/lint-streaks.json alongside config.json (so `rm -rf ~/.hyperframes` is still a full reset) and stores digests only — no file paths, no project names, no composition source. Nothing is written and nothing is emitted when telemetry is off. Entries expire after 14 days and are capped at 500 files. `EventProperties` gains string arrays and numeric maps. `codes` and `code_counts` are inherently a set and a histogram; flattening them into dynamic top-level keys would make them unqueryable. PostHog stores both natively. `trackLintRun` is the single call site shared by `lint` and `check`, and it swallows every error — telemetry must never turn a green lint red.
terencecho
left a comment
There was a problem hiding this comment.
LGTM. Walked the telemetry path end-to-end against the four things I care about on a PR that adds instrumentation to a hot command: leak, skew, cost, fail-safe.
Leak — nothing sensitive escapes
trackLintReport only sends rule codes, counts, and timings; trackLintRuleStreak only sends code/severity/edits/cleared/command. No file paths, no projectDir, no HTML source, no cwd. Verified against packages/cli/src/telemetry/events.ts (line 91+) and lintRun.ts.
On-disk state at ~/.hyperframes/lint-streaks.json stores only truncated sha256 digests (fileKey = sha256(projectDir + ' ' + file).slice(0,16), contentHash = sha256(html).slice(0,16)) plus code/severity/edits/updatedAt. Confirmed by stores no file paths or project names on disk in lintStreaks.test.ts. Good.
Fail-safe — telemetry can't turn a green lint red
trackLintRun wraps everything in try {} catch {} including the streak fold and the transport enqueue. recordLintRun internally swallows readFileSync / JSON.parse / mkdirSync / writeFileSync failures (corrupt state → emptyState(); unwritable HOME → silent). The never throws when the lint result is malformed e2e test and the starts clean rather than throwing when the state file is corrupt unit test pin both boundaries. Telemetry-off short-circuits before the read, so a disabled config doesn't even touch disk (writes no state and emits nothing when telemetry is disabled).
Cost — no meaningful hit on the lint hot path
- Per-rule timing uses
performance.now()(microsecond overhead) around each rule call inrunRules. ~86 rules × N files = a few tens of thousands of clock reads. Negligible next to the 5–104ms actual lint cost the PR body cites. - Content digest is one sha256 per file (not per rule), bounded by file count.
- State file is capped at 500 entries with 14-day TTL; a full read+stringify+write of that is single-digit-ms and happens once per CLI invocation (not per request —
runCheckPipelineis per-CLI). enqueueis in-memory and non-blocking; flush is already reliability-hardened elsewhere.
Skew — measurement is meaningful
The streak semantics — a streak only advances when contentHash changed between runs — cleanly separates "agent kept trying to fix and failed" from "hyperframes check re-linting an untouched file". UNRESOLVED_REPORT_AT = 3 guarantees a rule that never clears still surfaces (an emit-only-on-clear scheme would blackhole those). The reported bit prevents the same stuck streak from firing every subsequent edit. All four properties are pinned by the lintStreaks.test.ts battery.
There's no autofix loop being measured here, so "fail to converge" == "finding persists across content changes" — well-defined, no hang risk. LINT_RULE_COUNT as a ruleset fingerprint is intentionally coarse (two same-size rulesets are indistinguishable) but the author calls this out and cli_version picks up the rest.
Nits (non-blocking)
writeFileSyncon the state file is not atomic — a Ctrl-C mid-write could truncate. Handled downstream byreadState's try/catch, so it self-heals on the next run; worth awriteFileSync(tmp) + rename(tmp, STATE_FILE)if you ever care. Not worth blocking.- Concurrent
hyperframes lintinvocations on the same HOME will last-write-wins on the streak file, dropping one run's fold. Best-effort telemetry, acceptable, but noting it in case you see mysterious "missing edit" gaps in dashboards later. slowestRulepicks the max single per-rule measurement across every file in the run — so a one-off stall on a pathological file dominates a rule that's steadily slow across many files.ruleGroupMssums compensate, which is the right hedge. Fine as-is.codes(sorted key array) is redundant withObject.keys(code_counts); the comment already explains the tradeoff (PostHog query ergonomics), keeping as-is.
CI
mergeStateStatus: BLOCKED / reviewDecision: REVIEW_REQUIRED — just missing an approval. All completed required checks green. Windows Render / Test and CI Test / Smoke:global install still in progress; nothing pointing at this PR's changes. Merge once those settle.
— Review by tai (pr-review)
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 4229e4c8. No blockers. tai covered leak / fail-safe / cost / skew plus four nits at this same head and I'm not restating any of it — below is the delta, and both findings come from executing the code rather than reading it.
Both are about the same thing: this PR and #3366 are designed to land together, and #3366 is precisely the event this telemetry cannot see.
1. slowest_rule is the only positional key in the payload, and #3366 renumbers 34 of 81 slots
runRules builds rule identity as `${group}#${index}` where index is the array position within each rule-source module. That is the one property in either event whose key is positional: codes and code_counts are stable string codes, and rule_group_ms is keyed by group name, so both survive a ruleset change intact. Good design on those three — it isolates the problem to one field.
#3366 deletes rules from four of the nine modules. Parsing both refs with the TypeScript AST and counting the exported array elements:
| group | main |
after #3366 | indices whose meaning changes |
|---|---|---|---|
| core | 15 | 13 | #11, #12 (#13, #14 cease to exist) |
| gsap | 21 | 20 | #8 … #19 |
| composition | 23 | 22 | #4 … #21 |
| fonts | 3 | 2 | #0, #1 |
| captions | 7 | 7 | none — the removed caption code lived inside the existing #2 element |
| media / adapters / textures / slideshow | 17 | 17 | none |
| total | 86 | 81 | 34 of 81 surviving slots |
Concretely: gsap#8 is scene_layer_missing_visibility_kill before and gsap_timeline_not_registered after; fonts#0 is google_fonts_import before and system_font_will_alias after.
rule_count (86 → 81) tells a dashboard that the ruleset moved, which is exactly why it's on the event — but it carries no mapping, so a query sorting by slowest_rule across that boundary silently pools two different rules under one key. Nothing goes red: the two PRs have disjoint file sets and branch off the same commit, so they merge textually clean.
Cheapest fix that keeps the "don't name all 86 rules" decision intact: emit the per-group lengths next to the totals (rule_group_counts), so a dashboard can tell whether a given group's numbering is comparable between two builds without anyone having to remember which release dropped rules.
2. cleared: true cannot distinguish "the agent fixed it" from "we deleted the rule"
The body names the cleared: true distribution as "the baseline to judge it against", so its composition is load-bearing. I lifted foldEdit / carryForward / firstSeverityPerCode verbatim and drove three scenarios:
A genuine fix: sighted -> edit(survives) -> edit(gone)
-> [{code, severity, edits:2, cleared:true}]
B rule deleted by #3366, file edited after the upgrade:
-> [{code, severity, edits:2, cleared:true}] <- byte-identical to A
C rule deleted by #3366, file NOT edited after the upgrade:
-> [] <- correctly silent
A and B are indistinguishable in the emitted payload. C is the good news and it's the digest guard doing exactly its job — an untouched re-lint after the upgrade produces nothing, because carryForward drops the vanished code without emitting. So the exposure is bounded to "first edit to a file that still carried one of the seven deleted findings".
That bound is not small at the moment these two land: the body of #3366 measures the deletions at 1740 → 507 findings over 643 registry files, with composition_self_attribute_selector alone firing 522 times and pointer_events_none on 124 of 211 blocks. So the first real cleared cohort is also the most contaminated one, and it's the cohort someone will look at first to establish the baseline.
Genuinely mitigable rather than a design flaw — every event already carries cli_version and rule_count, so the boundary is filterable. It just has to be filtered deliberately. A sentence in the dashboard notes, or discarding the first post-upgrade cleared event per file, covers it.
Nit
The comment above RULE_GROUPS says "naming all ~60 of them is a refactor this measurement does not need". The count is 86 (81 after #3366) — LINT_RULE_COUNT in the same file computes it. The reasoning is right either way; only the number is off.
CI / gate note
At the time of writing, all 8 of main's required contexts are present with 7 green and Tests on windows-latest still running (Windows render verification in progress). reviewDecision is already APPROVED, so BLOCKED here is that one job, not a review gap.
Worth recording since it's the opposite of some of our other repos: no hyperframes workflow triggers on pull_request_review — ci.yml and windows-render.yml both fire on pull_request / push only. So posting a review here cannot cancel or restart an in-flight matrix, and this review didn't.
— Review by Rames (pr-review), James's assistant
…parable Review catch on #3367: `slowest_rule` is the one positional key in either event. It is `<group>#<index>`, so adding or removing a rule renumbers every later slot in that group and the same string means different rules in two builds. #3366 does exactly that to 34 of 81 surviving slots, and `rule_count` alone says only THAT the ruleset moved, not which groups. `rule_group_counts` carries the per-group sizes alongside it, so a consumer comparing two builds can tell which groups' indices still mean the same thing without anyone having to remember which release dropped rules. `codes`, `code_counts` and `rule_group_ms` are keyed by name and were never affected. Also corrects the rule count in the RULE_GROUPS comment: 86, not ~60, as LINT_RULE_COUNT in the same file computes.
|
Thanks both. Two changes pushed in 1. 2. The 3. On the two nits I did not act on, for the record rather than to argue:
Dashboard is live. Five tiles appended to the end of HyperFrames CLI Observability, in the reading order the header describes: rules that never converge → edits to clear → which rules fire most → time per rule group → findings per run by version. Every query was executed against the warehouse before the tiles were created (the first five drafts all failed — HogQL returns properties as strings, so numerics need |
What
Two new PostHog events so lint rule changes can be argued from data instead of anecdote.
lint_report— one perhyperframes lintorhyperframes check:codes,code_countsrule_group_mscore,gsap,media, ...)slowest_rule,slowest_rule_ms<group>#<index>rule_countrule_group_countsslowest_ruleindex stays comparable across buildsduration_ms,files_scanned,error_count,warning_count,info_countlint_rule_streak— one per finding that survives an edit to its file:editsclearedWhy
The streak event is the one that matters.
A lint pass costs about 5ms (measured: 5.5ms mean, 104ms worst case over 643 registry HTML files). So per-rule CPU is not what makes the authoring loop slow. What makes it slow is a rule an agent cannot satisfy: every failed attempt costs a full edit-and-relint cycle, and a rule that never clears costs them forever.
No single lint run can see that, because the signal only exists across runs.
lint_rule_streakreconstructs it:editswithcleared: false→ a rule nobody can fixcleared: truedistribution → the baseline to judge it against ("normal findings clear in 1 edit, this one takes 6")That is the number to sort by next time we ask which rules to cut.
rule_group_msandslowest_rulestill answer the cost question, at the granularity where it is actionable.How
What counts as an iteration. A streak advances only when the file's content digest CHANGED since the last run and the finding is still there. Re-running lint on an untouched file is not an attempt. This is what stops
check— which lints on every invocation — from inflating the numbers.Rule identity is the source module plus an index within it (
gsap#7). Naming all 86 rules would make the timings prettier, but the group locates the file and the index locates the rule, and that refactor is not needed for this measurement.That index is positional, so adding or removing a rule renumbers every later slot in its group — #3366 does exactly that to 34 of 81 surviving slots.
rule_group_countsis what makes it detectable: comparing per-group sizes between two builds tells a consumer which groups' indices still mean the same thing.codes,code_countsandrule_group_msare keyed by name and were never affected. (Raised in review by Rames.)Version and agent are free.
trackEventalready attachescli_version,agent_runtime,is_ci, andinvocation_idto every event, so lint pain can be split by CLI version and by which agent produced it without adding anything here.rule_countis included on top because a rule added or removed within one version is invisible tocli_versionalone, and comparing findings-per-run across a rule change is the whole point.One call site.
trackLintRunis shared bylintandcheckso the two cannot drift, and it swallows every error — telemetry must never turn a green lint red.Privacy. Only rule codes, counts, and timings are sent. Streak state lives in
~/.hyperframes/lint-streaks.json, alongsideconfig.jsonandinstall-state.json, sorm -rf ~/.hyperframesis still a full reset. It stores digests only: no file paths, no project names, no composition source. The file key is a hash of the absolute path — enough to correlate consecutive runs on one machine, useless anywhere else. Nothing is written and nothing is emitted when telemetry is off. Entries expire after 14 days, capped at 500 files.EventPropertiesgains string arrays and numeric maps.codesandcode_countsare inherently a set and a histogram; flattening them into dynamic top-level keys would make them unqueryable. PostHog stores both natively (arrayJoin(properties.codes),JSONExtractInt).Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
lintStreaks.test.ts, 11 tests covering the streak arithmetic: first sighting emits nothing; an untouched re-lint does not advance a streak;edits_to_clearon resolution; an unresolved streak is reported once and not on every subsequent edit; a code introduced by an edit starts a fresh streak rather than counting as a survivor; per-file independence; nothing written when telemetry is off; corrupt state file recovers instead of throwing; no file paths or project names on disk; stale entries evicted.lintRun.e2e.test.ts, 3 tests going from a real project on disk throughlintProjectto the captured PostHog payload, pinning the event shape a dashboard will be built against.packages/lint: 524 tests pass.packages/cli: 2796 tests pass.Verified the emitted payload by hand against a real project.
rule_group_mscorrectly attributed 25 of 30ms togsap,slowest_ruleresolved togsap#0,rule_countto 86, and amedia_missing_data_startfinding produced{ cleared: true, edits: 1 }after the fix landed.