fix: replace fan-out attribution with single-owner additive rollup and "Unassigned" bucket for dimension rankings - #5168
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDimension rankings, cost histograms, and token histograms now use additive single-owner attribution. Missing or empty dimension ownership is grouped into an ChangesDimension rollup attribution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DimensionRankingsTabView
participant GetDimensionRankings
participant RawLogTable
DimensionRankingsTabView->>GetDimensionRankings: request dimension rankings
GetDimensionRankings->>RawLogTable: query bucketed dimension values
RawLogTable-->>GetDimensionRankings: additive bucket totals
GetDimensionRankings-->>DimensionRankingsTabView: rankings with Unassigned label
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
6b015b3 to
e85b397
Compare
e85b397 to
2b43dc5
Compare
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix: Unassigned category for dashboard c..." | Re-trigger Greptile |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/logstore/rdb.go (1)
2184-2219: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
MAX(nameCol)can be NULL for the Unassigned bucket and break the scan intoName string.
When the grouped rows have no scalar owner,nameColcan be NULL for the whole bucket, soMAX(%s)returns NULL.Nameis a plain string field, and the scan fails before the laterunassignedDimensionNamefallback runs. Wrap the aggregate withCOALESCE(..., '').Proposed fix
var nameExpr string if nameCol != "" { - nameExpr = fmt.Sprintf("MAX(%s) as name", nameCol) + nameExpr = fmt.Sprintf("COALESCE(MAX(%s), '') as name", nameCol) } else { nameExpr = "'' as name" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/rdb.go` around lines 2184 - 2219, Update the nameExpr construction in the currentResults query to wrap the MAX(nameCol) aggregate with COALESCE and default NULL values to an empty string, while preserving the existing alias and fallback for an empty nameCol.
🧹 Nitpick comments (1)
framework/logstore/rdb.go (1)
2230-2247: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTotals are only computed inside the
if bucketedbranch.
requestCounts.ActualRequests/AttributedRequestsare left at their zero value wheneverbucketedisfalse. GivenisBucketedDimensioncurrently covers every valueDimensionColumnDefcan return forRankingDimension(team/business_unit/customer/user/virtual_key), this branch appears unreachable today, but if a non-rollupRankingDimensionis ever added,TotalActualRequests/TotalAttributedRequestswould silently report0instead of an error or a real count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/rdb.go` around lines 2230 - 2247, The request totals in the ranking flow are only populated under the bucketed condition, leaving non-bucketed dimensions with zero values. Update the logic around requestCounts and the countQuery so ActualRequests and AttributedRequests are computed for every supported dimension, or explicitly reject unsupported non-bucketed dimensions instead of returning zero; preserve the existing filters and terminal status conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@framework/logstore/rdb.go`:
- Around line 2184-2219: Update the nameExpr construction in the currentResults
query to wrap the MAX(nameCol) aggregate with COALESCE and default NULL values
to an empty string, while preserving the existing alias and fallback for an
empty nameCol.
---
Nitpick comments:
In `@framework/logstore/rdb.go`:
- Around line 2230-2247: The request totals in the ranking flow are only
populated under the bucketed condition, leaving non-bucketed dimensions with
zero values. Update the logic around requestCounts and the countQuery so
ActualRequests and AttributedRequests are computed for every supported
dimension, or explicitly reject unsupported non-bucketed dimensions instead of
returning zero; preserve the existing filters and terminal status conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6be40c7-f8bc-44d9-8c19-12a6de6369eb
📒 Files selected for processing (5)
framework/logstore/multi_team_matview_test.goframework/logstore/rdb.goframework/logstore/tables.goui/app/workspace/dashboard/components/dimensionRankingsTab.tsxui/app/workspace/dashboard/components/tabViews/dimensionRankingsTabView.tsx
2b43dc5 to
aecc830
Compare
The merge-base changed after approval.
aecc830 to
7e9eaae
Compare
7e9eaae to
7e3f334
Compare
Merge activity
|
7e3f334 to
80d0ad9
Compare
…d "Unassigned" bucket for dimension rankings (#5168) ## Summary Replaces the multi-value fan-out attribution model for dimension rankings (Teams, Business Units, Customers, Users, Virtual Keys) with a single-owner additive model. Previously, a request shared across multiple teams was credited to every team it touched, causing double-counting and making per-team spend non-additive relative to the org total. Now each request is attributed to exactly one owner (the scalar ID column), and requests with no scalar owner collapse into a synthetic "Unassigned" bucket instead of being dropped or fanned out. ## Changes - Removed `teamOrBUFanoutFrom` usage from `GetDimensionRankings`, `GetDimensionCostHistogram`, and `GetDimensionTokenHistogram`. All rollup dimensions now use the raw `logs` table with a `COALESCE(NULLIF(id_col, ''), 'unassigned')` group expression. - Introduced `isBucketedDimension` to identify the five rollup dimensions (team, business unit, customer, user, virtual key) and `bucketedIDExpr` to produce the SQL expression that maps NULL/empty owners to the synthetic `unassigned` bucket. - `TotalActualRequests` and `TotalAttributedRequests` are now equal for all bucketed dimensions (single-owner attribution is additive), and both reflect the true org-wide request count including the Unassigned bucket. - Bucketed dimensions are forced onto the raw query path; the matview reader has no Unassigned bucket and is bypassed for these dimensions. - The `attributed` prop in `DimensionRankingsTabView` is now always `true` (all dimensions use the same attribution model), and the UI suppresses the raw `unassigned` ID sub-label in the rankings table. - Renamed `TestDimensionRankings_ActualVsAttributedTotals` to `TestDimensionRankings_SingleOwnerAdditive` and updated its assertions to reflect the new semantics. Added `TestDimensionRankings_VirtualKeyUnassigned` to cover the virtual key dimension. ## Type of change - [x] Refactor - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] UI (React) ## How to test ```sh # Core go test ./framework/logstore/... -run TestDimensionRankings go test ./framework/logstore/... -run TestFilterBusinessUnitMatView # UI cd ui pnpm i pnpm build ``` Validate that the Teams dashboard tab sums per-team request counts to the org total, that requests with no scalar team owner appear under "Unassigned" rather than being dropped, and that no team is credited for a request it does not scalar-own. ## Breaking changes - [x] Yes The fan-out attribution model is removed. Callers that previously relied on `TotalAttributedRequests > TotalActualRequests` to detect shared requests will now always see them equal. Per-team request counts will decrease for teams that previously received fan-out credit for shared requests. ## Related issues ## Security considerations No auth, secrets, or PII changes. The `bucketedIDExpr` function interpolates only internal constant strings, not user input. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…d "Unassigned" bucket for dimension rankings (#5168) ## Summary Replaces the multi-value fan-out attribution model for dimension rankings (Teams, Business Units, Customers, Users, Virtual Keys) with a single-owner additive model. Previously, a request shared across multiple teams was credited to every team it touched, causing double-counting and making per-team spend non-additive relative to the org total. Now each request is attributed to exactly one owner (the scalar ID column), and requests with no scalar owner collapse into a synthetic "Unassigned" bucket instead of being dropped or fanned out. ## Changes - Removed `teamOrBUFanoutFrom` usage from `GetDimensionRankings`, `GetDimensionCostHistogram`, and `GetDimensionTokenHistogram`. All rollup dimensions now use the raw `logs` table with a `COALESCE(NULLIF(id_col, ''), 'unassigned')` group expression. - Introduced `isBucketedDimension` to identify the five rollup dimensions (team, business unit, customer, user, virtual key) and `bucketedIDExpr` to produce the SQL expression that maps NULL/empty owners to the synthetic `unassigned` bucket. - `TotalActualRequests` and `TotalAttributedRequests` are now equal for all bucketed dimensions (single-owner attribution is additive), and both reflect the true org-wide request count including the Unassigned bucket. - Bucketed dimensions are forced onto the raw query path; the matview reader has no Unassigned bucket and is bypassed for these dimensions. - The `attributed` prop in `DimensionRankingsTabView` is now always `true` (all dimensions use the same attribution model), and the UI suppresses the raw `unassigned` ID sub-label in the rankings table. - Renamed `TestDimensionRankings_ActualVsAttributedTotals` to `TestDimensionRankings_SingleOwnerAdditive` and updated its assertions to reflect the new semantics. Added `TestDimensionRankings_VirtualKeyUnassigned` to cover the virtual key dimension. ## Type of change - [x] Refactor - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] UI (React) ## How to test ```sh # Core go test ./framework/logstore/... -run TestDimensionRankings go test ./framework/logstore/... -run TestFilterBusinessUnitMatView # UI cd ui pnpm i pnpm build ``` Validate that the Teams dashboard tab sums per-team request counts to the org total, that requests with no scalar team owner appear under "Unassigned" rather than being dropped, and that no team is credited for a request it does not scalar-own. ## Breaking changes - [x] Yes The fan-out attribution model is removed. Callers that previously relied on `TotalAttributedRequests > TotalActualRequests` to detect shared requests will now always see them equal. Per-team request counts will decrease for teams that previously received fan-out credit for shared requests. ## Related issues ## Security considerations No auth, secrets, or PII changes. The `bucketedIDExpr` function interpolates only internal constant strings, not user input. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…d "Unassigned" bucket for dimension rankings (maximhq#5168) ## Summary Replaces the multi-value fan-out attribution model for dimension rankings (Teams, Business Units, Customers, Users, Virtual Keys) with a single-owner additive model. Previously, a request shared across multiple teams was credited to every team it touched, causing double-counting and making per-team spend non-additive relative to the org total. Now each request is attributed to exactly one owner (the scalar ID column), and requests with no scalar owner collapse into a synthetic "Unassigned" bucket instead of being dropped or fanned out. ## Changes - Removed `teamOrBUFanoutFrom` usage from `GetDimensionRankings`, `GetDimensionCostHistogram`, and `GetDimensionTokenHistogram`. All rollup dimensions now use the raw `logs` table with a `COALESCE(NULLIF(id_col, ''), 'unassigned')` group expression. - Introduced `isBucketedDimension` to identify the five rollup dimensions (team, business unit, customer, user, virtual key) and `bucketedIDExpr` to produce the SQL expression that maps NULL/empty owners to the synthetic `unassigned` bucket. - `TotalActualRequests` and `TotalAttributedRequests` are now equal for all bucketed dimensions (single-owner attribution is additive), and both reflect the true org-wide request count including the Unassigned bucket. - Bucketed dimensions are forced onto the raw query path; the matview reader has no Unassigned bucket and is bypassed for these dimensions. - The `attributed` prop in `DimensionRankingsTabView` is now always `true` (all dimensions use the same attribution model), and the UI suppresses the raw `unassigned` ID sub-label in the rankings table. - Renamed `TestDimensionRankings_ActualVsAttributedTotals` to `TestDimensionRankings_SingleOwnerAdditive` and updated its assertions to reflect the new semantics. Added `TestDimensionRankings_VirtualKeyUnassigned` to cover the virtual key dimension. ## Type of change - [x] Refactor - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] UI (React) ## How to test ```sh # Core go test ./framework/logstore/... -run TestDimensionRankings go test ./framework/logstore/... -run TestFilterBusinessUnitMatView # UI cd ui pnpm i pnpm build ``` Validate that the Teams dashboard tab sums per-team request counts to the org total, that requests with no scalar team owner appear under "Unassigned" rather than being dropped, and that no team is credited for a request it does not scalar-own. ## Breaking changes - [x] Yes The fan-out attribution model is removed. Callers that previously relied on `TotalAttributedRequests > TotalActualRequests` to detect shared requests will now always see them equal. Per-team request counts will decrease for teams that previously received fan-out credit for shared requests. ## Related issues ## Security considerations No auth, secrets, or PII changes. The `bucketedIDExpr` function interpolates only internal constant strings, not user input. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…d "Unassigned" bucket for dimension rankings (maximhq#5168) ## Summary Replaces the multi-value fan-out attribution model for dimension rankings (Teams, Business Units, Customers, Users, Virtual Keys) with a single-owner additive model. Previously, a request shared across multiple teams was credited to every team it touched, causing double-counting and making per-team spend non-additive relative to the org total. Now each request is attributed to exactly one owner (the scalar ID column), and requests with no scalar owner collapse into a synthetic "Unassigned" bucket instead of being dropped or fanned out. ## Changes - Removed `teamOrBUFanoutFrom` usage from `GetDimensionRankings`, `GetDimensionCostHistogram`, and `GetDimensionTokenHistogram`. All rollup dimensions now use the raw `logs` table with a `COALESCE(NULLIF(id_col, ''), 'unassigned')` group expression. - Introduced `isBucketedDimension` to identify the five rollup dimensions (team, business unit, customer, user, virtual key) and `bucketedIDExpr` to produce the SQL expression that maps NULL/empty owners to the synthetic `unassigned` bucket. - `TotalActualRequests` and `TotalAttributedRequests` are now equal for all bucketed dimensions (single-owner attribution is additive), and both reflect the true org-wide request count including the Unassigned bucket. - Bucketed dimensions are forced onto the raw query path; the matview reader has no Unassigned bucket and is bypassed for these dimensions. - The `attributed` prop in `DimensionRankingsTabView` is now always `true` (all dimensions use the same attribution model), and the UI suppresses the raw `unassigned` ID sub-label in the rankings table. - Renamed `TestDimensionRankings_ActualVsAttributedTotals` to `TestDimensionRankings_SingleOwnerAdditive` and updated its assertions to reflect the new semantics. Added `TestDimensionRankings_VirtualKeyUnassigned` to cover the virtual key dimension. ## Type of change - [x] Refactor - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] UI (React) ## How to test ```sh # Core go test ./framework/logstore/... -run TestDimensionRankings go test ./framework/logstore/... -run TestFilterBusinessUnitMatView # UI cd ui pnpm i pnpm build ``` Validate that the Teams dashboard tab sums per-team request counts to the org total, that requests with no scalar team owner appear under "Unassigned" rather than being dropped, and that no team is credited for a request it does not scalar-own. ## Breaking changes - [x] Yes The fan-out attribution model is removed. Callers that previously relied on `TotalAttributedRequests > TotalActualRequests` to detect shared requests will now always see them equal. Per-team request counts will decrease for teams that previously received fan-out credit for shared requests. ## Related issues ## Security considerations No auth, secrets, or PII changes. The `bucketedIDExpr` function interpolates only internal constant strings, not user input. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Replaces the multi-value fan-out attribution model for dimension rankings (Teams, Business Units, Customers, Users, Virtual Keys) with a single-owner additive model. Previously, a request shared across multiple teams was credited to every team it touched, causing double-counting and making per-team spend non-additive relative to the org total. Now each request is attributed to exactly one owner (the scalar ID column), and requests with no scalar owner collapse into a synthetic "Unassigned" bucket instead of being dropped or fanned out.
Changes
teamOrBUFanoutFromusage fromGetDimensionRankings,GetDimensionCostHistogram, andGetDimensionTokenHistogram. All rollup dimensions now use the rawlogstable with aCOALESCE(NULLIF(id_col, ''), 'unassigned')group expression.isBucketedDimensionto identify the five rollup dimensions (team, business unit, customer, user, virtual key) andbucketedIDExprto produce the SQL expression that maps NULL/empty owners to the syntheticunassignedbucket.TotalActualRequestsandTotalAttributedRequestsare now equal for all bucketed dimensions (single-owner attribution is additive), and both reflect the true org-wide request count including the Unassigned bucket.attributedprop inDimensionRankingsTabViewis now alwaystrue(all dimensions use the same attribution model), and the UI suppresses the rawunassignedID sub-label in the rankings table.TestDimensionRankings_ActualVsAttributedTotalstoTestDimensionRankings_SingleOwnerAdditiveand updated its assertions to reflect the new semantics. AddedTestDimensionRankings_VirtualKeyUnassignedto cover the virtual key dimension.Type of change
Affected areas
How to test
Validate that the Teams dashboard tab sums per-team request counts to the org total, that requests with no scalar team owner appear under "Unassigned" rather than being dropped, and that no team is credited for a request it does not scalar-own.
Breaking changes
The fan-out attribution model is removed. Callers that previously relied on
TotalAttributedRequests > TotalActualRequeststo detect shared requests will now always see them equal. Per-team request counts will decrease for teams that previously received fan-out credit for shared requests.Related issues
Security considerations
No auth, secrets, or PII changes. The
bucketedIDExprfunction interpolates only internal constant strings, not user input.Checklist
docs/contributing/README.mdand followed the guidelines