fix(analytics): nested SUM-then-AVG rollup in breakdowns and monthly view - #1209
Merged
Conversation
Contributor
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAnalytics provider, service, and monthly summary queries now perform per-timestamp nested rollups before averaging. Migrations, SQL mocks, and integration tests validate the corrected aggregation and migration rollback behavior. ChangesAnalytics Nested Rollup Semantics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AnalyticsStore
participant AnalyticsQueries
participant savings_snapshots
participant monthly_savings_summary
AnalyticsStore->>AnalyticsQueries: Request analytics breakdown
AnalyticsQueries->>savings_snapshots: Aggregate rows by dimensions and timestamp
savings_snapshots-->>AnalyticsQueries: Per-timestamp totals and averages
AnalyticsQueries-->>AnalyticsStore: Average timestamp-level results
AnalyticsStore->>monthly_savings_summary: Refresh monthly summary
monthly_savings_summary->>savings_snapshots: Build nested monthly rollup
savings_snapshots-->>monthly_savings_summary: Monthly aggregates and snapshot count
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
force-pushed
the
fix/cor-02-analytics-nested-rollup
branch
2 times, most recently
from
June 19, 2026 15:16
ab03e93 to
0cd7bdd
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
force-pushed
the
fix/cor-02-analytics-nested-rollup
branch
from
June 26, 2026 16:02
0cd7bdd to
c6129a8
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
Adversarial review (PR #1209)Reviewed against the six risk surfaces from the brief (exact aggregation shape, sibling queries, boundary aliasing, savings-already-net, active-only fetch, currency/unit consistency, tests, UI integration). The fix is sound; one blocker addressed in this push, no other findings. Verified
Fixes pushed
UNSTABLE statePre-rebase tip was UNSTABLE because:
CI for the post-rebase tip is in-flight; expect pre-commit migration check to pass now. VerdictLGTM. CR re-ping posted ( |
Contributor
|
(•ㅅ•)ゝ ✏️ Learnings added
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@internal/api/handler_purchases_revoke_test.go`:
- Around line 123-124: Retain coverage for PostgreSQL’s nil-success miss by
adding at least one history-fallthrough fixture that configures GetExecutionByID
to return (nil, nil), while preserving the existing wrapped ErrNotFound cases.
Update the relevant revokePurchase tests and analogous fixtures identified by
the repeated GetExecutionByID expectations.
In `@internal/api/handler_purchases_revoke.go`:
- Around line 162-166: Ensure the revoke flow does not process a nil execution:
in the handler around GetExecutionByID, require both execErr == nil and
execution != nil before entering the execution-revocation branch, preserving the
existing history lookup fallback for absent executions returned as (nil, nil).
In `@internal/api/handler_purchases_test.go`:
- Around line 1807-1808: Restore the GetExecutionByID mock in the affected test
to return (nil, nil), preserving coverage for PostgreSQL’s missing-row contract
and the nil dereference path in getPurchaseDetails; leave wrapped-error cases to
validate errors.Is separately.
In `@internal/api/handler_purchases.go`:
- Around line 1458-1460: Update the execution lookup handling in the relevant
handler to treat both config.ErrNotFound and a nil execution result with no
error as not found. Before dereferencing execution.PlanID, check execution ==
nil and return the same 404 NewClientError used by the existing errors.Is(err,
config.ErrNotFound) branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7be963d-d38d-47c8-86db-885ca326a5b3
📒 Files selected for processing (4)
internal/api/handler_purchases.gointernal/api/handler_purchases_revoke.gointernal/api/handler_purchases_revoke_test.gointernal/api/handler_purchases_test.go
Comment on lines
+123
to
+124
| // GetExecutionByID returns ErrNotFound: not an execution row, fall through to history. | ||
| mockStore.On("GetExecutionByID", ctx, "pid-1").Return(nil, fmt.Errorf("%w: execution pid-1", config.ErrNotFound)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retain coverage for PostgreSQL’s nil-success miss.
These fixtures exclusively model ErrNotFound, but the supplied PostgresStore.GetExecutionByID contract returns (nil, nil) for missing rows. Keep at least one history-fallthrough case using (nil, nil) in addition to the wrapped-error cases; otherwise the regression in revokePurchase remains green.
Also applies to: 152-153, 180-181, 207-208, 234-235, 349-350, 1007-1007, 1047-1047
🤖 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 `@internal/api/handler_purchases_revoke_test.go` around lines 123 - 124, Retain
coverage for PostgreSQL’s nil-success miss by adding at least one
history-fallthrough fixture that configures GetExecutionByID to return (nil,
nil), while preserving the existing wrapped ErrNotFound cases. Update the
relevant revokePurchase tests and analogous fixtures identified by the repeated
GetExecutionByID expectations.
Comment on lines
+162
to
+166
| execution, execErr := h.config.GetExecutionByID(ctx, purchaseID) | ||
| if execErr != nil && !errors.Is(execErr, config.ErrNotFound) { | ||
| return nil, fmt.Errorf("revoke: GetExecutionByID %s: %w", purchaseID, execErr) | ||
| } else if execution != nil { | ||
| } | ||
| if execErr == nil { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not revoke a nil execution.
The configured PostgreSQL store reports an absent execution as (nil, nil). This condition enters line 166 and skips loadAndRevokePurchaseHistory, whereas the previous execution != nil guard correctly fell through. Keep the nil guard so both not-found contracts reach history lookup.
Proposed fix
- if execErr == nil {
+ if execErr == nil && execution != nil {
return h.revokeScheduledExecution(ctx, session, execution)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| execution, execErr := h.config.GetExecutionByID(ctx, purchaseID) | |
| if execErr != nil && !errors.Is(execErr, config.ErrNotFound) { | |
| return nil, fmt.Errorf("revoke: GetExecutionByID %s: %w", purchaseID, execErr) | |
| } else if execution != nil { | |
| } | |
| if execErr == nil { | |
| execution, execErr := h.config.GetExecutionByID(ctx, purchaseID) | |
| if execErr != nil && !errors.Is(execErr, config.ErrNotFound) { | |
| return nil, fmt.Errorf("revoke: GetExecutionByID %s: %w", purchaseID, execErr) | |
| } | |
| if execErr == nil && execution != nil { |
🤖 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 `@internal/api/handler_purchases_revoke.go` around lines 162 - 166, Ensure the
revoke flow does not process a nil execution: in the handler around
GetExecutionByID, require both execErr == nil and execution != nil before
entering the execution-revocation branch, preserving the existing history lookup
fallback for absent executions returned as (nil, nil).
Comment on lines
+1807
to
+1808
| mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999"). | ||
| Return(nil, fmt.Errorf("%w: execution 99999999-9999-9999-9999-999999999999", config.ErrNotFound)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the nil-execution regression case.
This test should retain (nil, nil) to cover the documented PostgreSQL missing-row contract. The wrapped-error tests already validate errors.Is; replacing this fixture hides the nil dereference introduced in getPurchaseDetails.
Proposed fix
mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999").
- Return(nil, fmt.Errorf("%w: execution 99999999-9999-9999-9999-999999999999", config.ErrNotFound))
+ Return(nil, nil)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999"). | |
| Return(nil, fmt.Errorf("%w: execution 99999999-9999-9999-9999-999999999999", config.ErrNotFound)) | |
| mockStore.On("GetExecutionByID", ctx, "99999999-9999-9999-9999-999999999999"). | |
| Return(nil, nil) |
🤖 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 `@internal/api/handler_purchases_test.go` around lines 1807 - 1808, Restore the
GetExecutionByID mock in the affected test to return (nil, nil), preserving
coverage for PostgreSQL’s missing-row contract and the nil dereference path in
getPurchaseDetails; leave wrapped-error cases to validate errors.Is separately.
Comment on lines
+1458
to
+1460
| if errors.Is(err, config.ErrNotFound) { | ||
| return nil, NewClientError(404, "execution not found") | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Preserve the store’s (nil, nil) not-found contract.
PostgresStore.GetExecutionByID returns (nil, nil) for a missing row. This path now bypasses the 404 mapping and panics at execution.PlanID on line 1469. Treat either representation as not found.
Proposed fix
- if errors.Is(err, config.ErrNotFound) {
+ if errors.Is(err, config.ErrNotFound) || execution == nil {
return nil, NewClientError(404, "execution not found")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if errors.Is(err, config.ErrNotFound) { | |
| return nil, NewClientError(404, "execution not found") | |
| } | |
| if errors.Is(err, config.ErrNotFound) || execution == nil { | |
| return nil, NewClientError(404, "execution not found") | |
| } |
🤖 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 `@internal/api/handler_purchases.go` around lines 1458 - 1460, Update the
execution lookup handling in the relevant handler to treat both
config.ErrNotFound and a nil execution result with no error as not found. Before
dereferencing execution.PlanID, check execution == nil and return the same 404
NewClientError used by the existing errors.Is(err, config.ErrNotFound) branch.
…view Snapshot rows are run-rates written at (account, provider, service, region, commitment_type, timestamp) grain, so a breakdown bucket holds several rows sharing the SAME timestamp (one per region / commitment type / account). QueryByProvider, QueryByService and the monthly_savings_summary materialized view used a flat AVG across all rows in the bucket, which returns the mean per-row run-rate instead of the bucket's total: $100/mo in each of 5 regions reported $100 instead of $500, understating savings for any multi-region estate. Apply the H5 nested shape that daily_savings_trend and provider_savings_summary already use: the inner query SUMs the per-row run-rates into the bucket's instant total at each collection timestamp, the outer query AVGs those instant totals over time so results stay invariant to collection frequency. - QueryByProvider / QueryByService: rewrite to the nested rollup. - Migration 000076: recreate monthly_savings_summary with the nested rollup (snapshot_count keeps raw-row semantics, cast back to BIGINT); down restores the 000067/000074 flat-AVG definition. - Regression test (integration tag) replicating the failing scenario: two regions plus two commitment types sharing a timestamp; asserts buckets report the per-timestamp sum (325) not the row mean (108.33) across both Go queries and the view, plus a down/up round-trip. All subtests fail against the pre-fix code. - Update pgxmock SQL-shape expectations to assert the nested query. Closes #1151
…USER Three pre-commit blockers introduced by main commits landing after the PR was originally created: - Migration 000076 collision: 1a17089 (fix/main-ci-failing-jobs) added 000076_matview_unique_indexes_plain_columns; rename the COR-02 nested- rollup migration to 000077 and align its unique index to use NULLS NOT DISTINCT (consistent with 000076 which switched from COALESCE). - gocyclo complexity: 94d0d92 added logic to recEffectiveSavingsPct that pushed cyclomatic complexity to 12 (limit 10). Extract recOnDemandBaseline to bring both functions under the limit. No functional change. - trivy DS-0002: Dockerfile.test lacked a USER command. Add a non-root e2e user so the E2E runner does not execute as root.
Rebased onto current main, which had already promoted audit_actor_stamps to 000077 (5894580, the renumber landed in PR #1232 review). The COR-02 nested-rollup migration was added at 000077 in this PR, which now collides with main's 000077_audit_actor_stamps and trips the pre-commit "Check for conflicting migration numbers" hook (CI's pre-commit run was failing on this branch with `Duplicate migration number(s) found: 000077`). Rename to 000078: - internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.up.sql - internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.down.sql - internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go Body unchanged except for the leading `-- 000077:` / `-- 000077 down:` header comments updated to 000078; the body-comment references to `000067/000074` (the historical flat-AVG definitions) stay as-is because they cite past migrations, not the file being renamed. Verified: - ls internal/database/postgres/migrations/*.up.sql | cut -c1-6 | sort | uniq -d prints nothing. - go build ./... succeeds. - go test ./internal/analytics/... passes 186/186. Refs #1151 (COR-02).
UpsertRecommendations fails with recommendations_cloud_account_id_fkey when the accounts referenced by CloudAccountID do not exist in cloud_accounts yet (migration 000030 added this FK constraint). Add seedRecommendationCloudAccount helper (mirrors main branch) and call it at the top of AccountScopedEviction and AmbientAndRegisteredCoexist tests so the FK is satisfied before any recommendation inserts.
The rebase onto main surfaced a redeclaration: main already defines seedRecommendationCloudAccount in store_postgres_recommendations_test.go, and this branch added an identical copy, breaking the typecheck for both Lint Code and the integration build. Remove the branch's redundant copy so the package compiles; the retained definition is byte-identical in body.
cristim
force-pushed
the
fix/cor-02-analytics-nested-rollup
branch
from
July 10, 2026 12:57
ab6865d to
7c07b32
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
After the rebase renumbered this migration to 000078, main's 000079-000081 sit above it, so RollbackMigrations(..., 1) only undid the topmost migration and left the nested-rollup view in place; the flat-AVG assertion then failed. Migrate down to version 77 so 000078's down runs regardless of how many later migrations stack on top, then RunMigrations re-applies the full stack. Verified against a real postgres container: all four COR-02 subtests pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closes #1151 (review finding COR-02, P1).
Snapshot rows in
savings_snapshotsare run-rates written at (account, provider, service, region, commitment_type, timestamp) grain, so a breakdown bucket contains several rows sharing the SAME timestamp (one per region / commitment type / account).QueryByProvider,QueryByServiceand themonthly_savings_summarymaterialized view used a flatAVG(total_savings)across all rows in the bucket, which returns the mean per-row run-rate instead of the bucket's total: $100/mo in each of 5 regions reportedtotal_savings = $100instead of$500. Multi-region / multi-account estates saw systematically understated savings on the analytics view; single-region single-account deployments saw correct numbers, hiding the bug.The same migration that introduced these (000067) fixed
daily_savings_trendandprovider_savings_summarywith the correct nested shape (per H5 andfeedback_snapshot_runrate_avg), but skipped the two Go queries and the monthly view.Fix
Apply the same H5 nested SUM-then-AVG rollup everywhere: the inner query SUMs the per-row run-rates into the bucket's instant total at each collection timestamp, the outer query AVGs those instant totals over time, so results stay invariant to collection frequency. The collector stamps one shared
nowper collection run across all tenants, so per-timestamp summing across regions/accounts/commitment types is the correct instant total.internal/analytics/postgres_analytics.go:QueryByProviderandQueryByServicerewritten to the nested rollup.000076_monthly_summary_nested_rollup: recreatesmonthly_savings_summarywith the nested rollup.snapshot_countkeeps its raw-row semantics (SUM(ts_row_count)::BIGINT, sinceSUM(bigint)yieldsNUMERIC); the unique index is recreated unchanged for theCONCURRENTLYrefresh path. The down migration restores the 000067/000074 flat-AVG definition.SUM(total_savings)grouped by dims + timestamp).Test evidence
New regression test
TestAnalyticsNestedRollup_COR02(integration tag, testcontainers) replicates the real failing scenario: provideraws, servicerds, two collection timestamps, with two regions plus two commitment types sharing each timestamp. It asserts:QueryByProvider: AVG(T1 $250, T2 $400) = $325, not the pre-fix row mean $108.33QueryByService: us-east-1 = $225 (RI + SavingsPlan summed per timestamp), not $112.50monthly_savings_summary: $325 after refresh,snapshot_count= 6Verified the test FAILS against the pre-fix code: with
postgres_analytics.goreverted toorigin/mainand the migration removed, all three rollup subtests fail; with the fix restored, all pass.Runs:
go build ./...- successgo test ./internal/analytics/ ./internal/api/- 1813 passedgo test -tags=integration -run TestAnalyticsNestedRollup_COR02 ./internal/database/postgres/migrations/- 5 passed (real Postgres via testcontainers)Notes / out of scope
The analytics package's own integration-tagged test files (
postgres_analytics_db_test.go,postgres_analytics_integration_test.go) are bit-rotted and do not compile under-tags=integration(duplicategetMigrationsPath, staleQueryByProvider/QueryByServicesignatures, non-pointerTotalUsage/CoveragePercentageliterals). That predates this change and is why the regression test lives in the migrations package, whose integration tag builds. Worth a follow-up issue.Summary by CodeRabbit