feat(analytics): finish & wire historical savings snapshots (closes #1023, #1033) - #1049
Merged
cristim merged 11 commits intoJun 7, 2026
Merged
Conversation
…ount_id views (#1023) Migration 000067 prepares savings_snapshots and its materialized views for the now-live collector: - H4: widen account_id VARCHAR(20) -> VARCHAR(255) so Azure subscription IDs (36) and GCP project IDs (<=30) no longer overflow on insert. - H2: drop NOT NULL/DEFAULT 0 on total_usage and coverage_percentage so absent metrics stay NULL and AVG/SUM skip them instead of being dragged toward zero (project rule feedback_nullable_not_zero). - H3: recreate monthly_savings_summary / daily_savings_trend / provider_savings_summary carrying cloud_account_id so cloud_account_id-scoped Query* can read the pre-aggregated rows. Unique indexes COALESCE the nullable cloud_account_id to the nil UUID to keep the CONCURRENTLY-refresh grain unique. - M5/N4: narrow drop_old_savings_partitions' WHEN OTHERS handler to the expected name-parse error and surface SQLERRM so retention failures are observable. Idempotent throughout (feedback_migration_full_restore): the column-widen is guarded so a re-run converges rather than no-op'ing over a wrong type. The down keeps account_id at VARCHAR(255) deliberately to avoid truncating ids written while applied.
…d scoping (#1023) Fix the latent data-correctness bugs in the savings-snapshot subsystem so the data is correct once the collector flows, and make cloud_account_id the tenant scoping key across writes and Query*. Collector (collector.go): - H1: skip Term <= 0 rows instead of dividing by zero. The amortized commitment (UpfrontCost / (Term*HoursPerYear)) would otherwise produce +Inf/NaN that poisons every downstream SUM/AVG and errors at the DECIMAL bind. - H2: derive real total_usage from the source MonthlyCost (recurring covered spend) when present; leave it NULL otherwise. coverage_percentage stays NULL (purchase_history carries no on-demand baseline to derive it from) rather than a placeholder 0 (feedback_nullable_not_zero). - H3: populate cloud_account_id (the tenant FK) and key aggregation buckets on it, so two tenants sharing a provider account number never merge. account_id stays as a descriptive attribute. - Collect across all tenants via GetAllPurchaseHistory; treat ctx cancellation as terminal (feedback_ctx_cancel_terminal). Store (postgres_analytics.go, interfaces.go): - Query* now take the dual-column filter (accountUUIDs + accountExternalIDsByProvider) and OR both columns via accountFilterClause, mirroring the live purchase_history path so cloud_account_id-NULL rows still match. Nullable usage/coverage become *float64; SavingsSnapshot gains CloudAccountID *string. - M1: QueryMonthlyTotals uses make_interval(months => $1-1) for an exact inclusive N-month window (no off-by-one). - L3: CreatePartitionsForRange rejects a reversed range. L4: BulkInsertSnapshots validates commitment_type before COPY so one bad row can't abort the batch. - Add CreateFuturePartitions for the partition-maintenance job. Tests rewritten to exercise the REAL store via pgxmock (the parallel reimplementation that could drift is gone), with regression cases for H1 (no +Inf/NaN), H2 (real usage vs NULL), H3 (tenant non-merge), and ctx-cancel.
…n a schedule (#1033) Wire the previously-dead collector into both transports via a new scheduled task. The analytics_collect task (analytics_collect.go) runs the full pipeline end to end: ensure upcoming monthly partitions exist (M3), collect a snapshot across all tenants, apply retention, then refresh the materialized views over the fresh data. It is advisory-lock guarded like every other scheduled task and reaches both Lambda (EventBridge) and container (Cloud Scheduler / Logic Apps HTTP) deploys through the shared HandleScheduledTask dispatch. - New TaskCollectAnalytics ("analytics_collect") task type + dispatch + event parsing alongside the existing analytics_refresh task. - Collector is built once in reinitializeAfterConnect via a single localized call (newAnalyticsCollector) to keep the app.go footprint small for the in-flight app.go rebase. Exposed on the api handler so the manual /api/analytics/collect trigger works too. - Config knobs (ANALYTICS_COLLECTION_ENABLED, ANALYTICS_RETENTION_MONTHS, ANALYTICS_PARTITIONS_AHEAD) loaded from env and validated at startup (NewApplication fails fast on out-of-range values). - Pipeline steps are best-effort with a "partial" status on any single failure, but a cancelled context aborts and surfaces the error.
…lytics/trends (#1033) Surface the now-populated savings_snapshots series through a new read endpoint returning the monthly coverage/utilization/committed-spend/realized-savings trend plus by-provider and by-service breakdowns. - New AnalyticsSnapshotStoreInterface (read side of internal/analytics) wired on the api handler; nil disables the route with a 503, matching the existing /history/analytics empty-state contract. - getAnalyticsTrends enforces allowed_accounts scope BEFORE resolving filter ids (validateAnalyticsAccountScope), then resolves the requested account into the dual-column filter (resolveSingleAccountFilterIDs) so rows carrying only the external account_id are still matched and a scoping lookup never degrades to ("",nil). Distinct from the purchase_history-backed /history/analytics path, which the snapshot series complements with point-in-time coverage data. - Route registered at GET /api/analytics/trends (AuthUser). Tests cover dual-column scoping pass-through, 503 when unconfigured, scoped-user account_id requirement, and rejection of an out-of-scope account.
…#1033) Add the EventBridge rule + target + lambda permission that periodically invokes the analytics_collect task (snapshot + partition maintenance + retention + view refresh), mirroring the reap_stuck_purchases pattern. Gated by enable_analytics_collect_schedule (default true) with a daily analytics_collect_schedule default, since the collector produces a point-in-time series and a daily cadence captures a clean coverage/utilization trend without excess write volume.
Contributor
|
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 (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughConverts analytics to multi-tenant snapshot collection: nullable usage/coverage, cloud_account_id scoping, Postgres store updates (COPY, accountFilterClause, partition management), scheduled collector wiring, trends API, migrations, Terraform schedule, and many updated tests. ChangesMulti-Tenant Analytics Collection & Query System
Sequence DiagramsequenceDiagram
participant Client
participant API as Handler.getAnalyticsTrends
participant SnapshotStore as AnalyticsSnapshotStoreInterface
participant Postgres as PostgresAnalyticsStore
Client->>API: GET /api/analytics/trends?start=&end=&account_id=
API->>SnapshotStore: QueryMonthlyTotals/QueryByProvider/QueryByService (AccountUUIDs, externalIDs, dates)
SnapshotStore->>Postgres: SQL queries using accountFilterClause (cloud_account_id + account_id)
Postgres-->>SnapshotStore: rows (cloud_account_id, nullable totals/coverage)
SnapshotStore-->>API: assembled MonthlySummary/Provider/Service arrays
API-->>Client: TrendsResponse (RFC3339 start/end, months, monthly, by_provider, by_service)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
11 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/analytics/postgres_analytics_test.go (1)
216-230: ⚡ Quick winThis subtest doesn’t currently verify
commitment_typevalidation.Because pgxmock fails on
Acquirefirst, this path only proves “non-empty bulk insert returns an error,” not the invalid-type guard. Please rename the test or extract the validation into a unit-testable helper so this assertion matches reality.🤖 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/analytics/postgres_analytics_test.go` around lines 216 - 230, The test titled "rejects invalid commitment_type before COPY" misrepresents what it verifies because pgxmock fails on pgxmock.Acquire before commitment_type is validated; update the test to either rename it to reflect that it asserts acquisition fails (e.g., "errors when connection acquire fails") OR move the commitment type validation into a small, unit-testable helper function (e.g., ValidateCommitmentType or method on SavingsSnapshot) and add a direct unit test that calls that helper with SavingsSnapshot{CommitmentType: "bogus"} asserting an error; adjust the existing subtest to no longer claim it verifies invalid-type behavior and reference BulkInsertSnapshots, SavingsSnapshot, and pgxmock.Acquire when making the change.
🤖 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/analytics/collector.go`:
- Around line 22-25: The hard-coded purchaseHistoryFetchLimit = 100000 causes
GetAllPurchaseHistory(..., 100000) to silently truncate results; change the
collector to either query only active commitments (filter by active
status/expiry) or implement a paginated scan over GetAllPurchaseHistory (looping
with offset/limit or by cursor until no more rows) instead of a single fixed
cap; update references to purchaseHistoryFetchLimit and the code that calls
GetAllPurchaseHistory to use the new paging/cursor or active-only query so all
active 1y/3y commitments are included.
- Around line 145-154: The code currently converts monthly values to hourly
(agg.savings += p.EstimatedSavings / HoursPerMonth, agg.commitment +=
p.UpfrontCost / (float64(p.Term) * HoursPerYear), agg.usage += *p.MonthlyCost /
HoursPerMonth) which makes persisted totals schedule-dependent; instead persist
schedule-invariant units or apply elapsed-time weighting at aggregation: either
stop converting to hourly and store monthly amounts (use p.EstimatedSavings,
p.UpfrontCost amortized by term in months, and *p.MonthlyCost as-is) so
SUM(total_savings)/SUM(total_commitment)/SUM(total_usage) represent monthly
totals, or keep hourly internal values but adjust snapshot aggregation to
multiply by the elapsed hours for the snapshot window; update the code around
agg.savings, agg.commitment, agg.usage (and related snapshot SUM usage) to
implement one consistent approach and ensure p.MonthlyCost nil handling remains
unchanged.
In `@internal/api/handler_analytics.go`:
- Around line 48-50: The trends endpoint currently returns raw parseDateRange
errors which are treated as 500s; instead wrap/paraphrase parseDateRange
failures as a client error with HTTP 400. In the trends handler in
internal/api/handler_analytics.go, replace the direct return of err after
calling parseDateRange(...) with returning a ClientError (or using the existing
helper that produces a 400 ClientError) containing a concise message about
invalid start/end query parameters so malformed dates produce a 400 response.
In
`@internal/database/postgres/migrations/000067_analytics_snapshot_correctness.down.sql`:
- Around line 57-68: The REFRESH MATERIALIZED VIEW calls for
monthly_savings_summary, daily_savings_trend, and provider_savings_summary run
before NULLs are coerced and NOT NULL/defaults are restored on
savings_snapshots.total_usage and savings_snapshots.coverage_percentage; move
the REFRESH MATERIALIZED VIEW statements to after the UPDATE and ALTER TABLE
statements so the views are refreshed against the corrected data and schema
(i.e., perform the UPDATEs that set NULLs to 0 and the ALTER TABLE ... SET
DEFAULT/SET NOT NULL on total_usage and coverage_percentage first, then run
REFRESH MATERIALIZED VIEW for monthly_savings_summary, daily_savings_trend, and
provider_savings_summary).
In `@internal/server/analytics_collect.go`:
- Around line 35-44: LoadAnalyticsConfig currently uses getEnvInt for
RetentionMonths and PartitionsAhead which swallows parse errors and prevents
startup validation from failing; change LoadAnalyticsConfig to preserve
malformed integer env values by reading the raw env string (instead of
getEnvInt) for ANALYTICS_RETENTION_MONTHS and ANALYTICS_PARTITIONS_AHEAD and
storing those raw values in AnalyticsConfig (or a new temporary string field) so
Validate can detect and reject unparseable values; keep getEnvBool for Enabled
unchanged and ensure AnalyticsConfig/Validate are updated to parse/validate the
raw strings rather than relying on getEnvInt's defaulting behavior.
---
Nitpick comments:
In `@internal/analytics/postgres_analytics_test.go`:
- Around line 216-230: The test titled "rejects invalid commitment_type before
COPY" misrepresents what it verifies because pgxmock fails on pgxmock.Acquire
before commitment_type is validated; update the test to either rename it to
reflect that it asserts acquisition fails (e.g., "errors when connection acquire
fails") OR move the commitment type validation into a small, unit-testable
helper function (e.g., ValidateCommitmentType or method on SavingsSnapshot) and
add a direct unit test that calls that helper with
SavingsSnapshot{CommitmentType: "bogus"} asserting an error; adjust the existing
subtest to no longer claim it verifies invalid-type behavior and reference
BulkInsertSnapshots, SavingsSnapshot, and pgxmock.Acquire when making the
change.
🪄 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: 15a1dc24-c0fc-4a1e-b4e5-ce94250cd5c5
📒 Files selected for processing (22)
internal/analytics/collector.gointernal/analytics/collector_test.gointernal/analytics/interfaces.gointernal/analytics/postgres_analytics.gointernal/analytics/postgres_analytics_mock_test.gointernal/analytics/postgres_analytics_nilsafe_test.gointernal/analytics/postgres_analytics_pgxmock_test.gointernal/analytics/postgres_analytics_test.gointernal/api/handler.gointernal/api/handler_analytics.gointernal/api/handler_analytics_test.gointernal/api/router.gointernal/api/types.gointernal/database/postgres/migrations/000067_analytics_snapshot_correctness.down.sqlinternal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sqlinternal/server/analytics_collect.gointernal/server/app.gointernal/server/handler.gointernal/server/handler_coverage_test.gointernal/server/interfaces.goterraform/modules/compute/aws/lambda/main.tfterraform/modules/compute/aws/lambda/variables.tf
This was referenced Jun 7, 2026
Resolve the five CodeRabbit actionables (plus the test nitpick) on PR #1049: - handler_analytics: parseDateRange now returns a 400 ClientError for malformed start/end and reversed ranges so handler.go no longer maps them to HTTP 500. Fixes the trends endpoint and the two /history/* callers at the source. - migration 000067 down: refresh the materialized views AFTER restoring the NOT NULL/default semantics so the recreated views reflect coerced data. - analytics_collect: LoadAnalyticsConfig preserves a malformed ANALYTICS_RETENTION_MONTHS/ANALYTICS_PARTITIONS_AHEAD as a Validate-failing sentinel (fail-fast) instead of silently falling back to the default. - collector: log a loud warning when the purchase_history fetch hits its cap so the potential undercount is observable (full paginated/active-only read tracked in a follow-up). - postgres_analytics: extract validateCommitmentType so the COPY-path guard is unit-testable directly; rename the misleading bulk-insert subtest. Regression tests assert the 400 ClientError on bad dates, the fail-fast env sentinel, and the commitment_type guard.
The analytics_collect pipeline ran partition provisioning, retention, and materialized-view refresh on the scheduled-task context, which carries no DB deadline. Under RDS Proxy a session statement_timeout is not honoured, so a runaway DDL step (e.g. a CONCURRENTLY refresh blocked on a lock) could hang the whole scheduled run indefinitely. Wrap each DDL step (CreateFuturePartitions, DropOldPartitions, RefreshMaterializedViews) in a bounded child context (analyticsDDLTimeout, 5m) so the pipeline makes forward progress or fails fast. Regression test runs the pipeline under a deadline-free parent context and asserts each step observes a deadline (the bound can only have come from the pipeline itself). Folds finding 06-N3 from the data-layer review. Closes #1073
Member
Author
pr-iterate pass: CR actionables addressed + FOLD-1049 foldedCodeRabbit actionables (commit
|
| Finding | File | Disposition |
|---|---|---|
| Return 400 for invalid date inputs | handler_analytics.go (parseDateRange) |
Fixed: malformed/reversed dates now return NewClientError(400, ...) so handler.go no longer maps them to HTTP 500. Fixed at the source, so the trends endpoint and both /history/* callers are covered. |
| Refresh views after restoring NOT NULL/default | 000067_*.down.sql |
Fixed: moved the three REFRESH MATERIALIZED VIEW statements to after the NULL coercion + SET NOT NULL, so the recreated views reflect corrected data. |
| Fail-fast for malformed analytics int env vars | server/analytics_collect.go (LoadAnalyticsConfig) |
Fixed: a set-but-unparseable ANALYTICS_RETENTION_MONTHS/ANALYTICS_PARTITIONS_AHEAD is preserved as a 0 sentinel that Validate() rejects at startup, instead of silently defaulting. |
| Remove fixed 100k history cap or page the read | analytics/collector.go |
Partial + follow-up #1074: the collector now logs a loud WARNING when the fetch fills the cap so the undercount is observable. A paginated/active-only store read is a store-interface change tracked in #1074. |
| Persisted units don't match query aggregations | analytics/collector.go |
Follow-up #1074: correcting the schedule-dependent hourly-vs-monthly aggregation needs a design decision that also touches migration 000067's view math and the integration suite, so it is tracked separately. No consumer is misled yet (Trends frontend deferred in #1048). |
| Nitpick: subtest doesn't verify commitment_type | postgres_analytics_test.go |
Fixed: extracted validateCommitmentType; BulkInsertSnapshots calls it; new direct unit test TestValidateCommitmentType; renamed the misleading subtest to reflect the acquire-failure path it actually exercises. |
FOLD-1049 data-layer findings (commit cbaa51fe1)
- 06-N3 (bounded ctx for partition/retention/refresh under RDS Proxy): each scheduled DDL step now runs under a 5m per-step child context. Regression test
TestHandleCollectAnalytics_DDLStepsAreBounded. - 06-M1 / M3 / M5 / L3 / L4 / N1 / N4: already implemented by this PR (verified against committed code).
- 06-M2: the query builder uses only positional placeholders (
accountFilterClause,#nosec G201justified) mirroring the liveapi.accountFilterClausepath; no injection surface, so the($n IS NULL OR col=$n)rewrite was skipped as style-only.
Verification
go build ./...,go vet, andgo test ./internal/analytics/... ./internal/api/... ./internal/server/...all pass (2027 tests green in the touched packages).- No live Postgres in this environment: DB paths are covered via pgxmock against the real store; the
//go:build integrationsuite needs a real Postgres to run. - New regression tests fail on the pre-fix code (400 ClientError on bad dates; fail-fast env sentinel; bounded-ctx DDL steps; commitment_type guard).
Issues created: #1073 (06-N3, closed by this PR), #1074 (collector heavy-lift follow-up, not closed here).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Apply gofmt struct-field alignment to a pre-existing formatting violation surfaced by the `pre-commit run --all-files` CI hook (the file is unrelated to this PR's analytics changes but blocks the gofmt hook from passing). Whitespace only; no behavior change.
…dget The 100k-cap observability guard added to Collect pushed its cyclomatic complexity to 13, over the gocyclo pre-commit threshold of 10. Extract the per-purchase aggregation loop into aggregatePurchases (Collect drops to 8, the helper is 7). Behaviour is unchanged: same Term<=0 skip, expiry skip, terminal context-cancellation handling, and bucket aggregation; the existing collector tests cover it.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/analytics/postgres_analytics_test.go (1)
216-232: ⚡ Quick winDrop the duplicated acquire-failure subtest.
Lines 202-214 already cover this exact pgxmock limitation. Keeping a second copy adds noise and makes future updates easy to drift.
🤖 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/analytics/postgres_analytics_test.go` around lines 216 - 232, Remove the duplicated subtest that asserts acquire-failure behavior: delete the t.Run(...) block whose name is "non-empty slice errors when connection acquire fails" which calls pgxmock.NewPool(), newTestableStore(...), and store.BulkInsertSnapshots(...); the same case is already covered earlier, so keep the original and remove this duplicate to avoid redundancy and drift.
🤖 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/analytics/postgres_analytics.go`:
- Around line 139-148: SaveSnapshot currently defers invalid commitment_type
checks to the DB; call validateCommitmentType(commitmentType) at the start of
SaveSnapshot (same guard used by BulkInsertSnapshots) and return the resulting
error so malformed values are rejected client-side with the same error behavior
as BulkInsertSnapshots; update SaveSnapshot to validate before any DB calls and
propagate the validation error immediately.
---
Nitpick comments:
In `@internal/analytics/postgres_analytics_test.go`:
- Around line 216-232: Remove the duplicated subtest that asserts
acquire-failure behavior: delete the t.Run(...) block whose name is "non-empty
slice errors when connection acquire fails" which calls pgxmock.NewPool(),
newTestableStore(...), and store.BulkInsertSnapshots(...); the same case is
already covered earlier, so keep the original and remove this duplicate to avoid
redundancy and drift.
🪄 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: 25e9156d-1e8e-44bd-90e3-6343055145a3
📒 Files selected for processing (10)
internal/analytics/collector.gointernal/analytics/postgres_analytics.gointernal/analytics/postgres_analytics_test.gointernal/api/handler_analytics.gointernal/api/handler_analytics_test.gointernal/database/postgres/migrations/000067_analytics_snapshot_correctness.down.sqlinternal/server/analytics_collect.gointernal/server/analytics_collect_test.gointernal/server/handler_coverage_test.gopkg/exchange/reshape_crossfamily_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/exchange/reshape_crossfamily_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/handler_analytics.go
- internal/server/handler_coverage_test.go
- internal/server/analytics_collect.go
- internal/analytics/collector.go
…uplicate test SaveSnapshot previously deferred commitment_type validation to the database CHECK constraint while BulkInsertSnapshots rejected invalid values client-side via validateCommitmentType. The two write paths now behave consistently. Also removes the duplicate "non-empty slice errors when connection acquire fails" subtest in TestBulkInsertSnapshots, which was an identical copy of the immediately preceding "returns error for non-empty slice in test mode" case. Regression test added to TestSaveSnapshot: asserts that a bogus CommitmentType returns an error before any DB call is made (no pgxmock expectations set, so a missed early return would surface as an unexpected-call error).
Member
Author
|
Addressed CR pass-2 findings:
Test count: 181 -> 186 (all pass). Build and vet clean. @coderabbitai review |
Contributor
|
Kicking off the next review pass now. 🐇✨ 🧠 Learnings used✅ Action performedReview finished.
|
…rates (#1049) Address review on the historical-savings collector: - Truncation: the collector pulled GetAllPurchaseHistory(ctx, 100000), a single capped page, then filtered expired rows in Go. As purchase_history grows, expired rows push older-but-still-active 1y/3y commitments past the cap and the snapshot silently undercounts. Add GetActivePurchaseHistory(ctx, asOf) which filters to the in-term set in SQL (term > 0 AND timestamp + term*8760h > asOf), bounding the read by live commitments; drop the cap and the warning. - Schedule-dependent units: snapshots stored an hourly rate but the views and QueryByProvider/QueryByService SUM(total_savings/commitment/usage), so a $720/mo commitment summed over ~30 daily snapshots read as ~$21,600 and changed with the schedule. Store a monthly run-rate per snapshot and AVG over time. Service-grained aggregations use a plain AVG; the cross-service views (daily_savings_trend, provider_savings_summary) keep the SUM across services and AVG across time via a nested aggregation. Updates the store interface + all mocks and the affected tests.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
8 tasks
4 tasks
4 tasks
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.
What this does
Finishes and wires up the dormant
internal/analyticssavings-snapshotsubsystem into a production, multi-tenant historical-savings time-series, and
fixes the latent data-correctness bugs that would have become live Criticals the
moment the collector flowed. Closes the finish-or-remove decision in favour of
finishing.
The snapshot series is distinct from (and complements) the existing
purchase_history-backed/history/analyticspath: it captures point-in-timecoverage / utilization / committed-spend / realized-savings that the on-demand
purchase aggregation cannot reconstruct after the fact.
Tenant-key decision
cloud_account_id(thecloud_accountsUUID FK) is the tenant scoping key forall snapshot writes and
Query*filters, mirroringpurchase_history/purchase_executions. The provider account string (account_id) is kept as adescriptive attribute. Both are written when available because either may be the
only one populated on a row (AWS ambient-creds / legacy rows carry no
cloud_account_id). Scoping uses the same dual-column OR predicate as the livepath (
accountFilterClause), socloud_account_id-NULL rows are still matched.Migration (000067)
000066is reserved by an in-flight PR, so this uses000067.account_idVARCHAR(20)->VARCHAR(255)(Azure subscription / GCPproject IDs) - H4.
NOT NULL/DEFAULT 0ontotal_usageandcoverage_percentagesoabsent metrics stay
NULL- H2.cloud_account_id(withCOALESCE-to-nil-UUID unique indexes forCONCURRENTLYrefresh) - H3.drop_old_savings_partitions'WHEN OTHERSto the parse error +surface
SQLERRM- M5/N4.account_iddeliberately to avoid truncation.
Latent bugs fixed
Term: rows withTerm <= 0are skipped (no+Inf/NaN).total_usageis derived from the realrecurring
MonthlyCostwhen present and leftNULLotherwise;coverage_percentageisNULL(no on-demand baseline inpurchase_history)rather than a placeholder 0 (
feedback_nullable_not_zero).make_interval(months => $1-1).commitment_typevalidatedbefore COPY so one bad row can't abort the batch.
Wiring
analytics_collect(both transports via the sharedHandleScheduledTaskdispatch, advisory-lock guarded): ensure upcoming monthlypartitions (M3) -> collect a snapshot across all tenants -> apply retention ->
refresh materialized views.
reinitializeAfterConnectvia one localized call tokeep the
app.gofootprint small for the in-flightapp.gorebase.enable_analytics_collect_schedule, dailydefault), mirroring the reap-stuck-purchases pattern. Container deploys reach
the task through the existing Cloud Scheduler / Logic Apps HTTP path.
ANALYTICS_COLLECTION_ENABLED/ANALYTICS_RETENTION_MONTHS(default 24) /
ANALYTICS_PARTITIONS_AHEAD(default 3), validated at startup(fail-fast).
API
GET /api/analytics/trendsreturns the monthly series + by-provider/by-servicebreakdowns, scoped to
allowed_accounts. Scope is enforced BEFORE resolvingfilter ids; a scoping lookup never degrades to
("",nil).Scoping approach
validateAnalyticsAccountScope(admin/unrestricted pass-through; scoped usersrequire an in-scope
account_id) thenresolveSingleAccountFilterIDsinto thedual-column inputs, matching
getRIExchangeHistory/getHistoryAnalytics.Tested vs needs-DB
covered via pgxmock against the REAL store (the parallel reimplementation that
could drift is removed).
go build ./...,go vet, andgo test ./internal/analytics/... ./internal/api/... ./internal/server/...all pass (2016 tests green in the touched packages; 2188 incl. database).
Term<=0no longer corruptsaggregates (no
+Inf/NaN); usage reflects realMonthlyCostand staysNULLwhen absent; distinctcloud_account_iddoes not merge;Query*scopesvia the dual-column clause; trends endpoint rejects out-of-scope/unscoped.
//go:build integrationDB suite was updated for the new signatures butneeds a real Postgres to run.
terraform fmt/validatenot run (Terraform not installed here); the newblocks mirror the existing reap-stuck-purchases resources exactly.
Follow-ups
/api/analytics/trendsis intentionallydeferred to keep this PR reviewable - filed as feat(frontend): Trends view consuming GET /api/analytics/trends #1048.
Closes #1023
Closes #1033
Scope expanded
This PR's pr-iterate pass also folded the remaining
internal/analyticsdata-layer review findings (FOLD-1049 bucket) and addressed the 5 CodeRabbit
actionables + 1 nitpick raised on the first review:
CodeRabbit actionables (commit
398079150)parseDateRangereturns a 400 ClientError for malformed/reversed dates(was mapped to HTTP 500). Fixes the trends endpoint and both
/history/*callers at the source.
000067down: refresh materialized views AFTER restoring theNOT NULL/default columns so the recreated views reflect coerced data.
LoadAnalyticsConfigpreserves a malformedANALYTICS_RETENTION_MONTHS/ANALYTICS_PARTITIONS_AHEADas aValidate-failing sentinel (fail-fast) instead of silently defaulting.purchase_historyfetch hits its capso a potential undercount is observable.
validateCommitmentTypeso the COPY-path guard is unit-testable;fixed the misleading bulk-insert subtest name (nitpick).
Folded data-layer finding (commit
cbaa51fe1)refresh) now runs under a bounded per-step context, so a runaway statement
cannot hang the scheduled run when no
statement_timeoutis enforced(RDS Proxy).
The other FOLD-1049 findings (06-M1/M3/M5/L3/L4/N1/N4) were already implemented
by this PR; 06-M2 is a parameterized-query style note with no injection surface
(skipped). The two CodeRabbit "heavy lift" items (100k history cap -> paginated
read; schedule-dependent persisted units) need a design decision and are tracked
as a follow-up.
Also closes #1073
🤖 Generated with claude-flow
Summary by CodeRabbit
New Features
Improvements
Bug Fixes