fix(sales-dashboard): Correct understated API usage on the organisation chart - #8468
Conversation
…on chart The sales dashboard read API usage through its own Influx entry point, `get_event_list_for_organisation`, which was the only usage reader without a `group()` before `aggregateWindow`. Influx aggregates per series, so each label combination produced its own daily records, and the Python then appended them into one flat list per resource rather than summing them per day. Chart.js plotted the leading slice of that list, so a single client application's traffic was shown as the whole organisation's usage, with the x-axis labels taken from the first series only. Delete that function and have the sales dashboard call `get_usage_data`, the same entry point the customer-facing app uses, reshaping the result for Chart.js in `map_usage_data_to_daily_totals`. Both charts now share one query, so the grouping cannot go missing in one caller and not the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change replaces organisation event-list retrieval with Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The dashboard now shows daily organisation-wide usage totals rather than a single label slice. The aggregation and rendering paths have coverage for the intended behavior, with no current merge-blocking risk identified. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8468 +/- ##
========================================
Coverage 98.81% 98.82%
========================================
Files 1621 1622 +1
Lines 66299 66691 +392
========================================
+ Hits 65514 65906 +392
Misses 785 785 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20308 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ oss · depot-ubuntu-latest-arm-16 — run #20308 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20308 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20308 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20189 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20189 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20189 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20189 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
I'd appreciate a cleaner diff, but can't find a reason to block.
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7976
The sales dashboard read usage through its own Influx entry point,
get_event_list_for_organisation, the only usage reader without agroup()beforeaggregateWindow. Influx aggregates per series, so each label combination produced its own daily records, which were then appended into a flat list per resource instead of summed per day. Chart.js plotted the leading slice of that list, so one client application's traffic was shown as the whole organisation's usage.That function is deleted. The sales dashboard now calls
get_usage_data— the entry point the customer-facing app already uses — and reshapes the result for Chart.js inmap_usage_data_to_daily_totals. One shared query means the grouping cannot go missing in one caller and not the other.Worth noting for #7976: the API usage alert emails were correct all along. The affected organisation genuinely exceeded its plan allowance; this chart is what led us to believe otherwise.
How did you test this code?
Unit tests cover the mapper against multiple label rows per day, unordered days, and empty input, plus the sales dashboard view asserting both client applications' usage appears rather than one.
Verified against production data for the affected organisation. Summing every series over its billing sub-period returned 5,030,444 API calls from both the 15m and 1h downsampled buckets, and matched the customer-facing dashboard, while the sales dashboard chart displayed roughly 1/76th of that.
Also ran
make lint,make typecheck(baseline unchanged, no errors in the touched files) and the three affected test files (54 passed).🤖 Generated with Claude Code