ExperimentsOverview: drop Cumulative hint from goodput queries - #88
Conversation
proxy.session.goodput.* is delta temporality as of getlantern/engineering#3831, which switched the emitters so the ops collector could aggregate away route.id/instance.id/host.name (the histogram had reached ~55M series and was timing out SigNoz queries past ~1w). The mean-goodput math is unaffected: rate(sum)/rate(count) cancels the rate window either way. But naming the wrong temporality is not a no-op — v4 returns no series at all for rate() over a stream whose temporality does not match the hint, so the cards would go blank once the emitters roll. Delta is the API default, so the hint is simply dropped. Refs getlantern/engineering#3831
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes classify dashboard metrics as delta streams and remove the explicit cumulative temporality from the goodput query. The query now relies on the API default temporality for v4 series retrieval. ChangesMetrics temporality
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized query change removes an outdated temporality hint so goodput metrics continue to populate after emitters switch to delta temporality. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the experiments dashboard’s SigNoz builder queries for proxy.session.goodput.* to stop forcing "Cumulative" temporality, aligning the UI with the planned switch of the goodput histogram streams to delta temporality (per getlantern/engineering#3831) so the charts don’t go blank once emitters roll.
Changes:
- Removed the
temporality: "Cumulative"hint from the goodput sum/count queries used to compute mean per-session goodput. - Updated inline documentation to clarify when a temporality hint is needed and why it must match the underlying stream temporality.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/components/ExperimentsOverview.tsx |
Drops the "Cumulative" temporality hint from goodput queries and updates comments explaining why. |
src/api/client.ts |
Refines documentation for buildExperimentTrackQuery’s optional temporality and notes goodput now being delta per #3831. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
What
Drop the
temporality: "Cumulative"hint from theproxy.session.goodput.*queries.Part of getlantern/engineering#3831. Companion PRs: getlantern/lantern-cloud#3194, getlantern/http-proxy#683, getlantern/lantern-box#302.
Why
The goodput histogram becomes delta temporality in that work, so the ops collector can aggregate away
route.id/instance.id/host.name— the histogram had reached ~55M unique series (host.namealone has 10,854 distinct values on it) and was timing out every SigNoz query past a ~1 week window.The mean-goodput math here is unaffected either way:
avgGoodputByTrackcomputesrate(sum)/rate(count), and the rate window cancels in the ratio.But the hint itself is not a no-op. v4 returns no series at all for
rate()over a stream whose temporality does not match the hint — which is why it was added in the first place. Left in place, the goodput cards would silently go blank once the emitters roll. Delta is the API default, so the hint is simply removed.Testing
npm run build(tsc -b + vite) passes clean.Ordering
Safe to merge alongside the emitter PRs. Before they roll the hint is stale-but-harmless in this direction only if the emitters are already delta, so land this with them rather than well ahead.
/cc @jay-418 @Crosse
Summary by CodeRabbit