feat: Keep host on aggregated API usage buckets - #8471
gagantrivedi wants to merge 8 commits into
Conversation
host was written to APIUsageRaw and then dropped by the hourly rollup, so it only survived the raw-data retention window. Buckets now carry it: the rollup groups by host and the overlap check treats it as a dimension. Read paths are unchanged — aggregations still sum across hosts — so dashboards behave exactly as before while the data stays available.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe API usage bucket model and migration add a Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes 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 #8471 +/- ##
========================================
Coverage 98.81% 98.82%
========================================
Files 1621 1632 +11
Lines 66299 66700 +401
========================================
+ Hits 65514 65915 +401
Misses 785 785 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20528 (attempt 2)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results❌ private-cloud · depot-ubuntu-latest-arm-16 — run #20528 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise ✅ private-cloud · depot-ubuntu-latest-16 — run #20528 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20528 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20528 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20527 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20527 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20527 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20527 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20526 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20526 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20526 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: ccf868b1-727f-4ea5-a929-b42caf2afa63
📒 Files selected for processing (4)
api/app_analytics/migrations/0009_apiusagebucket_host.pyapi/app_analytics/models.pyapi/app_analytics/tasks.pyapi/tests/unit/app_analytics/test_tasks.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Visual Regression19 screenshots compared. See report for details. |
…computed Buckets written before host existed hold every host's count under "". Recomputing such a window from raw data now removes that row before writing the per-host rows, instead of adding to it and double counting. Also covers host grouping on the bucket-to-bucket path.
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: d0e764ab-4c02-4955-8026-e83b172f8c0d
📒 Files selected for processing (2)
api/app_analytics/tasks.pyapi/tests/unit/app_analytics/test_tasks.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
github-actions
Bot
added
feature
A raw group with host "" processed before a real host would have had its freshly written bucket deleted as legacy.
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ad885eef-d3e0-446d-923c-d2ab71903f2c
📒 Files selected for processing (2)
api/app_analytics/tasks.pyapi/tests/unit/app_analytics/test_tasks.py
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
|
|
||
| class APIUsageBucket(AbstractBucket): | ||
| resource = models.IntegerField(choices=Resource.choices) | ||
| host = models.CharField(max_length=255, default="") |
There was a problem hiding this comment.
Should we make this field nullable to make the migration easier?
There was a problem hiding this comment.
That used to be the case up to Postgres 11. For Postgres 11 and later, adding a column with a default value that's the same for every row only changes the table metadata
| # Buckets created before the `host` column existed hold the | ||
| # window's whole count under host "". Raw data recomputes that | ||
| # window per host with the same total, so the old row is removed | ||
| # rather than left to double the window. Removed before any upsert, | ||
| # so a row this pass writes under host "" is never taken for old. |
There was a problem hiding this comment.
This reads as slop to me... Can we rewrite it for human consumption? It also feels like we're handling something in runtime code that maybe would be better handled in a migration (if that's possible)?
Thinking about it, I guess the main thing that's missing here for me, is why old buckets need to be affected at all. As I understood, once data was bucketed, it shouldn't need to be touched again?
There was a problem hiding this comment.
okay, this is a bit complicated okay so the thing is since our bucketing task runs every 60 mins and we create buckets for every 15 mins now for a normal case each run rebuilds the last four completed 15 min buckets, so the next run recomputes most of the buckets the previous run already wrote and just overwrites them the problem that we are facing now with host is that since we are bucketing on host as well now those buckets will be counted twice(once for the actual host and once for host="") if we did not have this janky code now I dont know if that's acceptable or not I lean on the side of not so now I have moved it to migration that will delete the buckets of the last two hours so that we dont count them twice it can loose some data but it will not count it twice and it's out of the code path let me know if that makes any sense
There was a problem hiding this comment.
I guess the main thing that's missing here for me, is why old buckets need to be affected at all. As I understood, once data was bucketed, it shouldn't need to be touched again?
In principle yes, but the task is deliberately stateless: it doesn't remember what it processed, it just rebuilds the last hour of completed 15 min buckets whenever it runs. That means a late or failed run heals itself, and raw rows that land a bit late get picked up on the next rebuild.
| ) -> None: | ||
| # Given a window already bucketed before `host` existed, and its raw data | ||
| environment_id = 1 | ||
| when = timezone.now() - timedelta(minutes=90) |
There was a problem hiding this comment.
Other than now, I much prefer being explicit with vars like this.
| when = timezone.now() - timedelta(minutes=90) | |
| ninety_minutes_ago = timezone.now() - timedelta(minutes=90) |
…t at runtime Buckets written before the host column hold a window's whole count under host "". The rollup recomputes the trailing hour on every run, so a recomputed window would gain per-host rows next to the old one and count twice. Only rows within two hours of the migration can ever be recomputed, so the migration deletes those; the next run rebuilds them from raw data. Django drops the column default after AddField, so an old task processor still running cannot insert new host-less rows.
github-actions
Bot
added
feature
github-actions
Bot
added
feature
github-actions
Bot
added
feature
2 participants
Changes
Contributes to Flagsmith/flagsmith-private#256
hostis written toAPIUsageRawon every tracked request, but the hourly rollup dropped it — aggregated buckets carried no host, so the dimension only survived the raw-data retention window (30 days). With the Edge Proxy reporting its served requests withhost="edge-proxy", that dimension is how proxy-served traffic stays distinguishable from direct traffic long-term.APIUsageBucketgains ahostcolumn (default""for existing rows)._get_api_usage_source_data) groups byhoston both the raw→bucket and bucket→bucket paths, andpopulate_api_usage_bucketwrites it.hostas a dimension alongsideresource.FeatureEvaluationBucketis untouched (no host on its raw rows).How did you test this code?
New test: raw events from two hosts in one bucket window roll up into separate buckets, each keeping its host. Full
test_tasks.py(20) and the bucket read-path tests (test_analytics_db_service.py,test_commands.py, 29) pass against Postgres; ruff + mypy clean.