Fix 8 RUM data-quality defects found validating against live data (0.1.17) - #6
Merged
Merged
Conversation
…an names Two wire-contract additions the fixes that follow build on. app_unclean_exit exists because "the session ended without a clean shutdown signal" is not "the app crashed" — pagehide does not fire on force-quit, OS shutdown, tab discard or task-switcher eviction, so its absence is not evidence of a crash. It stays out of ERROR_CLASS_SPANS so it neither bypasses sampling as an error nor lands in the crash tables. user_frustration exists because the original user_interaction span is already emitted and ended by the time frustration detection completes, so annotating it in place is not reachable, and re-emitting under the same name double-counted. Also adds the crash.service.name / crash.service.version / crash.environment keys. Resource attributes are frozen at provider construction and there is no per-span override, so a deferred marker cannot restate where it came from except as span attributes.
The detector is a Web Worker watchdog: the main thread posts a beat every 1000ms and the worker reports how late it arrives. Workers are not throttled but a hidden tab's timers are clamped to roughly once a minute, so the worker faithfully measured a ~59s lateness that was throttling, not a hang. One idle tab produced 11 anr spans in 11 minutes with inter-arrival gaps spread across 16ms — a metronome, which no real workload-dependent stall could be. The main thread now stops beating while hidden and resets the worker's baseline before it resumes. The reset matters on its own: without it the first beat back charges the whole background interval, which is how a "resumed" lifecycle event was followed 16ms later by a 35s anr. Spans also carry anr.visibility_state; the SDK recorded no visibility signal at all, so it could not distinguish a hidden tab from a hung one. The span is no longer a zero-duration marker either. The hang length lived only in a bespoke string attribute, so the trace waterfall, p95 duration panels and anything else generic over spans read 0 — which at one point suggested the ANR data was empty when it was not. Finally, anr.duration / anr.threshold carried SECONDS under names that gave no unit, while the SDK's own option is anrThresholdMs. That mismatch already produced a 1000x display bug downstream. They become anr.duration_ms / anr.threshold_ms. Renaming rather than redefining in place is deliberate: the same key silently changing units by 1000x is exactly the failure that was paid for once already, and distinct keys let a single query serve old and new rows. The test suite is new; it executes the real worker source rather than a copy, so the reset and threshold logic are covered rather than reimplemented.
…shes The deferred crash marker lived under one unscoped localStorage key, and a single origin can serve many tenants under different paths. A tab that died in one tenant was therefore filed against whichever tenant the browser opened next, carrying that tenant's URLs and screen names into another's partition. The key is now scoped by service name and environment, which stops the cross-read at source rather than repairing the attribution afterwards. A legacy marker cannot be attributed to any tenant, so it is discarded rather than guessed at — guessing is what produced the bug. The marker also records the originating service name, version and environment and reports them as crash.* attributes, since the resource cannot restate them. The span's screen.name is now the dead session's rather than the detecting page's, which previously sat next to crash.last_screen describing a different page. These markers are no longer app_crash. Writing them as app_crash meant routine tab closes fed the crash counters and depressed crash-free rate. They are emitted as app_unclean_exit with forceSample, since they report on a previous session and the current session's sample decision has nothing to say about them.
Two defects that compounded rather than merely added. web.vital.id (unique per measurement), web.vital.value (already the histogram's sum) and web.vital.target_selector (a ~250-character CSS chain) were metric ATTRIBUTES, so every measurement became its own time series. Meanwhile the histograms were cumulative with a fixed start time, so FCP, LCP and TTFB — which fire once per page load — had their same count=1 point re-exported every 30 seconds for the life of the page: roughly 180 rows for 3 real measurements in a half-hour session. They compound because the metrics table sorts on attributes before time. Repeats alone would compress well; wide attributes alone would be sparse. Together the repeats can never co-locate, because every repeat carries a unique id. The compression that would have absorbed the duplication is what the cardinality destroys. emitHistogram and emitGauge also folded the whole commonAttributes() bag into every point, which put user.id — routinely an email address — into metric dimensions. Those are retained longer, rolled up harder and far more expensive to delete selectively than spans. metricAttributes() replaces it with a bounded set. session.id stays: the screen web-vitals dashboards filter on it. Everything dropped from metrics is unchanged on spans and logs, where correlation belongs. Instruments are cached rather than rebuilt on every record, matching how the view counters already work. The metric side of web vitals had no test at all — recorder.metrics() was never called anywhere in the suite.
A dead or rage click emitted a SECOND user_interaction span rather than a
distinct event, so view.action.count counted both. In one sampled session
view.action.count was 20 against view.frustration.count 3 — roughly 15% of the
reported actions were duplicates, skewing engagement metrics by exactly the
users having the worst experience. The twin also omitted user_interaction.id,
target and target.type, so it could not even be deduplicated.
Frustrations now emit user_frustration and carry the originating interaction's
id, target and target type, handed over by a one-slot registry the tap tracker
writes and the frustration tracker reads. Annotating the original span was not
reachable: it is emitted and ended synchronously, and holding every interaction
span open for the 600ms dead-click window would delay export for all of them.
Two more spurious-frustration bugs found while testing this. The 120ms rage
timer and the 600ms dead-click timer were independent, so one gesture could
produce three spans; a rage episode now claims the clicks that made it up and
reports once. And the "last error seen" sentinel was 0, which performance.now()
is also close to just after load — so every click in the first 100ms of a page
was classified as an error_click.
Separately, third-party request URLs are now sanitized by default. Analytics
beacons encode the current page URL in their query string, so a captured collect
call carried an entire dashboard URL — template variable values, and with them a
tenant's Kubernetes pod name — to the third party and then into stored traces.
firstPartyHosts only ever drove traceparent injection and filtered nothing.
thirdPartyResources ('sanitized' | 'off' | 'full') now governs how much of a
non-first-party URL is recorded. Same-origin is always first party, whatever
firstPartyHosts says, so an app that never configured it is unaffected.
Also adds anr.ts, crash.ts, frustration.ts, web-vitals.ts and the new interaction-registry.ts to the coverage include list. All of them were outside it, which is part of why these defects shipped.
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.
Closes B14-1858, B14-1859, B14-1860, B14-1861, B14-1862, B14-1863, B14-1864, B14-1865.
All eight came out of one exercise: validating logX self-RUM against live
nbg1-axidata on 2026-09-01. Every one is a defect in this SDK, not in the ClickHouse MVs or the Grafana plugins — the MVs faithfully store what we send, and we were sending wrong things.Between them they caused phantom hangs once a minute per backgrounded tab, normal tab closes counted as crashes (which does depress crash-free rate), one tenant's crashes filed against another tenant, unbounded metric cardinality in our own ClickHouse across all 12 nbg1 tenants, a customer email address in metric dimensions, ~15% inflation of interaction counts, and tenant Kubernetes pod names leaking to Google Analytics and back into
otel_traces.What changed
anrspans carry a real duration;anr.duration_ms/anr.threshold_msreplace the seconds-valued keysapp_unclean_exit, notapp_crashweb.vital.name/ratingonly; histograms exported as deltauser.idno longer written into metric dimensionsuser_frustration, correlated back to the interactionthirdPartyResources)Found while fixing these
Three defects in the same family that weren't on any ticket:
error_click. The "last error seen" sentinel was0, andperformance.now()is also near 0 then.anr.ts,crash.ts,frustration.tsandweb-vitals.tswere all outside the coverage include list — part of why this shipped. Added; thresholds still pass.Breaking
Four wire-contract changes, all documented in the CHANGELOG:
anr.duration/anr.threshold→anr.duration_ms/anr.threshold_ms. Renamed rather than redefined, so old and new rows stay distinguishable — the same key silently changing units by 1000x is exactly what PR base14#1704 already paid for. Readcoalesce(anr.duration_ms, anr.duration * 1000)while both exist.app_unclean_exit, notapp_crash. Crash counters need no change; anything that wants to see unclean exits must add the name.user_frustration, notuser_interaction. Aggregations overuser_interactionbecome correct automatically.setRuntimeAttribute()/setSessionAttributes()values and alluser.*no longer appear on metrics; unchanged on spans and logs.Requires the coordinated plugin change in base14 for (1) — merge that first or together, or ANR panels render 1000x wrong for new data.
Verification
make cigreen: fmt-check, lint, typecheck, 294 tests, build, export lint. Test count 270 → 294, including two new suites (anr.tsandfrustration.tspreviously had none). The ANR suite executes the real worker source rather than a reimplementation of it. Every one of the six commits typechecks and passes on its own.Not verified: jsdom cannot reproduce browser timer throttling, so the ANR fix is proven at the contract level (no beats while hidden, reset before resume) but not against a real backgrounded tab. Worth backgrounding a tab for 5+ minutes against a local collector before release and confirming zero
anrspans.Deliberately out of scope
Follow-up tickets rather than scope creep:
ui_hang.durationhas the same seconds-vs-Ms-config inconsistency; nativenetwork.tshas noshouldSkipat all and a weaker first-party matcher;app_unclean_exitanduser_frustrationneed a home in the per-tenantrum_tables_mv.sqland plugin renderers before they're visible anywhere; and whether logX should send dashboard URLs to Google Analytics at all is a product decision.