[IAmJSD]

Summary

The Trends view (#1317) is meant to show committed spend and usage over time. GET /api/analytics/trends could not supply either, because its monthly series did not carry them. This PR adds total_commitment (amortized upfront) and total_usage (recurring fees) to each monthly row; committed spend is their sum, and keeps an unknown value unknown rather than turning it into 0.

Refs #30

Root cause

monthly_savings_summary has had total_commitment and total_usage columns since migration 000067. Since migration 000143 its rollups also return NULL when any snapshot in the bucket has no figure. But QueryMonthlyTotals (internal/analytics/postgres_analytics.go) selected only total_savings, avg_coverage and snapshot_count, and analytics.MonthlySummary had no fields for the other two columns.

Fix

• internal/analytics/interfaces.go: MonthlySummary gets TotalCommitment and TotalUsage (*float64, json:"…,omitempty"). nil means unknown and is never read as 0. A known 0 is a non-nil pointer to 0, so it is still sent. Omitting an unknown value matches how the same struct already handles avg_coverage.
• internal/analytics/postgres_analytics.go: QueryMonthlyTotals selects and scans total_commitment, total_usage. There is no COALESCE, so the view's NULL reaches the caller unchanged.
• internal/api/openapi.yaml: TrendsMonthlySummary documents both fields. Each is a monthly run-rate (the bucket's total at each collection, averaged over the month, the same way as total_savings). Each is omitted when unknown and returned as 0 when it is a known zero.

TrendsResponse returns []analytics.MonthlySummary directly, so the handler needs no change. The collector test mock (mockAnalyticsStore.QueryMonthlyTotals in collector_test.go) also needs no change: the interface signature is the same, and the mock builds no MonthlySummary values.

There is no migration and no new write path. The query is a plain read of a materialized view, so concurrent callers are not a concern here: refreshes already run CONCURRENTLY against the unique index and are unchanged.

Verification

All evidence below is local: unit tests against pgxmock and testify mocks, and integration tests against real Postgres (testcontainers). Go 1.26.9, run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

go build ./...                                   # ok
go vet ./... && go vet -tags integration ./...   # ok
go test -count=1 ./internal/analytics/           # ok
go test -count=1 ./internal/api/                 # ok
go test -count=1 ./internal/mocks/               # ok
golangci-lint run --allow-parallel-runners ./internal/analytics/ ./internal/api/            # 0 issues
golangci-lint run --allow-parallel-runners --build-tags integration ./internal/analytics/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/analytics/interfaces.go internal/analytics/postgres_analytics.go   # nothing over 10
go test -tags integration -count=1 -timeout 20m \
  -run 'TestPostgresAnalyticsStore_QueryMonthlyTotals$|TestPostgresAnalyticsStore_QueryMonthlyTotals_UnknownStaysUnknown' \
  ./internal/analytics/                          # 2 PASS (real Postgres)

New and updated tests:

• Integration, new: TestPostgresAnalyticsStore_QueryMonthlyTotals_UnknownStaysUnknown seeds three buckets:

  • rds: two regions at one timestamp; commitment 300 and 100, usage 250 and NULL.
  • ec2: commitment NULL and 70, usage 40 and 60.
  • elasticache: commitment 0, usage 0.

  It refreshes the views and checks that rds commitment is 400 and rds usage is nil (not 250), that ec2 commitment is nil (not 70) and ec2 usage is 100, and that elasticache returns a non-nil 0 for both.

• Integration, extended: TestPostgresAnalyticsStore_QueryMonthlyTotals now also checks commitment 1000 and usage 800.

• Unit: TestPostgresAnalyticsStore_QueryMonthlyTotals_WithRows (known, NULL and zero rows), TestQueryMonthlyTotals, TestQueryMonthlyTotalsRowsErr (mock columns updated), TestMonthlySummary (unknown is omitted, a known 0 is sent), and TestHandler_getAnalyticsTrends_MonthlyCommitmentAndUsage (the wire shape of /api/analytics/trends).

Fail-before / pass-after:

• With interfaces.go and postgres_analytics.go reset to origin/main, the new tests do not compile (TotalCommitment undefined).
• With the new types but main's query and scan, these fail:
  • unit: TestPostgresAnalyticsStore_QueryMonthlyTotals_WithRows, TestQueryMonthlyTotals and TestQueryMonthlyTotalsRowsErr;
  • integration: TestPostgresAnalyticsStore_QueryMonthlyTotals and TestPostgresAnalyticsStore_QueryMonthlyTotals_UnknownStaysUnknown.
• Mutation probes, each reverted afterwards:
  • Wrapping both columns in COALESCE(…, 0) fails TestPostgresAnalyticsStore_QueryMonthlyTotals_UnknownStaysUnknown.
  • Dropping omitempty, so an unknown is sent as null, fails TestMonthlySummary and TestHandler_getAnalyticsTrends_MonthlyCommitmentAndUsage.
• On the branch everything passes.

Not run here: the other integration tests that read QueryMonthlyTotals (internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go, postgres_analytics_account_filter_db_test.go). They are left to the combined-tree run.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Out of scope

• The Coverage data source is a design decision and is not addressed. The collector still writes coverage_percentage = NULL, because purchase_history has no on-demand baseline, so avg_coverage stays omitted.
• by_provider / by_service (QueryByProvider / QueryByService) still return no commitment or usage. The issue's breakdowns only need savings and coverage. Adding the other two would mean a new NULL-propagating aggregate over savings_snapshots, which is better done as its own change if the UI wants it.
• Frontend changes, which the UI owner makes (see below).

Frontend follow-up for the UI owner

• Data: in the Trends sub-tab (feat(frontend): add a Trends sub-tab for the analytics trends endpoint #1317), each monthly[i] row now has total_commitment (upfront payments amortized per month) and total_usage (recurring monthly fees), both monthly run-rates in USD. Committed spend is total_commitment + total_usage; a no-upfront portfolio has total_commitment 0. If either is missing, show committed spend as unknown rather than summing a partial value. Plot it per month next to realized savings.
• Missing values: when a field is missing, the value is unknown. Render a gap or the shared money placeholder (fix(frontend): one money formatter and placeholder, right-align money #1376), never $0, and never sum it as 0 when combining rows across accounts or services. If any row in a month lacks the field, that month's combined total is unknown too.
• Zero: a present 0 is a real zero and should be shown as $0.
• Tests: cover all three cases: present, absent, and 0.

Maintainers: please mirror the issue's triage labels onto this PR: priority/p2, triaged, severity/low, urgency/this-quarter, impact/many, effort/m, type/feat.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 7b86e38
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 01b58276-b1cb-4cdb-b348-6ec9d4b71269

Commits

Reviewing files that changed from the base of the PR and between 7b86e38 and 750d421.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 8

• go.mod
• internal/analytics/interfaces.go
• internal/analytics/postgres_analytics.go
• internal/analytics/postgres_analytics_integration_test.go
• internal/analytics/postgres_analytics_pgxmock_test.go
• internal/analytics/postgres_analytics_test.go
• internal/api/handler_analytics_test.go
• internal/api/openapi.yaml

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE ec9e00371214849207a882589246bf0498185824

Blocking:

• none

Non-blocking:

• total_commitment is not total committed spend. The openapi description (and the UI follow-up in the PR body) calls total_commitment "Committed spend as a monthly run-rate". The collector (internal/analytics/collector.go:182-187) stores only the upfront cost amortized per month (UpfrontCost / (term*12)), and puts the recurring fee in total_usage (MonthlyCost). As a result:

  • a no-upfront RI or SP contributes $0 to total_commitment;
  • an all-upfront one makes total_usage unknown.

  A Trends chart built from the current description would show "committed spend $0" for an all-no-upfront portfolio. Please describe the fields as the amortized-upfront and recurring components, with committed spend being their sum (unknown if either is unknown), before the UI owner builds on it. That's a doc change only, and the data is right.

• by_provider / by_service still carry no commitment or usage, which is correctly listed as out of scope. Note that provider_savings_summary already has a NULL-propagating total_commitment column (migration 000143) if the UI later wants it per provider.

Attacked:

• NULL propagation: read migration 000143's monthly_savings_summary, the latest definition, with no later migration redefining it. Both columns are CASE WHEN COUNT(x)=COUNT(*) THEN AVG(...) at both rollup levels. The query adds no COALESCE and scans into *float64, and omitempty on a pointer keeps a known 0 (non-nil) in the JSON. The new integration test seeds NULL, mixed and 0 buckets against real Postgres.
• Also checked that the scan column order matches the SELECT order, and that MonthlySummary isn't built positionally anywhere else.
• Fail-before: I reset postgres_analytics.go to origin/main, keeping the new struct fields and tests. TestPostgresAnalyticsStore_QueryMonthlyTotals_WithRows, TestQueryMonthlyTotals and TestQueryMonthlyTotalsRowsErr FAIL. I restored the file afterwards.
• Ran on f637921a: go build ./...; go vet with and without -tags integration on internal/analytics and internal/api; golangci-lint v2.10.1 with and without --build-tags integration (0 issues); gocyclo -over 10 on both changed non-test files (silent); go test -race -short -count=1 ./internal/analytics/ ./internal/api/ (ok); go test -race -tags integration -count=1 ./internal/analytics/ (ok, 137s, real Postgres) and ./internal/api/ (ok, 298s, real Postgres).
• The head moved to ec9e0037 while I was reviewing. The only new commit is the x/net v0.62.0 bump, byte-identical to fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564. On ec9e0037 I re-ran go build ./... and go vet ./..., both clean.

Body-stale:

• The "Frontend follow-up" section says "Plot committed spend and usage per month". As explained above, total_commitment alone isn't committed spend.

_{Reviewer: rG1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 750d421123682d7b6e5363b30b32df177fbfe7a1

Blocking:

• none

Non-blocking:

• internal/analytics/interfaces.go:57: the Go doc comment still calls the two fields "run-rates of committed spend and covered usage". After 750d421 the OpenAPI text says, correctly, that TotalCommitment is only the amortized upfront part and that committed spend is TotalCommitment + TotalUsage. Worth aligning the comment so the next consumer doesn't plot TotalCommitment alone as "committed spend".
• openapi total_usage: "for example an all-upfront purchase" matches collector.go's comment. Per config.PurchaseHistoryRecord.MonthlyCost's doc, though, older AWS all-upfront rows hold a real 0.0, so an all-upfront bucket can also come back as a known 0 rather than omitted. The text is hedged ("for example"), so this is wording only.
• frontend/src/api/analytics.ts TrendsMonthlyRow doesn't have the two new optional fields yet. That fits Refs feat(frontend): Trends view consuming GET /api/analytics/trends #30, since the Trends view is the frontend half.

Attacked:

• Newest commit (750d421) checked against internal/analytics/collector.go. total_commitment is Σ UpfrontCost / (Term*12) per bucket per collection. A nil UpfrontCost makes the snapshot NULL, and a non-nil 0 means no upfront charge (common.PurchaseResult.Cost: "a non-nil zero means no upfront charge"), so "a no-upfront portfolio is returned as 0" holds. total_usage is Σ MonthlyCost, with any nil making the snapshot NULL. Migration 000143's monthly_savings_summary sums per timestamp and then AVGs over the month, NULL if any snapshot is NULL. That matches "summed per collection and averaged over the month" and "omitted when any snapshot in the bucket had no figure". The two descriptions are correct.
• QueryMonthlyTotals adds the two columns to the SELECT and the Scan in the same order, with no COALESCE. NUMERIC→*float64 scans correctly on real Postgres.
• Ran go build ./..., go vet on analytics, api and database/... with and without -tags integration, and go test -race -short -count=1 ./internal/analytics/ ./internal/api/ (ok). go test -race -tags integration -count=1 ./internal/analytics/ (whole package, ok) and -run 'TestAnalyticsNestedRollup|Rollups|Propagate' ./internal/database/postgres/migrations/ (ok). golangci-lint v2.10.1 found 0 issues with and without the integration tag, gocyclo is silent, and secrets-scan printed "clean; planted key flagged".
• Fail-before: with only postgres_analytics.go reset to origin/main (new struct fields kept), TestPostgresAnalyticsStore_QueryMonthlyTotals_WithRows, TestQueryMonthlyTotals, TestQueryMonthlyTotalsRowsErr and the integration tests TestPostgresAnalyticsStore_QueryMonthlyTotals and ..._UnknownStaysUnknown fail. Restored afterwards.

Body-stale:

• none

_{Reviewer: rH1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 750d421123682d7b6e5363b30b32df177fbfe7a1 merged onto main (b85e42a0), together with #1461, #1557, #1563 in that order (combined tree 4d91ad42, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 750d4211, green CI, and the combined-tree verification above. Squash-merged at exactly 750d421123682d7b6e5363b30b32df177fbfe7a1.