fix: gate ranking readers on fresh-aggregate matview window - #4355
Conversation
|
|
|
Warning Review limit reached
More reviews will be available in 1 hour, 40 minutes, and 54 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates the matview eligibility check for ranking functions in the logstore to use the fresh-aggregate rule, ensuring consistency with API logs endpoints. Documentation is clarified, and three ranking functions adopt the same 24-hour minimum-window policy to prevent hourly-rounded matview data from distorting short-window rankings. ChangesFresh-aggregate matview eligibility for ranking functions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Confidence Score: 5/5Safe to merge — the change is a one-line gate swap in each of three functions, all existing matview eligibility checks are still enforced, and the new gate is already battle-tested on the stats and pagination paths. The replacement function is a strict superset of the old gate (it calls canUseMatView internally and adds a window-size check on top), so the matview path can only become less frequently taken — never incorrectly enabled. The fanoutFrom guard in GetDimensionRankings is preserved. The doc-comment update is accurate. No logic paths were added or removed. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix: gate ranking readers on fresh-aggre..." | Re-trigger Greptile |
Model/user/dimension rankings used bare canUseMatView, so short dashboard windows (1h/6h) were served from mv_logs_hourly, which rounds the window out to full hour buckets and inflates ranking costs/requests by up to 2x against the raw-path stats and cost-histogram totals on the same page. Rankings now use canUseMatViewForFreshAggregate, same as GetStats and the pagination count: windows under 24h go to the raw logs table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
877a9e8 to
97cdca5
Compare
Merge activity
|
## Summary Ranking readers (`GetModelRankings`, `GetUserRankings`, `GetDimensionRankings`) were using `canUseMatView` to decide whether to query `mv_logs_hourly`. For short time windows, `mv_logs_hourly` rounds the window out to full hour buckets, which inflates ranking totals relative to the raw-path stats and cost-histogram numbers shown on the same dashboard. This PR switches all three ranking readers to use `canUseMatViewForFreshAggregate` — the same gate already used by `/api/logs/stats` and `/api/logs` — so that short windows consistently fall back to the raw table across all surfaces. ## Changes - `GetModelRankings`, `GetUserRankings`, and `GetDimensionRankings` now call `canUseMatViewForFreshAggregate` instead of `canUseMatView`, ensuring short windows bypass the materialized view. - The `canUseMatViewForFreshAggregate` doc comment is updated to reflect that ranking readers are now also governed by this gate. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the log store tests and verify that ranking queries for short time windows (less than 24 hours) route to the raw table rather than the materialized view. ```sh go test ./framework/logstore/... ``` Manually verify on a dashboard with a short time window (e.g. last 1 hour) that model/user/dimension ranking totals match the stats and cost-histogram totals shown alongside them. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified time window handling requirements for ranking queries to ensure consistent metrics. * **Refactor** * Updated ranking query optimization logic to improve consistency across different time windows and data sources. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…4355) ## Summary Ranking readers (`GetModelRankings`, `GetUserRankings`, `GetDimensionRankings`) were using `canUseMatView` to decide whether to query `mv_logs_hourly`. For short time windows, `mv_logs_hourly` rounds the window out to full hour buckets, which inflates ranking totals relative to the raw-path stats and cost-histogram numbers shown on the same dashboard. This PR switches all three ranking readers to use `canUseMatViewForFreshAggregate` — the same gate already used by `/api/logs/stats` and `/api/logs` — so that short windows consistently fall back to the raw table across all surfaces. ## Changes - `GetModelRankings`, `GetUserRankings`, and `GetDimensionRankings` now call `canUseMatViewForFreshAggregate` instead of `canUseMatView`, ensuring short windows bypass the materialized view. - The `canUseMatViewForFreshAggregate` doc comment is updated to reflect that ranking readers are now also governed by this gate. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the log store tests and verify that ranking queries for short time windows (less than 24 hours) route to the raw table rather than the materialized view. ```sh go test ./framework/logstore/... ``` Manually verify on a dashboard with a short time window (e.g. last 1 hour) that model/user/dimension ranking totals match the stats and cost-histogram totals shown alongside them. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified time window handling requirements for ranking queries to ensure consistent metrics. * **Refactor** * Updated ranking query optimization logic to improve consistency across different time windows and data sources. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…4355) ## Summary Ranking readers (`GetModelRankings`, `GetUserRankings`, `GetDimensionRankings`) were using `canUseMatView` to decide whether to query `mv_logs_hourly`. For short time windows, `mv_logs_hourly` rounds the window out to full hour buckets, which inflates ranking totals relative to the raw-path stats and cost-histogram numbers shown on the same dashboard. This PR switches all three ranking readers to use `canUseMatViewForFreshAggregate` — the same gate already used by `/api/logs/stats` and `/api/logs` — so that short windows consistently fall back to the raw table across all surfaces. ## Changes - `GetModelRankings`, `GetUserRankings`, and `GetDimensionRankings` now call `canUseMatViewForFreshAggregate` instead of `canUseMatView`, ensuring short windows bypass the materialized view. - The `canUseMatViewForFreshAggregate` doc comment is updated to reflect that ranking readers are now also governed by this gate. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the log store tests and verify that ranking queries for short time windows (less than 24 hours) route to the raw table rather than the materialized view. ```sh go test ./framework/logstore/... ``` Manually verify on a dashboard with a short time window (e.g. last 1 hour) that model/user/dimension ranking totals match the stats and cost-histogram totals shown alongside them. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified time window handling requirements for ranking queries to ensure consistent metrics. * **Refactor** * Updated ranking query optimization logic to improve consistency across different time windows and data sources. <!-- end of auto-generated comment: release notes by coderabbit.ai -->

Summary
Ranking readers (
GetModelRankings,GetUserRankings,GetDimensionRankings) were usingcanUseMatViewto decide whether to querymv_logs_hourly. For short time windows,mv_logs_hourlyrounds the window out to full hour buckets, which inflates ranking totals relative to the raw-path stats and cost-histogram numbers shown on the same dashboard. This PR switches all three ranking readers to usecanUseMatViewForFreshAggregate— the same gate already used by/api/logs/statsand/api/logs— so that short windows consistently fall back to the raw table across all surfaces.Changes
GetModelRankings,GetUserRankings, andGetDimensionRankingsnow callcanUseMatViewForFreshAggregateinstead ofcanUseMatView, ensuring short windows bypass the materialized view.canUseMatViewForFreshAggregatedoc comment is updated to reflect that ranking readers are now also governed by this gate.Type of change
Affected areas
How to test
Run the log store tests and verify that ranking queries for short time windows (less than 24 hours) route to the raw table rather than the materialized view.
go test ./framework/logstore/...Manually verify on a dashboard with a short time window (e.g. last 1 hour) that model/user/dimension ranking totals match the stats and cost-histogram totals shown alongside them.
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Documentation
Refactor