diff --git a/internal/analytics/collector.go b/internal/analytics/collector.go index 2ba9dc1a7..aaaff5f1f 100644 --- a/internal/analytics/collector.go +++ b/internal/analytics/collector.go @@ -1,8 +1,9 @@ -// Package analytics provides the hourly collector for savings data. +// Package analytics provides the scheduled collector for savings time-series data. package analytics import ( "context" + "errors" "fmt" "log" "time" @@ -10,26 +11,27 @@ import ( "github.com/LeanerCloud/CUDly/internal/config" ) -// Constants for time calculations +// Constants for time calculations. const ( - // HoursPerYear is the approximate number of hours in a year (365 days) + // HoursPerYear is the approximate number of hours in a year (365 days). HoursPerYear = 365 * 24 - // HoursPerMonth is the approximate number of hours in a month (30 days) - HoursPerMonth = 30 * 24 + // MonthsPerYear amortizes an upfront commitment cost into a monthly run-rate + // over the commitment term (term is expressed in years). + MonthsPerYear = 12 ) -// Collector aggregates savings data and writes it to PostgreSQL for analytics. +// Collector aggregates savings data and writes point-in-time snapshots to +// PostgreSQL for the historical-savings analytics time-series. It runs on a +// schedule (see server.handleCollectAnalytics) across all tenants. type Collector struct { store AnalyticsStore configStore config.StoreInterface - accountID string } // CollectorConfig holds configuration for the collector. type CollectorConfig struct { AnalyticsStore AnalyticsStore - AccountID string } // NewCollector creates a new savings collector. @@ -40,117 +42,194 @@ func NewCollector(cfg CollectorConfig, configStore config.StoreInterface) (*Coll if configStore == nil { return nil, fmt.Errorf("config store is required") } - return &Collector{ store: cfg.AnalyticsStore, configStore: configStore, - accountID: cfg.AccountID, }, nil } -// aggregateData holds aggregated savings data for a service/provider/region combination +// aggregateData holds aggregated savings data for one +// (cloud_account_id|account_id|service|provider|region|commitment_type) bucket. type aggregateData struct { - service string - provider string - region string - commitment float64 + accountID string + cloudAccountID *string + service string + provider string + region string + commitmentType string + commitment float64 + // usage accumulates the recurring (monthly) cost of the commitments in this + // bucket as the covered-usage proxy. usageKnown stays false until at least + // one contributing row carried a non-nil MonthlyCost, so a bucket made up + // entirely of all-upfront commitments writes NULL usage rather than 0 + // (feedback_nullable_not_zero). usage float64 + usageKnown bool savings float64 count int } -// Collect aggregates current savings data and writes it to PostgreSQL. -// This should be called hourly by EventBridge scheduled rule. +// aggKey is the bucket identity. cloudAccountID is dereferenced (or "" when +// nil) so two rows for the same provider account but differing UUID-vs-NULL +// don't merge across the tenant boundary. +func aggKey(p config.PurchaseHistoryRecord, commitmentType string) string { + cloud := "" + if p.CloudAccountID != nil { + cloud = *p.CloudAccountID + } + return fmt.Sprintf("%s|%s|%s|%s|%s|%s", cloud, p.AccountID, p.Service, p.Provider, p.Region, commitmentType) +} + +// Collect aggregates current savings data across all tenants and writes a +// snapshot row per bucket. Intended to be called on a schedule. func (c *Collector) Collect(ctx context.Context) error { - log.Printf("Analytics collector: Starting hourly collection for account %s", c.accountID) + log.Printf("Analytics collector: starting collection") + + now := time.Now().UTC() - // Get recent purchase history to calculate current savings - purchases, err := c.configStore.GetPurchaseHistory(ctx, c.accountID, 1000) + // Active-only read: the active filter is pushed into SQL so the result is + // bounded by the number of live commitments rather than by all history ever + // recorded. This avoids silently truncating older-but-still-active 1y/3y + // commitments the way a single capped all-history page did. + purchases, err := c.configStore.GetActivePurchaseHistory(ctx, now) if err != nil { - return fmt.Errorf("failed to get purchase history: %w", err) + return fmt.Errorf("failed to get active purchase history: %w", err) } - log.Printf("Analytics collector: Processing %d purchases", len(purchases)) + log.Printf("Analytics collector: processing %d active purchases", len(purchases)) - // Calculate savings from purchases - now := time.Now().UTC() + serviceMap, activePurchases, skippedBadTerm, err := aggregatePurchases(ctx, purchases, now) + if err != nil { + return err + } - // Aggregate savings by service, provider, region - serviceMap := make(map[string]*aggregateData) + log.Printf("Analytics collector: %d active, %d unique buckets, %d skipped (term<=0)", + activePurchases, len(serviceMap), skippedBadTerm) + + snapshots := buildSnapshots(serviceMap, now) + if len(snapshots) == 0 { + log.Printf("Analytics collector: no active purchases to snapshot") + return nil + } + + if err := c.store.BulkInsertSnapshots(ctx, snapshots); err != nil { + // Surface context cancellation distinctly so the caller doesn't retry a + // genuinely cancelled run as a transient failure. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("collection cancelled during write: %w", err) + } + return fmt.Errorf("failed to save snapshots: %w", err) + } + + log.Printf("Analytics collector: saved %d snapshots", len(snapshots)) + return nil +} + +// aggregatePurchases folds the active purchases into a per-bucket aggregation +// map keyed by aggKey. It skips Term<=0 rows (H1, counted as skippedBadTerm) and +// expired commitments, and treats context cancellation as terminal so a partial +// snapshot set is never written (feedback_ctx_cancel_terminal). Extracted from +// Collect to keep each function within the cyclomatic-complexity budget. +func aggregatePurchases(ctx context.Context, purchases []config.PurchaseHistoryRecord, now time.Time) (serviceMap map[string]*aggregateData, activePurchases, skippedBadTerm int, err error) { + serviceMap = make(map[string]*aggregateData) - // Process each purchase to calculate active savings - activePurchases := 0 for _, p := range purchases { - // Check if purchase is still active (within term) - purchaseTime := p.Timestamp - termDuration := time.Duration(p.Term*HoursPerYear) * time.Hour - expiryTime := purchaseTime.Add(termDuration) + if err := ctx.Err(); err != nil { + return nil, 0, 0, fmt.Errorf("collection cancelled after %d rows: %w", activePurchases, err) + } - if now.After(expiryTime) { - continue // Skip expired purchases + // H1: a Term <= 0 row would make the amortized-commitment division + // (UpfrontCost / (Term*MonthsPerYear)) produce +Inf/NaN, which then + // poisons every downstream SUM/AVG and errors at the DECIMAL bind. + // Skip it and count it for observability rather than feeding a zero + // denominator into the division. + if p.Term <= 0 { + skippedBadTerm++ + continue } + // Skip expired commitments (outside their term window). + expiryTime := p.Timestamp.Add(time.Duration(p.Term*HoursPerYear) * time.Hour) + if now.After(expiryTime) { + continue + } activePurchases++ - // Create unique key for this combination (service|provider|region) - key := fmt.Sprintf("%s|%s|%s", p.Service, p.Provider, p.Region) + commitmentType := commitmentTypeFor(p.Service) + key := aggKey(p, commitmentType) - if serviceMap[key] == nil { - serviceMap[key] = &aggregateData{ - service: p.Service, - provider: p.Provider, - region: p.Region, + agg := serviceMap[key] + if agg == nil { + agg = &aggregateData{ + accountID: p.AccountID, + cloudAccountID: p.CloudAccountID, + service: p.Service, + provider: p.Provider, + region: p.Region, + commitmentType: commitmentType, } + serviceMap[key] = agg } - agg := serviceMap[key] - - // Calculate hourly savings rate for this purchase - // EstimatedSavings is typically monthly, convert to hourly - hourlySavings := p.EstimatedSavings / HoursPerMonth - - agg.savings += hourlySavings - agg.commitment += p.UpfrontCost / (float64(p.Term) * HoursPerYear) // Amortized hourly + // Monthly savings run-rate (EstimatedSavings is already monthly). Stored as + // a point-in-time run-rate, not an accrued total, so the monthly trend AVGs + // snapshots and stays invariant to the collection schedule (daily vs hourly). + agg.savings += p.EstimatedSavings + // Upfront commitment amortized to a monthly run-rate over the term. + // Term > 0 guaranteed above. + agg.commitment += p.UpfrontCost / (float64(p.Term) * MonthsPerYear) + // H2: real covered usage from the recurring monthly cost when present. + // Nil MonthlyCost (e.g. AWS all-upfront) contributes nothing and leaves + // usage unknown rather than implicitly $0. + if p.MonthlyCost != nil { + agg.usage += *p.MonthlyCost + agg.usageKnown = true + } agg.count++ } - log.Printf("Analytics collector: Found %d active purchases, %d unique combinations", activePurchases, len(serviceMap)) + return serviceMap, activePurchases, skippedBadTerm, nil +} + +// commitmentTypeFor maps a service to its commitment_type. SavingsPlans is the +// only Savings Plan service today; everything else is a Reserved Instance. +func commitmentTypeFor(service string) string { + if service == "SavingsPlans" { + return "SavingsPlan" + } + return "RI" +} - // Build snapshots for all active combinations and bulk-insert in one round-trip. +// buildSnapshots converts the aggregation map into snapshot rows. total_usage is +// nil when no contributing row carried a recurring cost; coverage_percentage is +// nil because purchase_history carries no on-demand baseline to derive it from +// (writing a placeholder 0 would corrupt AVG, per feedback_nullable_not_zero). +func buildSnapshots(serviceMap map[string]*aggregateData, now time.Time) []SavingsSnapshot { snapshots := make([]SavingsSnapshot, 0, len(serviceMap)) for _, agg := range serviceMap { - commitmentType := "RI" - if agg.service == "SavingsPlans" { - commitmentType = "SavingsPlan" + var usage *float64 + if agg.usageKnown { + u := agg.usage + usage = &u } snapshots = append(snapshots, SavingsSnapshot{ - AccountID: c.accountID, + AccountID: agg.accountID, + CloudAccountID: agg.cloudAccountID, Timestamp: now, Provider: agg.provider, Service: agg.service, Region: agg.region, - CommitmentType: commitmentType, + CommitmentType: agg.commitmentType, TotalCommitment: agg.commitment, - TotalUsage: 0, // TODO: Can be calculated from CloudWatch if needed + TotalUsage: usage, TotalSavings: agg.savings, - CoveragePercentage: 0, // TODO: Calculate from usage data if needed + CoveragePercentage: nil, Metadata: map[string]any{ "active_purchases": agg.count, "collection_time": now.Format(time.RFC3339), }, }) } - - if len(snapshots) == 0 { - log.Printf("Analytics collector: No active purchases to snapshot") - return nil - } - - if err := c.store.BulkInsertSnapshots(ctx, snapshots); err != nil { - return fmt.Errorf("failed to save snapshots: %w", err) - } - - log.Printf("Analytics collector: Successfully saved %d snapshots", len(snapshots)) - return nil + return snapshots } diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 38d8fc5ae..a467cb23d 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -3,6 +3,7 @@ package analytics import ( "context" "errors" + "math" "testing" "time" @@ -17,10 +18,11 @@ type mockAnalyticsStore struct { saveSnapshotFunc func(ctx context.Context, snapshot *SavingsSnapshot) error bulkInsertSnapshotsFunc func(ctx context.Context, snapshots []SavingsSnapshot) error querySavingsFunc func(ctx context.Context, req QueryRequest) ([]SavingsSnapshot, error) - queryMonthlyTotalsFunc func(ctx context.Context, accountID string, months int) ([]MonthlySummary, error) - queryByProviderFunc func(ctx context.Context, accountID string, startDate, endDate time.Time) ([]ProviderBreakdown, error) - queryByServiceFunc func(ctx context.Context, accountID string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) + queryMonthlyTotalsFunc func(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, months int) ([]MonthlySummary, error) + queryByProviderFunc func(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, startDate, endDate time.Time) ([]ProviderBreakdown, error) + queryByServiceFunc func(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) createPartitionFunc func(ctx context.Context, forMonth time.Time) error + createFuturePartitionsFunc func(ctx context.Context, monthsAhead int) error dropOldPartitionsFunc func(ctx context.Context, retentionMonths int) error createPartitionsForRangeFunc func(ctx context.Context, startDate, endDate time.Time) error refreshMaterializedViewsFunc func(ctx context.Context) error @@ -52,23 +54,23 @@ func (m *mockAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequest) return nil, nil } -func (m *mockAnalyticsStore) QueryMonthlyTotals(ctx context.Context, accountID string, months int) ([]MonthlySummary, error) { +func (m *mockAnalyticsStore) QueryMonthlyTotals(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, months int) ([]MonthlySummary, error) { if m.queryMonthlyTotalsFunc != nil { - return m.queryMonthlyTotalsFunc(ctx, accountID, months) + return m.queryMonthlyTotalsFunc(ctx, accountUUIDs, accountExternalIDsByProvider, months) } return nil, nil } -func (m *mockAnalyticsStore) QueryByProvider(ctx context.Context, accountID string, startDate, endDate time.Time) ([]ProviderBreakdown, error) { +func (m *mockAnalyticsStore) QueryByProvider(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, startDate, endDate time.Time) ([]ProviderBreakdown, error) { if m.queryByProviderFunc != nil { - return m.queryByProviderFunc(ctx, accountID, startDate, endDate) + return m.queryByProviderFunc(ctx, accountUUIDs, accountExternalIDsByProvider, startDate, endDate) } return nil, nil } -func (m *mockAnalyticsStore) QueryByService(ctx context.Context, accountID string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) { +func (m *mockAnalyticsStore) QueryByService(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) { if m.queryByServiceFunc != nil { - return m.queryByServiceFunc(ctx, accountID, provider, startDate, endDate) + return m.queryByServiceFunc(ctx, accountUUIDs, accountExternalIDsByProvider, provider, startDate, endDate) } return nil, nil } @@ -80,6 +82,13 @@ func (m *mockAnalyticsStore) CreatePartition(ctx context.Context, forMonth time. return nil } +func (m *mockAnalyticsStore) CreateFuturePartitions(ctx context.Context, monthsAhead int) error { + if m.createFuturePartitionsFunc != nil { + return m.createFuturePartitionsFunc(ctx, monthsAhead) + } + return nil +} + func (m *mockAnalyticsStore) DropOldPartitions(ctx context.Context, retentionMonths int) error { if m.dropOldPartitionsFunc != nil { return m.dropOldPartitionsFunc(ctx, retentionMonths) @@ -110,7 +119,9 @@ func (m *mockAnalyticsStore) Close() error { // mockConfigStore implements config.StoreInterface for testing type mockConfigStore struct { - getPurchaseHistoryFunc func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) + getPurchaseHistoryFunc func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) + getAllPurchaseHistoryFunc func(ctx context.Context, limit int) ([]config.PurchaseHistoryRecord, error) + getActivePurchaseHistoryFunc func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) } func (m *mockConfigStore) GetGlobalConfig(ctx context.Context) (*config.GlobalConfig, error) { @@ -197,6 +208,16 @@ func (m *mockConfigStore) GetPurchaseHistory(ctx context.Context, accountID stri } func (m *mockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) ([]config.PurchaseHistoryRecord, error) { + if m.getAllPurchaseHistoryFunc != nil { + return m.getAllPurchaseHistoryFunc(ctx, limit) + } + return nil, nil +} + +func (m *mockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + if m.getActivePurchaseHistoryFunc != nil { + return m.getActivePurchaseHistoryFunc(ctx, asOf) + } return nil, nil } @@ -359,516 +380,285 @@ func (m *mockConfigStore) UpsertRIUtilizationCache(_ context.Context, _ string, return nil } +// strPtr is a test helper for *string fields. +func strPtr(s string) *string { return &s } + +// activeRecord returns a still-active purchase made 3 months ago with the given +// fields; helper to keep the table-driven tests terse. +func activeRecord(provider, service, region string, term int, savings, upfront float64) config.PurchaseHistoryRecord { + return config.PurchaseHistoryRecord{ + AccountID: "123456789012", + PurchaseID: "p-" + service + "-" + region, + Timestamp: time.Now().AddDate(0, -3, 0), + Provider: provider, + Service: service, + Region: region, + Term: term, + EstimatedSavings: savings, + UpfrontCost: upfront, + } +} + +func newTestCollector(t *testing.T, store *mockAnalyticsStore, cfgStore *mockConfigStore) *Collector { + t.Helper() + collector, err := NewCollector(CollectorConfig{AnalyticsStore: store}, cfgStore) + require.NoError(t, err) + return collector +} + // TestNewCollector tests the NewCollector function func TestNewCollector(t *testing.T) { t.Run("returns error when analytics store is nil", func(t *testing.T) { - cfg := CollectorConfig{ - AnalyticsStore: nil, - AccountID: "test-account", - } - configStore := &mockConfigStore{} - - collector, err := NewCollector(cfg, configStore) - + collector, err := NewCollector(CollectorConfig{AnalyticsStore: nil}, &mockConfigStore{}) assert.Nil(t, collector) - assert.Error(t, err) + require.Error(t, err) assert.Contains(t, err.Error(), "analytics store is required") }) t.Run("returns error when config store is nil", func(t *testing.T) { - cfg := CollectorConfig{ - AnalyticsStore: &mockAnalyticsStore{}, - AccountID: "test-account", - } - - collector, err := NewCollector(cfg, nil) - + collector, err := NewCollector(CollectorConfig{AnalyticsStore: &mockAnalyticsStore{}}, nil) assert.Nil(t, collector) - assert.Error(t, err) + require.Error(t, err) assert.Contains(t, err.Error(), "config store is required") }) t.Run("creates collector successfully with valid inputs", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - configStore := &mockConfigStore{} - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account-123", - } - - collector, err := NewCollector(cfg, configStore) - + collector, err := NewCollector(CollectorConfig{AnalyticsStore: &mockAnalyticsStore{}}, &mockConfigStore{}) require.NoError(t, err) assert.NotNil(t, collector) - assert.Equal(t, "test-account-123", collector.accountID) - }) - - t.Run("creates collector with empty account ID", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - configStore := &mockConfigStore{} - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "", - } - - collector, err := NewCollector(cfg, configStore) - - require.NoError(t, err) - assert.NotNil(t, collector) - assert.Equal(t, "", collector.accountID) }) } // TestCollectorCollect tests the Collect method func TestCollectorCollect(t *testing.T) { - t.Run("returns error when GetPurchaseHistory fails", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { + t.Run("returns error when GetAllPurchaseHistory fails", func(t *testing.T) { + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { return nil, errors.New("database connection failed") }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to get purchase history") + err := newTestCollector(t, store, cfgStore).Collect(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to get active purchase history") assert.Contains(t, err.Error(), "database connection failed") }) t.Run("handles empty purchase history", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - assert.Empty(t, analyticsStore.savedSnapshots) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + assert.Empty(t, store.savedSnapshots) }) t.Run("skips expired purchases", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - // Create a purchase that expired 2 years ago - expiredTime := time.Now().AddDate(-3, 0, 0) // 3 years ago - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: expiredTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 1, // 1 year term, so expired - EstimatedSavings: 100.0, - UpfrontCost: 500.0, - }, - }, nil - }, - } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", + store := &mockAnalyticsStore{} + expired := config.PurchaseHistoryRecord{ + AccountID: "123456789012", Timestamp: time.Now().AddDate(-3, 0, 0), + Provider: "aws", Service: "rds", Region: "us-east-1", + Term: 1, EstimatedSavings: 100, UpfrontCost: 500, } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - assert.Empty(t, analyticsStore.savedSnapshots) - }) - - t.Run("processes active purchases and creates snapshots", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - // Create an active purchase (purchased 6 months ago with 1 year term) - activeTime := time.Now().AddDate(0, -6, 0) - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - ResourceType: "db.m5.large", - Term: 1, // 1 year term - EstimatedSavings: 720.0, // Monthly savings - UpfrontCost: 1000.0, - }, - }, nil + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{expired}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - require.Len(t, analyticsStore.savedSnapshots, 1) - - snapshot := analyticsStore.savedSnapshots[0] - assert.Equal(t, "test-account", snapshot.AccountID) - assert.Equal(t, "aws", snapshot.Provider) - assert.Equal(t, "rds", snapshot.Service) - assert.Equal(t, "us-east-1", snapshot.Region) - assert.Equal(t, "RI", snapshot.CommitmentType) - assert.Greater(t, snapshot.TotalSavings, 0.0) - assert.Greater(t, snapshot.TotalCommitment, 0.0) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + assert.Empty(t, store.savedSnapshots) }) - t.Run("aggregates multiple purchases for same service/provider/region", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - activeTime := time.Now().AddDate(0, -3, 0) - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 1, - EstimatedSavings: 100.0, - UpfrontCost: 500.0, - }, - { - AccountID: "test-account", - PurchaseID: "purchase-2", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 1, - EstimatedSavings: 200.0, - UpfrontCost: 1000.0, - }, - }, nil + t.Run("processes active purchases and creates snapshots", func(t *testing.T) { + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, 720, 1000)}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - // Should only create one snapshot for the aggregated data - require.Len(t, analyticsStore.savedSnapshots, 1) - - snapshot := analyticsStore.savedSnapshots[0] - // Verify metadata shows 2 active purchases - assert.Equal(t, 2, snapshot.Metadata["active_purchases"]) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + require.Len(t, store.savedSnapshots, 1) + s := store.savedSnapshots[0] + assert.Equal(t, "123456789012", s.AccountID) + assert.Equal(t, "aws", s.Provider) + assert.Equal(t, "rds", s.Service) + assert.Equal(t, "RI", s.CommitmentType) + assert.Greater(t, s.TotalSavings, 0.0) + assert.Greater(t, s.TotalCommitment, 0.0) }) - t.Run("creates separate snapshots for different regions", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - activeTime := time.Now().AddDate(0, -3, 0) - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { + t.Run("aggregates multiple purchases for same bucket", func(t *testing.T) { + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 1, - EstimatedSavings: 100.0, - UpfrontCost: 500.0, - }, - { - AccountID: "test-account", - PurchaseID: "purchase-2", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-west-2", - Term: 1, - EstimatedSavings: 150.0, - UpfrontCost: 700.0, - }, + activeRecord("aws", "rds", "us-east-1", 1, 100, 500), + activeRecord("aws", "rds", "us-east-1", 1, 200, 1000), }, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - assert.Len(t, analyticsStore.savedSnapshots, 2) - - regions := make(map[string]bool) - for _, s := range analyticsStore.savedSnapshots { - regions[s.Region] = true - } - assert.True(t, regions["us-east-1"]) - assert.True(t, regions["us-west-2"]) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + require.Len(t, store.savedSnapshots, 1) + assert.Equal(t, 2, store.savedSnapshots[0].Metadata["active_purchases"]) }) - t.Run("creates separate snapshots for different providers", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - activeTime := time.Now().AddDate(0, -3, 0) - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { + t.Run("creates separate snapshots per region and provider", func(t *testing.T) { + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 1, - EstimatedSavings: 100.0, - UpfrontCost: 500.0, - }, - { - AccountID: "test-account", - PurchaseID: "purchase-2", - Timestamp: activeTime, - Provider: "gcp", - Service: "cloudsql", - Region: "us-east1", - Term: 1, - EstimatedSavings: 150.0, - UpfrontCost: 700.0, - }, + activeRecord("aws", "rds", "us-east-1", 1, 100, 500), + activeRecord("aws", "rds", "us-west-2", 1, 150, 700), + activeRecord("gcp", "cloudsql", "us-east1", 1, 150, 700), }, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - assert.Len(t, analyticsStore.savedSnapshots, 2) - - providers := make(map[string]bool) - for _, s := range analyticsStore.savedSnapshots { - providers[s.Provider] = true - } - assert.True(t, providers["aws"]) - assert.True(t, providers["gcp"]) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + assert.Len(t, store.savedSnapshots, 3) }) t.Run("sets SavingsPlan commitment type for SavingsPlans service", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - activeTime := time.Now().AddDate(0, -3, 0) - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "SavingsPlans", - Region: "us-east-1", - Term: 1, - EstimatedSavings: 500.0, - UpfrontCost: 2000.0, - }, - }, nil + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{activeRecord("aws", "SavingsPlans", "us-east-1", 1, 500, 2000)}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - require.Len(t, analyticsStore.savedSnapshots, 1) - assert.Equal(t, "SavingsPlan", analyticsStore.savedSnapshots[0].CommitmentType) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + require.Len(t, store.savedSnapshots, 1) + assert.Equal(t, "SavingsPlan", store.savedSnapshots[0].CommitmentType) }) t.Run("returns error when BulkInsertSnapshots fails", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{ + store := &mockAnalyticsStore{ bulkInsertSnapshotsFunc: func(ctx context.Context, snapshots []SavingsSnapshot) error { return errors.New("bulk insert failed") }, } - activeTime := time.Now().AddDate(0, -3, 0) - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 1, - EstimatedSavings: 100.0, - UpfrontCost: 500.0, - }, - }, nil + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, 100, 500)}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.Error(t, err) + err := newTestCollector(t, store, cfgStore).Collect(context.Background()) + require.Error(t, err) assert.Contains(t, err.Error(), "failed to save snapshots") }) - t.Run("handles 3-year term purchases correctly", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - // Purchase made 2 years ago with 3-year term (still active) - activeTime := time.Now().AddDate(-2, 0, 0) - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 3, // 3 year term - EstimatedSavings: 1000.0, - UpfrontCost: 5000.0, - }, - }, nil + t.Run("calculates monthly savings run-rate and amortized commitment correctly", func(t *testing.T) { + store := &mockAnalyticsStore{} + const monthlySavings, upfront = 720.0, 8760.0 + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, monthlySavings, upfront)}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) - - assert.NoError(t, err) - require.Len(t, analyticsStore.savedSnapshots, 1) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + require.Len(t, store.savedSnapshots, 1) + assert.InDelta(t, monthlySavings, store.savedSnapshots[0].TotalSavings, 0.001) + assert.InDelta(t, upfront/(1*MonthsPerYear), store.savedSnapshots[0].TotalCommitment, 0.001) }) - t.Run("calculates hourly savings correctly", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - activeTime := time.Now().AddDate(0, -1, 0) // 1 month ago - monthlySavings := 720.0 // $720/month - expectedHourlySavings := monthlySavings / HoursPerMonth + // ── Regression tests for the latent data bugs (#1023) ── - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: 1, - EstimatedSavings: monthlySavings, - UpfrontCost: 0, - }, - }, nil + t.Run("H1: Term<=0 row is skipped and does not corrupt aggregates", func(t *testing.T) { + store := &mockAnalyticsStore{} + good := activeRecord("aws", "rds", "us-east-1", 1, 100, 500) + badTerm := activeRecord("aws", "rds", "us-east-1", 0, 999, 999) // Term==0 -> +Inf commitment pre-fix + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{good, badTerm}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", - } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) - - err = collector.Collect(context.Background()) + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + require.Len(t, store.savedSnapshots, 1) + s := store.savedSnapshots[0] + // Only the good row contributed: no +Inf/NaN, exactly one active purchase. + assert.Equal(t, 1, s.Metadata["active_purchases"]) + assert.False(t, math.IsInf(s.TotalCommitment, 0), "commitment must not be Inf") + assert.False(t, math.IsNaN(s.TotalCommitment), "commitment must not be NaN") + assert.InDelta(t, 500.0/(1*MonthsPerYear), s.TotalCommitment, 0.001) + }) - assert.NoError(t, err) - require.Len(t, analyticsStore.savedSnapshots, 1) - assert.InDelta(t, expectedHourlySavings, analyticsStore.savedSnapshots[0].TotalSavings, 0.001) + t.Run("H1: a negative Term is also skipped", func(t *testing.T) { + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", -1, 100, 500)}, nil + }, + } + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + assert.Empty(t, store.savedSnapshots, "a negative-term-only history yields no snapshot") }) - t.Run("calculates amortized hourly commitment correctly", func(t *testing.T) { - analyticsStore := &mockAnalyticsStore{} - activeTime := time.Now().AddDate(0, -1, 0) - upfrontCost := 8760.0 // $8760 for 1 year - term := 1 - expectedHourlyCommitment := upfrontCost / (float64(term) * HoursPerYear) // Should be $1/hour + t.Run("H2: usage reflects real MonthlyCost; absent stays NULL not 0", func(t *testing.T) { + store := &mockAnalyticsStore{} + withCost := activeRecord("aws", "rds", "us-east-1", 1, 100, 500) + withCost.MonthlyCost = func() *float64 { v := 360.0; return &v }() // $360/mo + noCost := activeRecord("aws", "ec2", "us-east-1", 1, 100, 500) // MonthlyCost nil - configStore := &mockConfigStore{ - getPurchaseHistoryFunc: func(ctx context.Context, accountID string, limit int) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{ - { - AccountID: "test-account", - PurchaseID: "purchase-1", - Timestamp: activeTime, - Provider: "aws", - Service: "rds", - Region: "us-east-1", - Term: term, - EstimatedSavings: 100.0, - UpfrontCost: upfrontCost, - }, - }, nil + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{withCost, noCost}, nil }, } - cfg := CollectorConfig{ - AnalyticsStore: analyticsStore, - AccountID: "test-account", + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + require.Len(t, store.savedSnapshots, 2) + + byService := map[string]SavingsSnapshot{} + for _, s := range store.savedSnapshots { + byService[s.Service] = s } - collector, err := NewCollector(cfg, configStore) - require.NoError(t, err) + // rds carried a recurring cost -> real, non-nil usage (monthly run-rate). + require.NotNil(t, byService["rds"].TotalUsage) + assert.InDelta(t, 360.0, *byService["rds"].TotalUsage, 0.001) + // ec2 had no recurring cost -> usage is NULL (nil), never a placeholder 0. + assert.Nil(t, byService["ec2"].TotalUsage) + // coverage is never a placeholder 0 (no on-demand baseline source). + assert.Nil(t, byService["rds"].CoveragePercentage) + assert.Nil(t, byService["ec2"].CoveragePercentage) + }) - err = collector.Collect(context.Background()) + t.Run("H3: cloud_account_id is populated and partitions the tenant boundary", func(t *testing.T) { + store := &mockAnalyticsStore{} + tenantA := activeRecord("aws", "rds", "us-east-1", 1, 100, 500) + tenantA.CloudAccountID = strPtr("11111111-1111-1111-1111-111111111111") + tenantB := activeRecord("aws", "rds", "us-east-1", 1, 200, 800) + tenantB.CloudAccountID = strPtr("22222222-2222-2222-2222-222222222222") + // Same provider account string, different cloud_account_id -> must NOT merge. + tenantB.AccountID = tenantA.AccountID + + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{tenantA, tenantB}, nil + }, + } + require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) + require.Len(t, store.savedSnapshots, 2, "distinct cloud_account_id must not be merged") + ids := map[string]bool{} + for _, s := range store.savedSnapshots { + require.NotNil(t, s.CloudAccountID) + ids[*s.CloudAccountID] = true + } + assert.True(t, ids["11111111-1111-1111-1111-111111111111"]) + assert.True(t, ids["22222222-2222-2222-2222-222222222222"]) + }) - assert.NoError(t, err) - require.Len(t, analyticsStore.savedSnapshots, 1) - assert.InDelta(t, expectedHourlyCommitment, analyticsStore.savedSnapshots[0].TotalCommitment, 0.001) + t.Run("ctx cancellation is terminal and surfaces an error", func(t *testing.T) { + store := &mockAnalyticsStore{} + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{activeRecord("aws", "rds", "us-east-1", 1, 100, 500)}, nil + }, + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + err := newTestCollector(t, store, cfgStore).Collect(ctx) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + assert.Empty(t, store.savedSnapshots, "no snapshots written on a cancelled run") }) } @@ -879,9 +669,8 @@ func TestConstants(t *testing.T) { assert.Equal(t, 8760, HoursPerYear) }) - t.Run("HoursPerMonth is correct", func(t *testing.T) { - assert.Equal(t, 30*24, HoursPerMonth) - assert.Equal(t, 720, HoursPerMonth) + t.Run("MonthsPerYear is correct", func(t *testing.T) { + assert.Equal(t, 12, MonthsPerYear) }) } diff --git a/internal/analytics/interfaces.go b/internal/analytics/interfaces.go index b3da10399..ca847e978 100644 --- a/internal/analytics/interfaces.go +++ b/internal/analytics/interfaces.go @@ -5,83 +5,111 @@ import ( "time" ) -// SavingsSnapshot represents a single savings data point +// SavingsSnapshot represents a single savings data point. +// +// Tenant key: CloudAccountID (the cloud_accounts UUID FK) is the multi-tenant +// scoping key, mirroring purchase_history / purchase_executions. AccountID is +// the cloud-provider external account string (AWS account number, Azure +// subscription id, GCP project id), kept as a descriptive attribute. A row may +// carry only one of them populated (CloudAccountID is NULL on the AWS ambient- +// credentials path and on legacy rows), so both are written when available. type SavingsSnapshot struct { - ID string `json:"id"` - AccountID string `json:"account_id"` - Timestamp time.Time `json:"timestamp"` - Provider string `json:"provider"` - Service string `json:"service"` - Region string `json:"region"` - CommitmentType string `json:"commitment_type"` // "RI" or "SavingsPlan" - TotalCommitment float64 `json:"total_commitment"` - TotalUsage float64 `json:"total_usage"` - TotalSavings float64 `json:"total_savings"` - CoveragePercentage float64 `json:"coverage_percentage"` + ID string `json:"id"` + AccountID string `json:"account_id"` + // CloudAccountID is the cloud_accounts UUID FK and the tenant key. Nil when + // the source row had no cloud_account_id (AWS ambient creds / legacy rows). + CloudAccountID *string `json:"cloud_account_id,omitempty"` + Timestamp time.Time `json:"timestamp"` + Provider string `json:"provider"` + Service string `json:"service"` + Region string `json:"region"` + CommitmentType string `json:"commitment_type"` // "RI" or "SavingsPlan" + TotalCommitment float64 `json:"total_commitment"` + // TotalUsage is the on-demand-equivalent recurring spend the commitments in + // this bucket cover. Nil when the source data carried no recurring/monthly + // cost (e.g. AWS all-upfront), so AVG/SUM skip it instead of being dragged + // toward zero (project rule feedback_nullable_not_zero). + TotalUsage *float64 `json:"total_usage,omitempty"` + TotalSavings float64 `json:"total_savings"` + // CoveragePercentage is committed spend / total eligible (on-demand) spend. + // Nil when no on-demand baseline was available to compute it; never a + // placeholder 0 (feedback_nullable_not_zero). + CoveragePercentage *float64 `json:"coverage_percentage,omitempty"` Metadata map[string]any `json:"metadata,omitempty"` } -// QueryRequest defines parameters for querying savings data +// QueryRequest defines parameters for querying savings data. +// +// Scoping uses the same dual-column model as the live purchase_history path: +// rows match when cloud_account_id = ANY(AccountUUIDs) OR (provider = p AND +// account_id = ANY(AccountExternalIDsByProvider[p])). Both nil/empty means +// "all accounts accessible to the caller" — the caller MUST enforce scoping +// upstream before passing empty filters. type QueryRequest struct { - AccountID string - Provider string // optional filter - Service string // optional filter - StartDate time.Time - EndDate time.Time - Limit int + AccountUUIDs []string + AccountExternalIDsByProvider map[string][]string + Provider string // optional filter + Service string // optional filter + StartDate time.Time + EndDate time.Time + Limit int } -// MonthlySummary represents aggregated monthly savings +// MonthlySummary represents aggregated monthly savings. type MonthlySummary struct { - Month time.Time `json:"month"` - AccountID string `json:"account_id"` - Provider string `json:"provider"` - Service string `json:"service"` - TotalSavings float64 `json:"total_savings"` - AvgCoverage float64 `json:"avg_coverage"` - SnapshotCount int `json:"snapshot_count"` + Month time.Time `json:"month"` + AccountID string `json:"account_id"` + CloudAccountID *string `json:"cloud_account_id,omitempty"` + Provider string `json:"provider"` + Service string `json:"service"` + TotalSavings float64 `json:"total_savings"` + // AvgCoverage is nil when every snapshot in the bucket had NULL coverage. + AvgCoverage *float64 `json:"avg_coverage,omitempty"` + SnapshotCount int `json:"snapshot_count"` } -// ProviderBreakdown represents savings breakdown by provider +// ProviderBreakdown represents savings breakdown by provider. type ProviderBreakdown struct { - Provider string `json:"provider"` - Service string `json:"service"` - TotalSavings float64 `json:"total_savings"` - AvgCoverage float64 `json:"avg_coverage"` + Provider string `json:"provider"` + Service string `json:"service"` + TotalSavings float64 `json:"total_savings"` + AvgCoverage *float64 `json:"avg_coverage,omitempty"` } -// ServiceBreakdown represents savings breakdown by service +// ServiceBreakdown represents savings breakdown by service. type ServiceBreakdown struct { - Service string `json:"service"` - Region string `json:"region"` - TotalSavings float64 `json:"total_savings"` - AvgCoverage float64 `json:"avg_coverage"` + Service string `json:"service"` + Region string `json:"region"` + TotalSavings float64 `json:"total_savings"` + AvgCoverage *float64 `json:"avg_coverage,omitempty"` } -// AnalyticsStore defines the interface for analytics storage +// AnalyticsStore defines the interface for analytics storage. type AnalyticsStore interface { - // SaveSnapshot stores a single savings snapshot + // SaveSnapshot stores a single savings snapshot. SaveSnapshot(ctx context.Context, snapshot *SavingsSnapshot) error - // BulkInsertSnapshots inserts multiple snapshots efficiently (for migrations) + // BulkInsertSnapshots inserts multiple snapshots efficiently. BulkInsertSnapshots(ctx context.Context, snapshots []SavingsSnapshot) error - // QuerySavings retrieves savings snapshots based on query parameters + // QuerySavings retrieves savings snapshots based on query parameters. QuerySavings(ctx context.Context, req QueryRequest) ([]SavingsSnapshot, error) - // Aggregated queries (using materialized views for performance) - QueryMonthlyTotals(ctx context.Context, accountID string, months int) ([]MonthlySummary, error) - QueryByProvider(ctx context.Context, accountID string, startDate, endDate time.Time) ([]ProviderBreakdown, error) - QueryByService(ctx context.Context, accountID string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) + // Aggregated queries (using materialized views for performance). Scoping is + // the dual-column model; pass empty filters only after enforcing scope. + QueryMonthlyTotals(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, months int) ([]MonthlySummary, error) + QueryByProvider(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, startDate, endDate time.Time) ([]ProviderBreakdown, error) + QueryByService(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) - // Partition management + // Partition management. CreatePartition(ctx context.Context, forMonth time.Time) error + CreateFuturePartitions(ctx context.Context, monthsAhead int) error DropOldPartitions(ctx context.Context, retentionMonths int) error CreatePartitionsForRange(ctx context.Context, startDate, endDate time.Time) error - // Materialized view management + // Materialized view management. RefreshMaterializedViews(ctx context.Context) error - // Close cleans up resources + // Close cleans up resources. Close() error } diff --git a/internal/analytics/postgres_analytics.go b/internal/analytics/postgres_analytics.go index b753ef2ce..4b9968c6a 100644 --- a/internal/analytics/postgres_analytics.go +++ b/internal/analytics/postgres_analytics.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "fmt" + "sort" + "strings" "time" "github.com/LeanerCloud/CUDly/internal/database" @@ -21,31 +23,84 @@ type dbConn interface { Acquire(ctx context.Context) (*pgxpool.Conn, error) } -// PostgresAnalyticsStore implements AnalyticsStore using PostgreSQL +// PostgresAnalyticsStore implements AnalyticsStore using PostgreSQL. type PostgresAnalyticsStore struct { db dbConn } -// NewPostgresAnalyticsStore creates a new PostgreSQL analytics store +// NewPostgresAnalyticsStore creates a new PostgreSQL analytics store. func NewPostgresAnalyticsStore(db *database.Connection) *PostgresAnalyticsStore { return &PostgresAnalyticsStore{db: db} } -// Verify PostgresAnalyticsStore implements AnalyticsStore +// Verify PostgresAnalyticsStore implements AnalyticsStore. var _ AnalyticsStore = (*PostgresAnalyticsStore)(nil) +// accountFilterClause builds the dual-column account WHERE fragment plus the +// full positional arg list for the analytics queries. baseArgs holds the fixed +// leading binds; the account array binds (if any) are appended after them and +// the returned clause references them by the right positions. +// +// savings_snapshots carries two account identifiers, either of which may be the +// only one populated on a row: account_id (the cloud-provider external number) +// and cloud_account_id (the cloud_accounts UUID FK, NULL on the AWS ambient and +// legacy rows). Matching only one column silently drops rows that carry only the +// other, so we OR both, with the external-id half grouped per provider so a +// reused external number across providers cannot leak the wrong rows. This +// mirrors api.accountFilterClause on the live purchase_history path +// (issue #701/#498/#866). Both empty -> "TRUE" (caller must enforce scope). +func accountFilterClause(accountUUIDs []string, accountExternalIDsByProvider map[string][]string, baseArgs []any) (clause string, args []any) { + args = baseArgs + var ors []string + if len(accountUUIDs) > 0 { + args = append(args, accountUUIDs) + ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args))) + } + for _, provider := range sortedProviderKeys(accountExternalIDsByProvider) { + exts := accountExternalIDsByProvider[provider] + if len(exts) == 0 { + continue + } + if provider == "" { + args = append(args, exts) + ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args))) + continue + } + args = append(args, provider) + providerArg := len(args) + args = append(args, exts) + ors = append(ors, fmt.Sprintf("(provider = $%d AND account_id = ANY($%d))", providerArg, len(args))) + } + if len(ors) == 0 { + return "TRUE", args + } + return "(" + strings.Join(ors, " OR ") + ")", args +} + +// sortedProviderKeys returns the map keys in ascending order so generated SQL +// (and its bind-arg ordering) is deterministic and testable. +func sortedProviderKeys(m map[string][]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // ========================================== // SNAPSHOT OPERATIONS // ========================================== -// SaveSnapshot stores a single savings snapshot +// SaveSnapshot stores a single savings snapshot. func (s *PostgresAnalyticsStore) SaveSnapshot(ctx context.Context, snapshot *SavingsSnapshot) error { - // Generate UUID if not provided + if err := validateCommitmentType(snapshot.CommitmentType); err != nil { + return fmt.Errorf("invalid savings snapshot: %w", err) + } if snapshot.ID == "" { snapshot.ID = uuid.New().String() } - // Marshal metadata to JSONB var metadataJSON []byte var err error if snapshot.Metadata != nil { @@ -57,15 +112,16 @@ func (s *PostgresAnalyticsStore) SaveSnapshot(ctx context.Context, snapshot *Sav query := ` INSERT INTO savings_snapshots ( - id, account_id, timestamp, provider, service, region, + id, account_id, cloud_account_id, timestamp, provider, service, region, commitment_type, total_commitment, total_usage, total_savings, coverage_percentage, metadata - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) ` _, err = s.db.Exec(ctx, query, snapshot.ID, snapshot.AccountID, + snapshot.CloudAccountID, snapshot.Timestamp, snapshot.Provider, snapshot.Service, @@ -77,46 +133,58 @@ func (s *PostgresAnalyticsStore) SaveSnapshot(ctx context.Context, snapshot *Sav snapshot.CoveragePercentage, metadataJSON, ) - if err != nil { return fmt.Errorf("failed to save savings snapshot: %w", err) } + return nil +} +// validateCommitmentType rejects a commitment_type that the savings_snapshots +// table CHECK constraint would reject. Extracted so the guard is unit-testable +// directly (the COPY path acquires a real pooled connection that pgxmock can't +// stand in for, so an end-to-end test can only prove the acquire-failure path). +func validateCommitmentType(commitmentType string) error { + if commitmentType != "RI" && commitmentType != "SavingsPlan" { + return fmt.Errorf("invalid commitment_type %q (want RI or SavingsPlan)", commitmentType) + } return nil } -// BulkInsertSnapshots inserts multiple snapshots efficiently +// BulkInsertSnapshots inserts multiple snapshots efficiently via COPY. func (s *PostgresAnalyticsStore) BulkInsertSnapshots(ctx context.Context, snapshots []SavingsSnapshot) error { if len(snapshots) == 0 { return nil } - // Use COPY for efficient bulk insert conn, err := s.db.Acquire(ctx) if err != nil { return fmt.Errorf("failed to acquire connection: %w", err) } defer conn.Release() - // Prepare COPY statement _, err = conn.Conn().CopyFrom( ctx, pgx.Identifier{"savings_snapshots"}, []string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }, pgx.CopyFromSlice(len(snapshots), func(i int) ([]any, error) { snapshot := snapshots[i] - // Generate UUID if not provided if snapshot.ID == "" { snapshot.ID = uuid.New().String() } - // Marshal metadata. Use []byte so pgx transmits it as a JSON value - // for the jsonb column rather than as a bytea literal. + // Validate commitment_type against the table CHECK before COPY so a + // single bad value doesn't abort the entire batch server-side (L4). + if err := validateCommitmentType(snapshot.CommitmentType); err != nil { + return nil, fmt.Errorf("snapshot %d: %w", i, err) + } + + // Marshal metadata as []byte so pgx transmits it as a JSON value for + // the jsonb column rather than as a bytea literal. var metadataJSON []byte if snapshot.Metadata != nil { data, err := json.Marshal(snapshot.Metadata) @@ -129,6 +197,7 @@ func (s *PostgresAnalyticsStore) BulkInsertSnapshots(ctx context.Context, snapsh return []any{ snapshot.ID, snapshot.AccountID, + snapshot.CloudAccountID, snapshot.Timestamp, snapshot.Provider, snapshot.Service, @@ -142,49 +211,41 @@ func (s *PostgresAnalyticsStore) BulkInsertSnapshots(ctx context.Context, snapsh }, nil }), ) - if err != nil { return fmt.Errorf("failed to bulk insert snapshots: %w", err) } - return nil } -// QuerySavings retrieves savings snapshots based on query parameters +// QuerySavings retrieves savings snapshots based on query parameters. func (s *PostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequest) ([]SavingsSnapshot, error) { - // Build query with optional filters + accountClause, args := accountFilterClause(req.AccountUUIDs, req.AccountExternalIDsByProvider, []any{req.StartDate, req.EndDate}) + + // #nosec G201 — accountClause references only parameter placeholders built + // internally; the optional provider/service filters below are also bound. query := ` - SELECT id, account_id, timestamp, provider, service, region, + SELECT id, account_id, cloud_account_id, timestamp, provider, service, region, commitment_type, total_commitment, total_usage, total_savings, coverage_percentage, metadata FROM savings_snapshots - WHERE account_id = $1 - AND timestamp >= $2 - AND timestamp <= $3 - ` - - args := []any{req.AccountID, req.StartDate, req.EndDate} - argIndex := 4 + WHERE timestamp >= $1 + AND timestamp <= $2 + AND ` + accountClause - // Add optional filters if req.Provider != "" { - query += fmt.Sprintf(" AND provider = $%d", argIndex) args = append(args, req.Provider) - argIndex++ + query += fmt.Sprintf(" AND provider = $%d", len(args)) } - if req.Service != "" { - query += fmt.Sprintf(" AND service = $%d", argIndex) args = append(args, req.Service) - argIndex++ + query += fmt.Sprintf(" AND service = $%d", len(args)) } query += " ORDER BY timestamp DESC" - // Add limit if req.Limit > 0 { - query += fmt.Sprintf(" LIMIT $%d", argIndex) args = append(args, req.Limit) + query += fmt.Sprintf(" LIMIT $%d", len(args)) } rows, err := s.db.Query(ctx, query, args...) @@ -201,6 +262,7 @@ func (s *PostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequ err := rows.Scan( &snapshot.ID, &snapshot.AccountID, + &snapshot.CloudAccountID, &snapshot.Timestamp, &snapshot.Provider, &snapshot.Service, @@ -216,7 +278,6 @@ func (s *PostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequ return nil, fmt.Errorf("failed to scan snapshot: %w", err) } - // Unmarshal metadata if len(metadataJSON) > 0 { if err := json.Unmarshal(metadataJSON, &snapshot.Metadata); err != nil { return nil, fmt.Errorf("failed to unmarshal metadata: %w", err) @@ -233,17 +294,26 @@ func (s *PostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequ // AGGREGATED QUERIES // ========================================== -// QueryMonthlyTotals retrieves monthly aggregated totals -func (s *PostgresAnalyticsStore) QueryMonthlyTotals(ctx context.Context, accountID string, months int) ([]MonthlySummary, error) { +// QueryMonthlyTotals retrieves monthly aggregated totals for the last N months +// (inclusive of the current month). months <= 0 returns no rows. +func (s *PostgresAnalyticsStore) QueryMonthlyTotals(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, months int) ([]MonthlySummary, error) { + if months <= 0 { + return []MonthlySummary{}, nil + } + // Last N inclusive months: floor(now) back N-1 months. make_interval avoids + // the INTERVAL '1 month' * N off-by-one (M1). + accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{months}) + + // #nosec G201 — accountClause uses only internally-built placeholders. query := ` - SELECT month, account_id, provider, service, total_savings, avg_coverage, snapshot_count + SELECT month, account_id, cloud_account_id, provider, service, total_savings, avg_coverage, snapshot_count FROM monthly_savings_summary - WHERE account_id = $1 - AND month >= DATE_TRUNC('month', NOW() - INTERVAL '1 month' * $2) + WHERE month >= DATE_TRUNC('month', NOW()) - make_interval(months => $1 - 1) + AND ` + accountClause + ` ORDER BY month DESC, provider, service ` - rows, err := s.db.Query(ctx, query, accountID, months) + rows, err := s.db.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("failed to query monthly totals: %w", err) } @@ -255,6 +325,7 @@ func (s *PostgresAnalyticsStore) QueryMonthlyTotals(ctx context.Context, account err := rows.Scan( &summary.Month, &summary.AccountID, + &summary.CloudAccountID, &summary.Provider, &summary.Service, &summary.TotalSavings, @@ -270,19 +341,22 @@ func (s *PostgresAnalyticsStore) QueryMonthlyTotals(ctx context.Context, account return summaries, rows.Err() } -// QueryByProvider retrieves savings breakdown by provider -func (s *PostgresAnalyticsStore) QueryByProvider(ctx context.Context, accountID string, startDate, endDate time.Time) ([]ProviderBreakdown, error) { +// QueryByProvider retrieves savings breakdown by provider/service. +func (s *PostgresAnalyticsStore) QueryByProvider(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, startDate, endDate time.Time) ([]ProviderBreakdown, error) { + accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{startDate, endDate}) + + // #nosec G201 — accountClause uses only internally-built placeholders. query := ` - SELECT provider, service, SUM(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage + SELECT provider, service, AVG(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage FROM savings_snapshots - WHERE account_id = $1 - AND timestamp >= $2 - AND timestamp <= $3 + WHERE timestamp >= $1 + AND timestamp <= $2 + AND ` + accountClause + ` GROUP BY provider, service ORDER BY total_savings DESC ` - rows, err := s.db.Query(ctx, query, accountID, startDate, endDate) + rows, err := s.db.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("failed to query by provider: %w", err) } @@ -306,20 +380,30 @@ func (s *PostgresAnalyticsStore) QueryByProvider(ctx context.Context, accountID return breakdowns, rows.Err() } -// QueryByService retrieves savings breakdown by service -func (s *PostgresAnalyticsStore) QueryByService(ctx context.Context, accountID string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) { - query := ` - SELECT service, region, SUM(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage +// QueryByService retrieves savings breakdown by service/region, optionally +// filtered to a single provider. An empty provider returns all providers' +// services. +func (s *PostgresAnalyticsStore) QueryByService(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) { + accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{startDate, endDate}) + + providerClause := "" + if provider != "" { + args = append(args, provider) + providerClause = fmt.Sprintf(" AND provider = $%d", len(args)) + } + + // #nosec G201 — accountClause / providerClause use only internally-built placeholders. + query := fmt.Sprintf(` + SELECT service, region, AVG(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage FROM savings_snapshots - WHERE account_id = $1 - AND provider = $2 - AND timestamp >= $3 - AND timestamp <= $4 + WHERE timestamp >= $1 + AND timestamp <= $2 + AND %s%s GROUP BY service, region ORDER BY total_savings DESC - ` + `, accountClause, providerClause) - rows, err := s.db.Query(ctx, query, accountID, provider, startDate, endDate) + rows, err := s.db.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("failed to query by service: %w", err) } @@ -347,33 +431,43 @@ func (s *PostgresAnalyticsStore) QueryByService(ctx context.Context, accountID s // PARTITION MANAGEMENT // ========================================== -// CreatePartition creates a partition for a specific month +// CreatePartition creates a partition for a specific month. func (s *PostgresAnalyticsStore) CreatePartition(ctx context.Context, forMonth time.Time) error { - query := `SELECT create_savings_snapshot_partition($1)` - - _, err := s.db.Exec(ctx, query, forMonth) - if err != nil { + if _, err := s.db.Exec(ctx, `SELECT create_savings_snapshot_partition($1)`, forMonth); err != nil { return fmt.Errorf("failed to create partition: %w", err) } + return nil +} +// CreateFuturePartitions ensures partitions exist for the current month plus +// monthsAhead months ahead via the create_future_savings_partitions SQL helper. +func (s *PostgresAnalyticsStore) CreateFuturePartitions(ctx context.Context, monthsAhead int) error { + if monthsAhead < 0 { + return fmt.Errorf("monthsAhead must be >= 0, got %d", monthsAhead) + } + if _, err := s.db.Exec(ctx, `SELECT create_future_savings_partitions($1)`, monthsAhead); err != nil { + return fmt.Errorf("failed to create future partitions: %w", err) + } return nil } -// DropOldPartitions removes partitions older than retention period +// DropOldPartitions removes partitions older than the retention period. func (s *PostgresAnalyticsStore) DropOldPartitions(ctx context.Context, retentionMonths int) error { - query := `SELECT drop_old_savings_partitions($1)` - - _, err := s.db.Exec(ctx, query, retentionMonths) - if err != nil { + if retentionMonths <= 0 { + return fmt.Errorf("retentionMonths must be > 0, got %d", retentionMonths) + } + if _, err := s.db.Exec(ctx, `SELECT drop_old_savings_partitions($1)`, retentionMonths); err != nil { return fmt.Errorf("failed to drop old partitions: %w", err) } - return nil } -// CreatePartitionsForRange creates partitions for a date range (used during migration) +// CreatePartitionsForRange creates partitions for each month in a date range +// (used during backfill / migration). func (s *PostgresAnalyticsStore) CreatePartitionsForRange(ctx context.Context, startDate, endDate time.Time) error { - // Create partition for each month in the range + if startDate.After(endDate) { + return fmt.Errorf("startDate %v must not be after endDate %v", startDate, endDate) + } current := time.Date(startDate.Year(), startDate.Month(), 1, 0, 0, 0, 0, time.UTC) end := time.Date(endDate.Year(), endDate.Month(), 1, 0, 0, 0, 0, time.UTC) @@ -383,7 +477,6 @@ func (s *PostgresAnalyticsStore) CreatePartitionsForRange(ctx context.Context, s } current = current.AddDate(0, 1, 0) } - return nil } @@ -391,15 +484,11 @@ func (s *PostgresAnalyticsStore) CreatePartitionsForRange(ctx context.Context, s // MATERIALIZED VIEW MANAGEMENT // ========================================== -// RefreshMaterializedViews refreshes all analytics materialized views +// RefreshMaterializedViews refreshes all analytics materialized views. func (s *PostgresAnalyticsStore) RefreshMaterializedViews(ctx context.Context) error { - query := `SELECT refresh_savings_materialized_views()` - - _, err := s.db.Exec(ctx, query) - if err != nil { + if _, err := s.db.Exec(ctx, `SELECT refresh_savings_materialized_views()`); err != nil { return fmt.Errorf("failed to refresh materialized views: %w", err) } - return nil } @@ -407,7 +496,7 @@ func (s *PostgresAnalyticsStore) RefreshMaterializedViews(ctx context.Context) e // CLEANUP // ========================================== -// Close cleans up resources (no-op for PostgreSQL store) +// Close cleans up resources (no-op for PostgreSQL store). func (s *PostgresAnalyticsStore) Close() error { return nil } diff --git a/internal/analytics/postgres_analytics_mock_test.go b/internal/analytics/postgres_analytics_mock_test.go index 1c81aaa6c..97e41ffcb 100644 --- a/internal/analytics/postgres_analytics_mock_test.go +++ b/internal/analytics/postgres_analytics_mock_test.go @@ -30,139 +30,41 @@ func TestSaveSnapshotMarshalError(t *testing.T) { }) } -// TestQueryFiltersBuilding verifies the query filter building logic -func TestQueryFiltersBuilding(t *testing.T) { - t.Run("basic query without filters", func(t *testing.T) { - req := QueryRequest{ - AccountID: "account-123", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), - } - - // Simulate the args building logic from QuerySavings - args := []interface{}{req.AccountID, req.StartDate, req.EndDate} - argIndex := 4 - - if req.Provider != "" { - args = append(args, req.Provider) - argIndex++ - } - - if req.Service != "" { - args = append(args, req.Service) - argIndex++ - } - - if req.Limit > 0 { - args = append(args, req.Limit) - } - - assert.Len(t, args, 3) - assert.Equal(t, 4, argIndex) // No filters added - }) - - t.Run("query with provider filter adds arg", func(t *testing.T) { - req := QueryRequest{ - AccountID: "account-123", - Provider: "aws", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), - } - - args := []interface{}{req.AccountID, req.StartDate, req.EndDate} - argIndex := 4 - - if req.Provider != "" { - args = append(args, req.Provider) - argIndex++ - } - - if req.Service != "" { - args = append(args, req.Service) - argIndex++ - } - - assert.Len(t, args, 4) - assert.Equal(t, 5, argIndex) +// TestAccountFilterClause exercises the real dual-column scoping clause builder +// used by every Query* method (replaces the old test that re-implemented the +// arg-building logic without calling production code). +func TestAccountFilterClause(t *testing.T) { + base := func() []any { return []any{"start", "end"} } + + t.Run("both empty yields TRUE and unchanged args", func(t *testing.T) { + clause, args := accountFilterClause(nil, nil, base()) + assert.Equal(t, "TRUE", clause) + assert.Len(t, args, 2) }) - t.Run("query with service filter adds arg", func(t *testing.T) { - req := QueryRequest{ - AccountID: "account-123", - Service: "rds", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), - } - - args := []interface{}{req.AccountID, req.StartDate, req.EndDate} - argIndex := 4 - - if req.Provider != "" { - args = append(args, req.Provider) - argIndex++ - } - - if req.Service != "" { - args = append(args, req.Service) - argIndex++ - } - - assert.Len(t, args, 4) - assert.Equal(t, 5, argIndex) + t.Run("UUIDs only match cloud_account_id", func(t *testing.T) { + clause, args := accountFilterClause([]string{"u1", "u2"}, nil, base()) + assert.Equal(t, "(cloud_account_id = ANY($3))", clause) + assert.Len(t, args, 3) }) - t.Run("query with both filters adds two args", func(t *testing.T) { - req := QueryRequest{ - AccountID: "account-123", - Provider: "aws", - Service: "rds", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), - } - - args := []interface{}{req.AccountID, req.StartDate, req.EndDate} - argIndex := 4 - - if req.Provider != "" { - args = append(args, req.Provider) - argIndex++ - } - - if req.Service != "" { - args = append(args, req.Service) - argIndex++ - } - - assert.Len(t, args, 5) - assert.Equal(t, 6, argIndex) + t.Run("external ids are grouped and provider-scoped", func(t *testing.T) { + clause, args := accountFilterClause( + []string{"u1"}, + map[string][]string{"aws": {"123"}, "azure": {"sub-1"}}, + base(), + ) + // Deterministic, sorted provider order: aws then azure. + assert.Equal(t, + "(cloud_account_id = ANY($3) OR (provider = $4 AND account_id = ANY($5)) OR (provider = $6 AND account_id = ANY($7)))", + clause) + assert.Len(t, args, 7) }) - t.Run("query with limit adds arg", func(t *testing.T) { - req := QueryRequest{ - AccountID: "account-123", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), - Limit: 10, - } - - args := []interface{}{req.AccountID, req.StartDate, req.EndDate} - argIndex := 4 - - if req.Provider != "" { - args = append(args, req.Provider) - argIndex++ - } - - if req.Service != "" { - args = append(args, req.Service) - argIndex++ - } - - if req.Limit > 0 { - args = append(args, req.Limit) - } - - assert.Len(t, args, 4) + t.Run("empty provider key matches account_id with no provider gate", func(t *testing.T) { + clause, args := accountFilterClause(nil, map[string][]string{"": {"123"}}, base()) + assert.Equal(t, "(account_id = ANY($3))", clause) + assert.Len(t, args, 3) }) } diff --git a/internal/analytics/postgres_analytics_nilsafe_test.go b/internal/analytics/postgres_analytics_nilsafe_test.go index 0e1b5a27e..3789dfa26 100644 --- a/internal/analytics/postgres_analytics_nilsafe_test.go +++ b/internal/analytics/postgres_analytics_nilsafe_test.go @@ -36,12 +36,13 @@ func TestPostgresAnalyticsStore_SaveSnapshot_MetadataMarshalError(t *testing.T) t.Run("returns error when metadata contains un-marshallable value", func(t *testing.T) { snapshot := &SavingsSnapshot{ - ID: "existing-id", - AccountID: "account-123", - Timestamp: time.Now().UTC(), - Provider: "aws", - Service: "rds", - Region: "us-east-1", + ID: "existing-id", + AccountID: "account-123", + Timestamp: time.Now().UTC(), + Provider: "aws", + Service: "rds", + Region: "us-east-1", + CommitmentType: "RI", // A channel is not JSON-serialisable, so json.Marshal will fail. Metadata: map[string]any{ "bad_field": make(chan int), @@ -57,9 +58,10 @@ func TestPostgresAnalyticsStore_SaveSnapshot_MetadataMarshalError(t *testing.T) // When ID is empty, a UUID is generated. The un-marshallable metadata // error is still returned, but the ID field on the snapshot is set first. snapshot := &SavingsSnapshot{ - ID: "", // will be populated - AccountID: "account-123", - Timestamp: time.Now().UTC(), + ID: "", // will be populated + AccountID: "account-123", + Timestamp: time.Now().UTC(), + CommitmentType: "SavingsPlan", Metadata: map[string]any{ "bad_field": make(chan int), }, @@ -84,17 +86,18 @@ func TestPostgresAnalyticsStore_SaveSnapshot_MetadataMarshalError(t *testing.T) // real (or mock-backed) *database.Connection. // TestPostgresAnalyticsStore_CreatePartitionsForRange_ReversedDates verifies -// that when start > end the loop body is never entered and the function returns -// nil without touching s.db. +// that when start > end the function rejects the reversed range with an error +// (L3 fix) rather than silently returning nil success without touching s.db. func TestPostgresAnalyticsStore_CreatePartitionsForRange_ReversedDates(t *testing.T) { store := NewPostgresAnalyticsStore(nil) - t.Run("start after end returns nil without DB call", func(t *testing.T) { + t.Run("start after end returns error without DB call", func(t *testing.T) { future := time.Date(2030, 6, 1, 0, 0, 0, 0, time.UTC) past := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC) err := store.CreatePartitionsForRange(context.Background(), future, past) - assert.NoError(t, err) + require.Error(t, err) + assert.Contains(t, err.Error(), "must not be after") }) t.Run("start equal to end with no DB panics – test only the date logic", func(t *testing.T) { diff --git a/internal/analytics/postgres_analytics_pgxmock_test.go b/internal/analytics/postgres_analytics_pgxmock_test.go index d60979a48..5babf69c2 100644 --- a/internal/analytics/postgres_analytics_pgxmock_test.go +++ b/internal/analytics/postgres_analytics_pgxmock_test.go @@ -27,6 +27,9 @@ func anyArgs(n int) []interface{} { return args } +// f64ptr returns a pointer to f for the nullable *float64 snapshot fields. +func f64ptr(f float64) *float64 { return &f } + // ─── SaveSnapshot ────────────────────────────────────────────────────────────── func TestPostgresAnalyticsStore_SaveSnapshot_Success(t *testing.T) { @@ -34,7 +37,7 @@ func TestPostgresAnalyticsStore_SaveSnapshot_Success(t *testing.T) { ctx := context.Background() mock.ExpectExec(`INSERT INTO savings_snapshots`). - WithArgs(anyArgs(12)...). + WithArgs(anyArgs(13)...). WillReturnResult(pgxmock.NewResult("INSERT", 1)) snap := &SavingsSnapshot{ @@ -45,9 +48,9 @@ func TestPostgresAnalyticsStore_SaveSnapshot_Success(t *testing.T) { Region: "us-east-1", CommitmentType: "RI", TotalCommitment: 1000.0, - TotalUsage: 900.0, + TotalUsage: f64ptr(900.0), TotalSavings: 100.0, - CoveragePercentage: 90.0, + CoveragePercentage: f64ptr(90.0), } err := store.SaveSnapshot(ctx, snap) require.NoError(t, err) @@ -60,10 +63,10 @@ func TestPostgresAnalyticsStore_SaveSnapshot_ExecError(t *testing.T) { ctx := context.Background() mock.ExpectExec(`INSERT INTO savings_snapshots`). - WithArgs(anyArgs(12)...). + WithArgs(anyArgs(13)...). WillReturnError(errors.New("db error")) - snap := &SavingsSnapshot{AccountID: "acct1", Timestamp: time.Now()} + snap := &SavingsSnapshot{AccountID: "acct1", Timestamp: time.Now(), CommitmentType: "RI"} err := store.SaveSnapshot(ctx, snap) assert.ErrorContains(t, err, "failed to save savings snapshot") assert.NoError(t, mock.ExpectationsWereMet()) @@ -74,14 +77,15 @@ func TestPostgresAnalyticsStore_SaveSnapshot_WithMetadata(t *testing.T) { ctx := context.Background() mock.ExpectExec(`INSERT INTO savings_snapshots`). - WithArgs(anyArgs(12)...). + WithArgs(anyArgs(13)...). WillReturnResult(pgxmock.NewResult("INSERT", 1)) snap := &SavingsSnapshot{ - ID: "preset-id", - AccountID: "acct1", - Timestamp: time.Now(), - Metadata: map[string]any{"env": "prod"}, + ID: "preset-id", + AccountID: "acct1", + Timestamp: time.Now(), + CommitmentType: "SavingsPlan", + Metadata: map[string]any{"env": "prod"}, } err := store.SaveSnapshot(ctx, snap) require.NoError(t, err) @@ -95,7 +99,7 @@ func TestPostgresAnalyticsStore_QuerySavings_Empty(t *testing.T) { ctx := context.Background() rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }) @@ -104,9 +108,9 @@ func TestPostgresAnalyticsStore_QuerySavings_Empty(t *testing.T) { WillReturnRows(rows) result, err := store.QuerySavings(ctx, QueryRequest{ - AccountID: "acct1", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), + AccountUUIDs: []string{"acct1"}, + StartDate: time.Now().Add(-24 * time.Hour), + EndDate: time.Now(), }) require.NoError(t, err) assert.Empty(t, result) @@ -117,11 +121,11 @@ func TestPostgresAnalyticsStore_QuerySavings_WithFilters(t *testing.T) { ctx := context.Background() rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", - }).AddRow("id1", "acct1", time.Now(), "aws", "ec2", "us-east-1", "RI", - 1000.0, 900.0, 100.0, 90.0, []byte(nil)) + }).AddRow("id1", "acct1", strPtr("cloud-1"), time.Now(), "aws", "ec2", "us-east-1", "RI", + 1000.0, f64ptr(900.0), 100.0, f64ptr(90.0), []byte(nil)) // With provider + service + limit: 3 base + 2 filters + 1 limit = 6 args mock.ExpectQuery(`SELECT id, account_id`). @@ -129,12 +133,12 @@ func TestPostgresAnalyticsStore_QuerySavings_WithFilters(t *testing.T) { WillReturnRows(rows) result, err := store.QuerySavings(ctx, QueryRequest{ - AccountID: "acct1", - Provider: "aws", - Service: "ec2", - Limit: 10, - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), + AccountUUIDs: []string{"acct1"}, + Provider: "aws", + Service: "ec2", + Limit: 10, + StartDate: time.Now().Add(-24 * time.Hour), + EndDate: time.Now(), }) require.NoError(t, err) assert.Len(t, result, 1) @@ -146,20 +150,20 @@ func TestPostgresAnalyticsStore_QuerySavings_WithMetadata(t *testing.T) { ctx := context.Background() rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", - }).AddRow("id1", "acct1", time.Now(), "aws", "ec2", "us-east-1", "RI", - 1000.0, 900.0, 100.0, 90.0, []byte(`{"key":"val"}`)) + }).AddRow("id1", "acct1", strPtr("cloud-1"), time.Now(), "aws", "ec2", "us-east-1", "RI", + 1000.0, f64ptr(900.0), 100.0, f64ptr(90.0), []byte(`{"key":"val"}`)) mock.ExpectQuery(`SELECT id, account_id`). WithArgs(anyArgs(3)...). WillReturnRows(rows) result, err := store.QuerySavings(ctx, QueryRequest{ - AccountID: "acct1", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), + AccountUUIDs: []string{"acct1"}, + StartDate: time.Now().Add(-24 * time.Hour), + EndDate: time.Now(), }) require.NoError(t, err) require.Len(t, result, 1) @@ -175,9 +179,9 @@ func TestPostgresAnalyticsStore_QuerySavings_QueryError(t *testing.T) { WillReturnError(errors.New("db error")) _, err := store.QuerySavings(ctx, QueryRequest{ - AccountID: "acct1", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), + AccountUUIDs: []string{"acct1"}, + StartDate: time.Now().Add(-24 * time.Hour), + EndDate: time.Now(), }) assert.ErrorContains(t, err, "failed to query savings") } @@ -187,20 +191,20 @@ func TestPostgresAnalyticsStore_QuerySavings_MetadataUnmarshalError(t *testing.T ctx := context.Background() rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", - }).AddRow("id1", "acct1", time.Now(), "aws", "ec2", "us-east-1", "RI", - 1000.0, 900.0, 100.0, 90.0, []byte(`{invalid json`)) + }).AddRow("id1", "acct1", strPtr("cloud-1"), time.Now(), "aws", "ec2", "us-east-1", "RI", + 1000.0, f64ptr(900.0), 100.0, f64ptr(90.0), []byte(`{invalid json`)) mock.ExpectQuery(`SELECT id, account_id`). WithArgs(anyArgs(3)...). WillReturnRows(rows) _, err := store.QuerySavings(ctx, QueryRequest{ - AccountID: "acct1", - StartDate: time.Now().Add(-time.Hour), - EndDate: time.Now(), + AccountUUIDs: []string{"acct1"}, + StartDate: time.Now().Add(-time.Hour), + EndDate: time.Now(), }) assert.ErrorContains(t, err, "failed to unmarshal metadata") } @@ -215,9 +219,9 @@ func TestPostgresAnalyticsStore_QuerySavings_ScanError(t *testing.T) { WillReturnRows(rows) _, err := store.QuerySavings(ctx, QueryRequest{ - AccountID: "acct1", - StartDate: time.Now().Add(-time.Hour), - EndDate: time.Now(), + AccountUUIDs: []string{"acct1"}, + StartDate: time.Now().Add(-time.Hour), + EndDate: time.Now(), }) assert.Error(t, err) } @@ -229,14 +233,14 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_Empty(t *testing.T) { ctx := context.Background() rows := pgxmock.NewRows([]string{ - "month", "account_id", "provider", "service", + "month", "account_id", "cloud_account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", }) mock.ExpectQuery(`SELECT month`). WithArgs(anyArgs(2)...). WillReturnRows(rows) - result, err := store.QueryMonthlyTotals(ctx, "acct1", 6) + result, err := store.QueryMonthlyTotals(ctx, []string{"acct1"}, nil, 6) require.NoError(t, err) assert.Empty(t, result) } @@ -249,7 +253,7 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_Error(t *testing.T) { WithArgs(anyArgs(2)...). WillReturnError(errors.New("db down")) - _, err := store.QueryMonthlyTotals(ctx, "acct1", 6) + _, err := store.QueryMonthlyTotals(ctx, []string{"acct1"}, nil, 6) assert.ErrorContains(t, err, "failed to query monthly totals") } @@ -258,15 +262,15 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_WithRows(t *testing.T) { ctx := context.Background() rows := pgxmock.NewRows([]string{ - "month", "account_id", "provider", "service", + "month", "account_id", "cloud_account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", - }).AddRow(time.Now(), "acct1", "aws", "ec2", 500.0, 85.0, 10) + }).AddRow(time.Now(), "acct1", strPtr("cloud-1"), "aws", "ec2", 500.0, f64ptr(85.0), 10) mock.ExpectQuery(`SELECT month`). WithArgs(anyArgs(2)...). WillReturnRows(rows) - result, err := store.QueryMonthlyTotals(ctx, "acct1", 6) + result, err := store.QueryMonthlyTotals(ctx, []string{"acct1"}, nil, 6) require.NoError(t, err) require.Len(t, result, 1) assert.Equal(t, "aws", result[0].Provider) @@ -281,7 +285,7 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_ScanError(t *testing.T) { WithArgs(anyArgs(2)...). WillReturnRows(rows) - _, err := store.QueryMonthlyTotals(ctx, "acct1", 3) + _, err := store.QueryMonthlyTotals(ctx, []string{"acct1"}, nil, 3) assert.Error(t, err) } @@ -296,7 +300,7 @@ func TestPostgresAnalyticsStore_QueryByProvider_Empty(t *testing.T) { WithArgs(anyArgs(3)...). WillReturnRows(rows) - result, err := store.QueryByProvider(ctx, "acct1", time.Now().Add(-time.Hour), time.Now()) + result, err := store.QueryByProvider(ctx, []string{"acct1"}, nil, time.Now().Add(-time.Hour), time.Now()) require.NoError(t, err) assert.Empty(t, result) } @@ -309,7 +313,7 @@ func TestPostgresAnalyticsStore_QueryByProvider_Error(t *testing.T) { WithArgs(anyArgs(3)...). WillReturnError(errors.New("err")) - _, err := store.QueryByProvider(ctx, "acct1", time.Now().Add(-time.Hour), time.Now()) + _, err := store.QueryByProvider(ctx, []string{"acct1"}, nil, time.Now().Add(-time.Hour), time.Now()) assert.ErrorContains(t, err, "failed to query by provider") } @@ -318,12 +322,12 @@ func TestPostgresAnalyticsStore_QueryByProvider_WithRows(t *testing.T) { ctx := context.Background() rows := pgxmock.NewRows([]string{"provider", "service", "total_savings", "avg_coverage"}). - AddRow("aws", "ec2", 200.0, 80.0) + AddRow("aws", "ec2", 200.0, f64ptr(80.0)) mock.ExpectQuery(`SELECT provider`). WithArgs(anyArgs(3)...). WillReturnRows(rows) - result, err := store.QueryByProvider(ctx, "acct1", time.Now().Add(-time.Hour), time.Now()) + result, err := store.QueryByProvider(ctx, []string{"acct1"}, nil, time.Now().Add(-time.Hour), time.Now()) require.NoError(t, err) require.Len(t, result, 1) assert.Equal(t, "aws", result[0].Provider) @@ -338,7 +342,7 @@ func TestPostgresAnalyticsStore_QueryByProvider_ScanError(t *testing.T) { WithArgs(anyArgs(3)...). WillReturnRows(rows) - _, err := store.QueryByProvider(ctx, "acct1", time.Now().Add(-time.Hour), time.Now()) + _, err := store.QueryByProvider(ctx, []string{"acct1"}, nil, time.Now().Add(-time.Hour), time.Now()) assert.Error(t, err) } @@ -353,7 +357,7 @@ func TestPostgresAnalyticsStore_QueryByService_Empty(t *testing.T) { WithArgs(anyArgs(4)...). WillReturnRows(rows) - result, err := store.QueryByService(ctx, "acct1", "aws", time.Now().Add(-time.Hour), time.Now()) + result, err := store.QueryByService(ctx, []string{"acct1"}, nil, "aws", time.Now().Add(-time.Hour), time.Now()) require.NoError(t, err) assert.Empty(t, result) } @@ -366,7 +370,7 @@ func TestPostgresAnalyticsStore_QueryByService_Error(t *testing.T) { WithArgs(anyArgs(4)...). WillReturnError(errors.New("err")) - _, err := store.QueryByService(ctx, "acct1", "aws", time.Now().Add(-time.Hour), time.Now()) + _, err := store.QueryByService(ctx, []string{"acct1"}, nil, "aws", time.Now().Add(-time.Hour), time.Now()) assert.ErrorContains(t, err, "failed to query by service") } @@ -375,12 +379,12 @@ func TestPostgresAnalyticsStore_QueryByService_WithRows(t *testing.T) { ctx := context.Background() rows := pgxmock.NewRows([]string{"service", "region", "total_savings", "avg_coverage"}). - AddRow("ec2", "us-east-1", 300.0, 75.0) + AddRow("ec2", "us-east-1", 300.0, f64ptr(75.0)) mock.ExpectQuery(`SELECT service`). WithArgs(anyArgs(4)...). WillReturnRows(rows) - result, err := store.QueryByService(ctx, "acct1", "aws", time.Now().Add(-time.Hour), time.Now()) + result, err := store.QueryByService(ctx, []string{"acct1"}, nil, "aws", time.Now().Add(-time.Hour), time.Now()) require.NoError(t, err) require.Len(t, result, 1) assert.Equal(t, "ec2", result[0].Service) @@ -395,7 +399,7 @@ func TestPostgresAnalyticsStore_QueryByService_ScanError(t *testing.T) { WithArgs(anyArgs(4)...). WillReturnRows(rows) - _, err := store.QueryByService(ctx, "acct1", "aws", time.Now().Add(-time.Hour), time.Now()) + _, err := store.QueryByService(ctx, []string{"acct1"}, nil, "aws", time.Now().Add(-time.Hour), time.Now()) assert.Error(t, err) } diff --git a/internal/analytics/postgres_analytics_test.go b/internal/analytics/postgres_analytics_test.go index 39bbfb6ab..e2f651fdc 100644 --- a/internal/analytics/postgres_analytics_test.go +++ b/internal/analytics/postgres_analytics_test.go @@ -13,306 +13,35 @@ import ( "github.com/stretchr/testify/require" ) -// testablePostgresAnalyticsStore is a test-only wrapper that allows mocking +// testablePostgresAnalyticsStore is a thin test wrapper that delegates to the +// REAL PostgresAnalyticsStore (with a pgxmock pool as its dbConn) so these +// tests exercise the production SQL rather than a parallel reimplementation +// that could silently drift from it. The embedded *PostgresAnalyticsStore +// satisfies AnalyticsStore; only SaveSnapshot is overridden to give empty IDs +// a deterministic value the assertions can match. type testablePostgresAnalyticsStore struct { + *PostgresAnalyticsStore mock pgxmock.PgxPoolIface } -// Verify testablePostgresAnalyticsStore implements AnalyticsStore -var _ AnalyticsStore = (*testablePostgresAnalyticsStore)(nil) +func newTestableStore(mock pgxmock.PgxPoolIface) *testablePostgresAnalyticsStore { + return &testablePostgresAnalyticsStore{ + PostgresAnalyticsStore: &PostgresAnalyticsStore{db: mock}, + mock: mock, + } +} -// SaveSnapshot stores a single savings snapshot +// SaveSnapshot delegates to the real store after assigning a deterministic ID +// for the empty-ID case so the WithArgs expectations stay stable. func (s *testablePostgresAnalyticsStore) SaveSnapshot(ctx context.Context, snapshot *SavingsSnapshot) error { - // Generate UUID if not provided if snapshot.ID == "" { snapshot.ID = "generated-uuid" } - - // Marshal metadata to JSONB - var metadataJSON []byte - var err error - if snapshot.Metadata != nil { - metadataJSON, err = json.Marshal(snapshot.Metadata) - if err != nil { - return err - } - } - - query := ` - INSERT INTO savings_snapshots ( - id, account_id, timestamp, provider, service, region, - commitment_type, total_commitment, total_usage, total_savings, - coverage_percentage, metadata - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) - ` - - _, err = s.mock.Exec(ctx, query, - snapshot.ID, - snapshot.AccountID, - snapshot.Timestamp, - snapshot.Provider, - snapshot.Service, - snapshot.Region, - snapshot.CommitmentType, - snapshot.TotalCommitment, - snapshot.TotalUsage, - snapshot.TotalSavings, - snapshot.CoveragePercentage, - metadataJSON, - ) - - if err != nil { - return err - } - - return nil -} - -// BulkInsertSnapshots inserts multiple snapshots efficiently -func (s *testablePostgresAnalyticsStore) BulkInsertSnapshots(ctx context.Context, snapshots []SavingsSnapshot) error { - if len(snapshots) == 0 { - return nil - } - // For testing, we'll use batch insert instead of COPY - return errors.New("bulk insert requires real connection") + return s.PostgresAnalyticsStore.SaveSnapshot(ctx, snapshot) } -// QuerySavings retrieves savings snapshots based on query parameters -func (s *testablePostgresAnalyticsStore) QuerySavings(ctx context.Context, req QueryRequest) ([]SavingsSnapshot, error) { - query := ` - SELECT id, account_id, timestamp, provider, service, region, - commitment_type, total_commitment, total_usage, total_savings, - coverage_percentage, metadata - FROM savings_snapshots - WHERE account_id = $1 - AND timestamp >= $2 - AND timestamp <= $3 - ` - - args := []interface{}{req.AccountID, req.StartDate, req.EndDate} - argIndex := 4 - - if req.Provider != "" { - args = append(args, req.Provider) - argIndex++ - } - - if req.Service != "" { - args = append(args, req.Service) - argIndex++ - } - - if req.Limit > 0 { - args = append(args, req.Limit) - } - _ = argIndex // suppress unused variable warning - - rows, err := s.mock.Query(ctx, query, args...) - if err != nil { - return nil, err - } - defer rows.Close() - - snapshots := make([]SavingsSnapshot, 0) - for rows.Next() { - var snapshot SavingsSnapshot - var metadataJSON []byte - - err := rows.Scan( - &snapshot.ID, - &snapshot.AccountID, - &snapshot.Timestamp, - &snapshot.Provider, - &snapshot.Service, - &snapshot.Region, - &snapshot.CommitmentType, - &snapshot.TotalCommitment, - &snapshot.TotalUsage, - &snapshot.TotalSavings, - &snapshot.CoveragePercentage, - &metadataJSON, - ) - if err != nil { - return nil, err - } - - if len(metadataJSON) > 0 { - if err := json.Unmarshal(metadataJSON, &snapshot.Metadata); err != nil { - return nil, err - } - } - - snapshots = append(snapshots, snapshot) - } - - return snapshots, rows.Err() -} - -// QueryMonthlyTotals retrieves monthly aggregated totals -func (s *testablePostgresAnalyticsStore) QueryMonthlyTotals(ctx context.Context, accountID string, months int) ([]MonthlySummary, error) { - query := ` - SELECT month, account_id, provider, service, total_savings, avg_coverage, snapshot_count - FROM monthly_savings_summary - WHERE account_id = $1 - AND month >= DATE_TRUNC('month', NOW() - INTERVAL '1 month' * $2) - ORDER BY month DESC, provider, service - ` - - rows, err := s.mock.Query(ctx, query, accountID, months) - if err != nil { - return nil, err - } - defer rows.Close() - - summaries := make([]MonthlySummary, 0) - for rows.Next() { - var summary MonthlySummary - err := rows.Scan( - &summary.Month, - &summary.AccountID, - &summary.Provider, - &summary.Service, - &summary.TotalSavings, - &summary.AvgCoverage, - &summary.SnapshotCount, - ) - if err != nil { - return nil, err - } - summaries = append(summaries, summary) - } - - return summaries, rows.Err() -} - -// QueryByProvider retrieves savings breakdown by provider -func (s *testablePostgresAnalyticsStore) QueryByProvider(ctx context.Context, accountID string, startDate, endDate time.Time) ([]ProviderBreakdown, error) { - query := ` - SELECT provider, service, SUM(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage - FROM savings_snapshots - WHERE account_id = $1 - AND timestamp >= $2 - AND timestamp <= $3 - GROUP BY provider, service - ORDER BY total_savings DESC - ` - - rows, err := s.mock.Query(ctx, query, accountID, startDate, endDate) - if err != nil { - return nil, err - } - defer rows.Close() - - breakdowns := make([]ProviderBreakdown, 0) - for rows.Next() { - var breakdown ProviderBreakdown - err := rows.Scan( - &breakdown.Provider, - &breakdown.Service, - &breakdown.TotalSavings, - &breakdown.AvgCoverage, - ) - if err != nil { - return nil, err - } - breakdowns = append(breakdowns, breakdown) - } - - return breakdowns, rows.Err() -} - -// QueryByService retrieves savings breakdown by service -func (s *testablePostgresAnalyticsStore) QueryByService(ctx context.Context, accountID string, provider string, startDate, endDate time.Time) ([]ServiceBreakdown, error) { - query := ` - SELECT service, region, SUM(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage - FROM savings_snapshots - WHERE account_id = $1 - AND provider = $2 - AND timestamp >= $3 - AND timestamp <= $4 - GROUP BY service, region - ORDER BY total_savings DESC - ` - - rows, err := s.mock.Query(ctx, query, accountID, provider, startDate, endDate) - if err != nil { - return nil, err - } - defer rows.Close() - - breakdowns := make([]ServiceBreakdown, 0) - for rows.Next() { - var breakdown ServiceBreakdown - err := rows.Scan( - &breakdown.Service, - &breakdown.Region, - &breakdown.TotalSavings, - &breakdown.AvgCoverage, - ) - if err != nil { - return nil, err - } - breakdowns = append(breakdowns, breakdown) - } - - return breakdowns, rows.Err() -} - -// CreatePartition creates a partition for a specific month -func (s *testablePostgresAnalyticsStore) CreatePartition(ctx context.Context, forMonth time.Time) error { - query := `SELECT create_savings_snapshot_partition($1)` - - _, err := s.mock.Exec(ctx, query, forMonth) - if err != nil { - return err - } - - return nil -} - -// DropOldPartitions removes partitions older than retention period -func (s *testablePostgresAnalyticsStore) DropOldPartitions(ctx context.Context, retentionMonths int) error { - query := `SELECT drop_old_savings_partitions($1)` - - _, err := s.mock.Exec(ctx, query, retentionMonths) - if err != nil { - return err - } - - return nil -} - -// CreatePartitionsForRange creates partitions for a date range -func (s *testablePostgresAnalyticsStore) CreatePartitionsForRange(ctx context.Context, startDate, endDate time.Time) error { - current := time.Date(startDate.Year(), startDate.Month(), 1, 0, 0, 0, 0, time.UTC) - end := time.Date(endDate.Year(), endDate.Month(), 1, 0, 0, 0, 0, time.UTC) - - for !current.After(end) { - if err := s.CreatePartition(ctx, current); err != nil { - return err - } - current = current.AddDate(0, 1, 0) - } - - return nil -} - -// RefreshMaterializedViews refreshes all analytics materialized views -func (s *testablePostgresAnalyticsStore) RefreshMaterializedViews(ctx context.Context) error { - query := `SELECT refresh_savings_materialized_views()` - - _, err := s.mock.Exec(ctx, query) - if err != nil { - return err - } - - return nil -} - -// Close cleans up resources -func (s *testablePostgresAnalyticsStore) Close() error { - return nil -} +// Verify the wrapper still satisfies AnalyticsStore. +var _ AnalyticsStore = (*testablePostgresAnalyticsStore)(nil) // ===================== // Tests @@ -342,7 +71,7 @@ func TestSaveSnapshot(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() snapshot := &SavingsSnapshot{ @@ -354,9 +83,9 @@ func TestSaveSnapshot(t *testing.T) { Region: "us-east-1", CommitmentType: "RI", TotalCommitment: 100.0, - TotalUsage: 80.0, + TotalUsage: f64ptr(80.0), TotalSavings: 20.0, - CoveragePercentage: 80.0, + CoveragePercentage: f64ptr(80.0), Metadata: map[string]interface{}{"key": "value"}, } @@ -364,6 +93,7 @@ func TestSaveSnapshot(t *testing.T) { WithArgs( snapshot.ID, snapshot.AccountID, + snapshot.CloudAccountID, snapshot.Timestamp, snapshot.Provider, snapshot.Service, @@ -387,31 +117,19 @@ func TestSaveSnapshot(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) snapshot := &SavingsSnapshot{ - ID: "", // empty ID - AccountID: "account-123", - Timestamp: time.Now().UTC(), - Provider: "aws", - Service: "rds", + ID: "", // empty ID + AccountID: "account-123", + Timestamp: time.Now().UTC(), + Provider: "aws", + Service: "rds", + CommitmentType: "RI", } mock.ExpectExec(`INSERT INTO savings_snapshots`). - WithArgs( - "generated-uuid", // should be generated - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - ). + WithArgs(append([]any{"generated-uuid"}, anyArgs(12)...)...). WillReturnResult(pgxmock.NewResult("INSERT", 1)) err = store.SaveSnapshot(context.Background(), snapshot) @@ -425,32 +143,20 @@ func TestSaveSnapshot(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) snapshot := &SavingsSnapshot{ - ID: "test-id", - AccountID: "account-123", - Timestamp: time.Now().UTC(), - Provider: "aws", - Service: "rds", - Metadata: nil, + ID: "test-id", + AccountID: "account-123", + Timestamp: time.Now().UTC(), + Provider: "aws", + Service: "rds", + CommitmentType: "SavingsPlan", + Metadata: nil, } mock.ExpectExec(`INSERT INTO savings_snapshots`). - WithArgs( - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), - pgxmock.AnyArg(), // nil metadata becomes empty byte slice - ). + WithArgs(anyArgs(13)...). // includes nil cloud_account_id + nil metadata WillReturnResult(pgxmock.NewResult("INSERT", 1)) err = store.SaveSnapshot(context.Background(), snapshot) @@ -463,21 +169,17 @@ func TestSaveSnapshot(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) snapshot := &SavingsSnapshot{ - ID: "test-id", - AccountID: "account-123", - Timestamp: time.Now().UTC(), + ID: "test-id", + AccountID: "account-123", + Timestamp: time.Now().UTC(), + CommitmentType: "RI", } mock.ExpectExec(`INSERT INTO savings_snapshots`). - WithArgs( - pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg(), - pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg(), - pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg(), - pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg(), - ). + WithArgs(anyArgs(13)...). WillReturnError(errors.New("database error")) err = store.SaveSnapshot(context.Background(), snapshot) @@ -485,6 +187,29 @@ func TestSaveSnapshot(t *testing.T) { assert.Contains(t, err.Error(), "database error") assert.NoError(t, mock.ExpectationsWereMet()) }) + + t.Run("rejects invalid commitment_type before any DB call", func(t *testing.T) { + mock, err := pgxmock.NewPool() + require.NoError(t, err) + defer mock.Close() + + store := newTestableStore(mock) + + snapshot := &SavingsSnapshot{ + ID: "test-id", + AccountID: "account-123", + Timestamp: time.Now().UTC(), + CommitmentType: "bogus", + } + + // No mock expectations set: if the DB were contacted the test would fail + // with an unexpected call, proving the guard fires client-side. + err = store.SaveSnapshot(context.Background(), snapshot) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid commitment_type") + assert.Contains(t, err.Error(), "bogus") + assert.NoError(t, mock.ExpectationsWereMet()) + }) } // TestBulkInsertSnapshots tests the BulkInsertSnapshots method @@ -494,7 +219,7 @@ func TestBulkInsertSnapshots(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) err = store.BulkInsertSnapshots(context.Background(), []SavingsSnapshot{}) assert.NoError(t, err) @@ -505,11 +230,35 @@ func TestBulkInsertSnapshots(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) - err = store.BulkInsertSnapshots(context.Background(), []SavingsSnapshot{{}}) + // The real store's COPY path acquires a pooled connection; pgxmock's + // Acquire is unimplemented, so a non-empty bulk insert surfaces that. + err = store.BulkInsertSnapshots(context.Background(), []SavingsSnapshot{{CommitmentType: "RI"}}) assert.Error(t, err) - assert.Contains(t, err.Error(), "bulk insert requires real connection") + assert.Contains(t, err.Error(), "failed to acquire connection") + }) + +} + +// TestValidateCommitmentType directly exercises the commitment_type guard that +// BulkInsertSnapshots applies in its COPY builder (L4), since the COPY path +// itself can only be reached with a real pooled connection. +func TestValidateCommitmentType(t *testing.T) { + t.Run("accepts RI", func(t *testing.T) { + assert.NoError(t, validateCommitmentType("RI")) + }) + t.Run("accepts SavingsPlan", func(t *testing.T) { + assert.NoError(t, validateCommitmentType("SavingsPlan")) + }) + t.Run("rejects an unknown value", func(t *testing.T) { + err := validateCommitmentType("bogus") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid commitment_type") + assert.Contains(t, err.Error(), "bogus") + }) + t.Run("rejects empty", func(t *testing.T) { + assert.Error(t, validateCommitmentType("")) }) } @@ -520,29 +269,29 @@ func TestQuerySavings(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) metadataJSON, _ := json.Marshal(map[string]interface{}{"key": "value"}) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }).AddRow( - "snapshot-1", "account-123", now, "aws", "rds", "us-east-1", - "RI", 100.0, 80.0, 20.0, 80.0, metadataJSON, + "snapshot-1", "account-123", strPtr("cloud-1"), now, "aws", "rds", "us-east-1", + "RI", 100.0, f64ptr(80.0), 20.0, f64ptr(80.0), metadataJSON, ) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, } snapshots, err := store.QuerySavings(context.Background(), req) @@ -560,26 +309,26 @@ func TestQuerySavings(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now, "aws"). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - Provider: "aws", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + Provider: "aws", + StartDate: startDate, + EndDate: now, } _, err = store.QuerySavings(context.Background(), req) @@ -592,26 +341,26 @@ func TestQuerySavings(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now, "rds"). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}, "rds"). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - Service: "rds", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + Service: "rds", + StartDate: startDate, + EndDate: now, } _, err = store.QuerySavings(context.Background(), req) @@ -624,26 +373,26 @@ func TestQuerySavings(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now, 10). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}, 10). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, - Limit: 10, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, + Limit: 10, } _, err = store.QuerySavings(context.Background(), req) @@ -656,25 +405,25 @@ func TestQuerySavings(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, } snapshots, err := store.QuerySavings(context.Background(), req) @@ -689,19 +438,19 @@ func TestQuerySavings(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnError(errors.New("database error")) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, } _, err = store.QuerySavings(context.Background(), req) @@ -715,28 +464,28 @@ func TestQuerySavings(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }).AddRow( - "snapshot-1", "account-123", now, "aws", "rds", "us-east-1", - "RI", 100.0, 80.0, 20.0, 80.0, []byte("invalid json"), + "snapshot-1", "account-123", strPtr("cloud-1"), now, "aws", "rds", "us-east-1", + "RI", 100.0, f64ptr(80.0), 20.0, f64ptr(80.0), []byte("invalid json"), ) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, } _, err = store.QuerySavings(context.Background(), req) @@ -752,21 +501,21 @@ func TestQueryMonthlyTotals(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) month := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) rows := pgxmock.NewRows([]string{ - "month", "account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", + "month", "account_id", "cloud_account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", }). - AddRow(month, "account-123", "aws", "rds", 1500.0, 85.0, 720). - AddRow(month, "account-123", "aws", "elasticache", 800.0, 75.0, 720) + AddRow(month, "account-123", strPtr("cloud-1"), "aws", "rds", 1500.0, f64ptr(85.0), 720). + AddRow(month, "account-123", strPtr("cloud-1"), "aws", "elasticache", 800.0, f64ptr(75.0), 720) - mock.ExpectQuery(`SELECT month, account_id, provider, service, total_savings, avg_coverage, snapshot_count`). - WithArgs("account-123", 6). + mock.ExpectQuery(`SELECT month, account_id`). + WithArgs(6, []string{"account-123"}). WillReturnRows(rows) - summaries, err := store.QueryMonthlyTotals(context.Background(), "account-123", 6) + summaries, err := store.QueryMonthlyTotals(context.Background(), []string{"account-123"}, nil, 6) require.NoError(t, err) assert.Len(t, summaries, 2) assert.Equal(t, "rds", summaries[0].Service) @@ -779,13 +528,13 @@ func TestQueryMonthlyTotals(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) - mock.ExpectQuery(`SELECT month, account_id, provider, service, total_savings, avg_coverage, snapshot_count`). - WithArgs("account-123", 6). + mock.ExpectQuery(`SELECT month, account_id`). + WithArgs(6, []string{"account-123"}). WillReturnError(errors.New("database error")) - _, err = store.QueryMonthlyTotals(context.Background(), "account-123", 6) + _, err = store.QueryMonthlyTotals(context.Background(), []string{"account-123"}, nil, 6) assert.Error(t, err) assert.Contains(t, err.Error(), "database error") assert.NoError(t, mock.ExpectationsWereMet()) @@ -796,17 +545,17 @@ func TestQueryMonthlyTotals(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) rows := pgxmock.NewRows([]string{ - "month", "account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", + "month", "account_id", "cloud_account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", }) - mock.ExpectQuery(`SELECT month, account_id, provider, service, total_savings, avg_coverage, snapshot_count`). - WithArgs("account-123", 6). + mock.ExpectQuery(`SELECT month, account_id`). + WithArgs(6, []string{"account-123"}). WillReturnRows(rows) - summaries, err := store.QueryMonthlyTotals(context.Background(), "account-123", 6) + summaries, err := store.QueryMonthlyTotals(context.Background(), []string{"account-123"}, nil, 6) require.NoError(t, err) assert.Empty(t, summaries) assert.NoError(t, mock.ExpectationsWereMet()) @@ -820,7 +569,7 @@ func TestQueryByProvider(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) @@ -828,14 +577,14 @@ func TestQueryByProvider(t *testing.T) { rows := pgxmock.NewRows([]string{ "provider", "service", "total_savings", "avg_coverage", }). - AddRow("aws", "rds", 2500.0, 85.0). - AddRow("aws", "elasticache", 1200.0, 75.0) + AddRow("aws", "rds", 2500.0, f64ptr(85.0)). + AddRow("aws", "elasticache", 1200.0, f64ptr(75.0)) - mock.ExpectQuery(`SELECT provider, service, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) - breakdowns, err := store.QueryByProvider(context.Background(), "account-123", startDate, now) + breakdowns, err := store.QueryByProvider(context.Background(), []string{"account-123"}, nil, startDate, now) require.NoError(t, err) assert.Len(t, breakdowns, 2) assert.Equal(t, "aws", breakdowns[0].Provider) @@ -849,16 +598,16 @@ func TestQueryByProvider(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) - mock.ExpectQuery(`SELECT provider, service, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnError(errors.New("database error")) - _, err = store.QueryByProvider(context.Background(), "account-123", startDate, now) + _, err = store.QueryByProvider(context.Background(), []string{"account-123"}, nil, startDate, now) assert.Error(t, err) assert.Contains(t, err.Error(), "database error") assert.NoError(t, mock.ExpectationsWereMet()) @@ -872,7 +621,7 @@ func TestQueryByService(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) @@ -880,14 +629,14 @@ func TestQueryByService(t *testing.T) { rows := pgxmock.NewRows([]string{ "service", "region", "total_savings", "avg_coverage", }). - AddRow("rds", "us-east-1", 1800.0, 90.0). - AddRow("rds", "us-west-2", 700.0, 75.0) + AddRow("rds", "us-east-1", 1800.0, f64ptr(90.0)). + AddRow("rds", "us-west-2", 700.0, f64ptr(75.0)) - mock.ExpectQuery(`SELECT service, region, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", "aws", startDate, now). + mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnRows(rows) - breakdowns, err := store.QueryByService(context.Background(), "account-123", "aws", startDate, now) + breakdowns, err := store.QueryByService(context.Background(), []string{"account-123"}, nil, "aws", startDate, now) require.NoError(t, err) assert.Len(t, breakdowns, 2) assert.Equal(t, "rds", breakdowns[0].Service) @@ -901,16 +650,16 @@ func TestQueryByService(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) - mock.ExpectQuery(`SELECT service, region, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", "aws", startDate, now). + mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnError(errors.New("database error")) - _, err = store.QueryByService(context.Background(), "account-123", "aws", startDate, now) + _, err = store.QueryByService(context.Background(), []string{"account-123"}, nil, "aws", startDate, now) assert.Error(t, err) assert.Contains(t, err.Error(), "database error") assert.NoError(t, mock.ExpectationsWereMet()) @@ -924,7 +673,7 @@ func TestCreatePartition(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) forMonth := time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC) @@ -942,7 +691,7 @@ func TestCreatePartition(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) forMonth := time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC) @@ -964,7 +713,7 @@ func TestDropOldPartitions(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) mock.ExpectExec(`SELECT drop_old_savings_partitions`). WithArgs(12). @@ -980,7 +729,7 @@ func TestDropOldPartitions(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) mock.ExpectExec(`SELECT drop_old_savings_partitions`). WithArgs(12). @@ -1000,7 +749,7 @@ func TestCreatePartitionsForRange(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) startDate := time.Date(2024, 1, 15, 0, 0, 0, 0, time.UTC) endDate := time.Date(2024, 3, 20, 0, 0, 0, 0, time.UTC) @@ -1030,7 +779,7 @@ func TestCreatePartitionsForRange(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) startDate := time.Date(2024, 1, 15, 0, 0, 0, 0, time.UTC) endDate := time.Date(2024, 3, 20, 0, 0, 0, 0, time.UTC) @@ -1059,7 +808,7 @@ func TestRefreshMaterializedViews(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) mock.ExpectExec(`SELECT refresh_savings_materialized_views`). WillReturnResult(pgxmock.NewResult("SELECT", 1)) @@ -1074,7 +823,7 @@ func TestRefreshMaterializedViews(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) mock.ExpectExec(`SELECT refresh_savings_materialized_views`). WillReturnError(errors.New("refresh error")) @@ -1099,9 +848,9 @@ func TestSavingsSnapshot(t *testing.T) { Region: "us-east-1", CommitmentType: "RI", TotalCommitment: 100.50, - TotalUsage: 80.25, + TotalUsage: f64ptr(80.25), TotalSavings: 20.25, - CoveragePercentage: 80.0, + CoveragePercentage: f64ptr(80.0), Metadata: map[string]interface{}{ "key": "value", }, @@ -1115,9 +864,11 @@ func TestSavingsSnapshot(t *testing.T) { assert.Equal(t, "us-east-1", snapshot.Region) assert.Equal(t, "RI", snapshot.CommitmentType) assert.InDelta(t, 100.50, snapshot.TotalCommitment, 0.001) - assert.InDelta(t, 80.25, snapshot.TotalUsage, 0.001) + require.NotNil(t, snapshot.TotalUsage) + assert.InDelta(t, 80.25, *snapshot.TotalUsage, 0.001) assert.InDelta(t, 20.25, snapshot.TotalSavings, 0.001) - assert.InDelta(t, 80.0, snapshot.CoveragePercentage, 0.001) + require.NotNil(t, snapshot.CoveragePercentage) + assert.InDelta(t, 80.0, *snapshot.CoveragePercentage, 0.001) assert.Equal(t, "value", snapshot.Metadata["key"]) }) @@ -1132,9 +883,9 @@ func TestSavingsSnapshot(t *testing.T) { Region: "us-east-1", CommitmentType: "RI", TotalCommitment: 100.50, - TotalUsage: 80.25, + TotalUsage: f64ptr(80.25), TotalSavings: 20.25, - CoveragePercentage: 80.0, + CoveragePercentage: f64ptr(80.0), } data, err := json.Marshal(snapshot) @@ -1157,15 +908,15 @@ func TestQueryRequest(t *testing.T) { start := time.Now().Add(-24 * time.Hour) end := time.Now() req := QueryRequest{ - AccountID: "account-123", - Provider: "aws", - Service: "rds", - StartDate: start, - EndDate: end, - Limit: 100, + AccountUUIDs: []string{"account-123"}, + Provider: "aws", + Service: "rds", + StartDate: start, + EndDate: end, + Limit: 100, } - assert.Equal(t, "account-123", req.AccountID) + assert.Equal(t, []string{"account-123"}, req.AccountUUIDs) assert.Equal(t, "aws", req.Provider) assert.Equal(t, "rds", req.Service) assert.Equal(t, start, req.StartDate) @@ -1175,9 +926,9 @@ func TestQueryRequest(t *testing.T) { t.Run("handles optional fields", func(t *testing.T) { req := QueryRequest{ - AccountID: "account-123", - StartDate: time.Now().Add(-24 * time.Hour), - EndDate: time.Now(), + AccountUUIDs: []string{"account-123"}, + StartDate: time.Now().Add(-24 * time.Hour), + EndDate: time.Now(), } assert.Equal(t, "", req.Provider) // Optional, can be empty @@ -1196,7 +947,7 @@ func TestMonthlySummary(t *testing.T) { Provider: "aws", Service: "rds", TotalSavings: 1500.50, - AvgCoverage: 85.5, + AvgCoverage: f64ptr(85.5), SnapshotCount: 720, } @@ -1205,7 +956,8 @@ func TestMonthlySummary(t *testing.T) { assert.Equal(t, "aws", summary.Provider) assert.Equal(t, "rds", summary.Service) assert.InDelta(t, 1500.50, summary.TotalSavings, 0.001) - assert.InDelta(t, 85.5, summary.AvgCoverage, 0.001) + require.NotNil(t, summary.AvgCoverage) + assert.InDelta(t, 85.5, *summary.AvgCoverage, 0.001) assert.Equal(t, 720, summary.SnapshotCount) }) @@ -1217,7 +969,7 @@ func TestMonthlySummary(t *testing.T) { Provider: "aws", Service: "rds", TotalSavings: 1500.50, - AvgCoverage: 85.5, + AvgCoverage: f64ptr(85.5), SnapshotCount: 720, } @@ -1241,13 +993,14 @@ func TestProviderBreakdown(t *testing.T) { Provider: "aws", Service: "rds", TotalSavings: 2500.75, - AvgCoverage: 90.5, + AvgCoverage: f64ptr(90.5), } assert.Equal(t, "aws", breakdown.Provider) assert.Equal(t, "rds", breakdown.Service) assert.InDelta(t, 2500.75, breakdown.TotalSavings, 0.001) - assert.InDelta(t, 90.5, breakdown.AvgCoverage, 0.001) + require.NotNil(t, breakdown.AvgCoverage) + assert.InDelta(t, 90.5, *breakdown.AvgCoverage, 0.001) }) t.Run("json marshaling works correctly", func(t *testing.T) { @@ -1255,7 +1008,7 @@ func TestProviderBreakdown(t *testing.T) { Provider: "gcp", Service: "cloudsql", TotalSavings: 1200.00, - AvgCoverage: 75.0, + AvgCoverage: f64ptr(75.0), } data, err := json.Marshal(breakdown) @@ -1277,13 +1030,14 @@ func TestServiceBreakdown(t *testing.T) { Service: "elasticache", Region: "us-west-2", TotalSavings: 800.25, - AvgCoverage: 82.0, + AvgCoverage: f64ptr(82.0), } assert.Equal(t, "elasticache", breakdown.Service) assert.Equal(t, "us-west-2", breakdown.Region) assert.InDelta(t, 800.25, breakdown.TotalSavings, 0.001) - assert.InDelta(t, 82.0, breakdown.AvgCoverage, 0.001) + require.NotNil(t, breakdown.AvgCoverage) + assert.InDelta(t, 82.0, *breakdown.AvgCoverage, 0.001) }) t.Run("json marshaling works correctly", func(t *testing.T) { @@ -1291,7 +1045,7 @@ func TestServiceBreakdown(t *testing.T) { Service: "memorystore", Region: "us-central1", TotalSavings: 450.00, - AvgCoverage: 70.0, + AvgCoverage: f64ptr(70.0), } data, err := json.Marshal(breakdown) @@ -1322,7 +1076,7 @@ func TestQuerySavingsRowScanError(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) @@ -1332,14 +1086,14 @@ func TestQuerySavingsRowScanError(t *testing.T) { "id", "account_id", // Missing other columns }).AddRow("snapshot-1", "account-123").RowError(0, errors.New("scan error")) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, } _, err = store.QuerySavings(context.Background(), req) @@ -1355,17 +1109,17 @@ func TestQueryMonthlyTotalsRowScanError(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) rows := pgxmock.NewRows([]string{ "month", "account_id", // Missing other columns }).AddRow(time.Now(), "account-123").RowError(0, errors.New("scan error")) - mock.ExpectQuery(`SELECT month, account_id, provider, service, total_savings, avg_coverage, snapshot_count`). - WithArgs("account-123", 6). + mock.ExpectQuery(`SELECT month, account_id`). + WithArgs(6, []string{"account-123"}). WillReturnRows(rows) - _, err = store.QueryMonthlyTotals(context.Background(), "account-123", 6) + _, err = store.QueryMonthlyTotals(context.Background(), []string{"account-123"}, nil, 6) assert.Error(t, err) assert.NoError(t, mock.ExpectationsWereMet()) }) @@ -1378,7 +1132,7 @@ func TestQueryByProviderRowScanError(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) @@ -1387,11 +1141,11 @@ func TestQueryByProviderRowScanError(t *testing.T) { "provider", // Missing other columns }).AddRow("aws").RowError(0, errors.New("scan error")) - mock.ExpectQuery(`SELECT provider, service, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) - _, err = store.QueryByProvider(context.Background(), "account-123", startDate, now) + _, err = store.QueryByProvider(context.Background(), []string{"account-123"}, nil, startDate, now) assert.Error(t, err) assert.NoError(t, mock.ExpectationsWereMet()) }) @@ -1404,7 +1158,7 @@ func TestQueryByServiceRowScanError(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) @@ -1413,11 +1167,11 @@ func TestQueryByServiceRowScanError(t *testing.T) { "service", // Missing other columns }).AddRow("rds").RowError(0, errors.New("scan error")) - mock.ExpectQuery(`SELECT service, region, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", "aws", startDate, now). + mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnRows(rows) - _, err = store.QueryByService(context.Background(), "account-123", "aws", startDate, now) + _, err = store.QueryByService(context.Background(), []string{"account-123"}, nil, "aws", startDate, now) assert.Error(t, err) assert.NoError(t, mock.ExpectationsWereMet()) }) @@ -1430,29 +1184,29 @@ func TestRowsErr(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) metadataJSON, _ := json.Marshal(map[string]interface{}{}) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }).AddRow( - "snapshot-1", "account-123", now, "aws", "rds", "us-east-1", - "RI", 100.0, 80.0, 20.0, 80.0, metadataJSON, + "snapshot-1", "account-123", strPtr("cloud-1"), now, "aws", "rds", "us-east-1", + "RI", 100.0, f64ptr(80.0), 20.0, f64ptr(80.0), metadataJSON, ) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, } snapshots, err := store.QuerySavings(context.Background(), req) @@ -1469,19 +1223,19 @@ func TestQueryMonthlyTotalsRowsErr(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) month := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) rows := pgxmock.NewRows([]string{ - "month", "account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", - }).AddRow(month, "account-123", "aws", "rds", 1500.0, 85.0, 720) + "month", "account_id", "cloud_account_id", "provider", "service", "total_savings", "avg_coverage", "snapshot_count", + }).AddRow(month, "account-123", strPtr("cloud-1"), "aws", "rds", 1500.0, f64ptr(85.0), 720) - mock.ExpectQuery(`SELECT month, account_id, provider, service, total_savings, avg_coverage, snapshot_count`). - WithArgs("account-123", 6). + mock.ExpectQuery(`SELECT month, account_id`). + WithArgs(6, []string{"account-123"}). WillReturnRows(rows) - summaries, err := store.QueryMonthlyTotals(context.Background(), "account-123", 6) + summaries, err := store.QueryMonthlyTotals(context.Background(), []string{"account-123"}, nil, 6) require.NoError(t, err) assert.Len(t, summaries, 1) assert.NoError(t, mock.ExpectationsWereMet()) @@ -1495,20 +1249,20 @@ func TestQueryByProviderRowsErr(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) rows := pgxmock.NewRows([]string{ "provider", "service", "total_savings", "avg_coverage", - }).AddRow("aws", "rds", 2500.0, 85.0) + }).AddRow("aws", "rds", 2500.0, f64ptr(85.0)) - mock.ExpectQuery(`SELECT provider, service, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) - breakdowns, err := store.QueryByProvider(context.Background(), "account-123", startDate, now) + breakdowns, err := store.QueryByProvider(context.Background(), []string{"account-123"}, nil, startDate, now) require.NoError(t, err) assert.Len(t, breakdowns, 1) assert.NoError(t, mock.ExpectationsWereMet()) @@ -1522,20 +1276,20 @@ func TestQueryByServiceRowsErr(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) rows := pgxmock.NewRows([]string{ "service", "region", "total_savings", "avg_coverage", - }).AddRow("rds", "us-east-1", 1800.0, 90.0) + }).AddRow("rds", "us-east-1", 1800.0, f64ptr(90.0)) - mock.ExpectQuery(`SELECT service, region, SUM\(total_savings\) as total_savings`). - WithArgs("account-123", "aws", startDate, now). + mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnRows(rows) - breakdowns, err := store.QueryByService(context.Background(), "account-123", "aws", startDate, now) + breakdowns, err := store.QueryByService(context.Background(), []string{"account-123"}, nil, "aws", startDate, now) require.NoError(t, err) assert.Len(t, breakdowns, 1) assert.NoError(t, mock.ExpectationsWereMet()) @@ -1549,25 +1303,25 @@ func TestErrNoRowsHandling(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) now := time.Now().UTC() startDate := now.Add(-24 * time.Hour) rows := pgxmock.NewRows([]string{ - "id", "account_id", "timestamp", "provider", "service", "region", + "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }) - mock.ExpectQuery(`SELECT id, account_id, timestamp, provider, service, region`). - WithArgs("account-123", startDate, now). + mock.ExpectQuery(`SELECT id, account_id`). + WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) req := QueryRequest{ - AccountID: "account-123", - StartDate: startDate, - EndDate: now, + AccountUUIDs: []string{"account-123"}, + StartDate: startDate, + EndDate: now, } snapshots, err := store.QuerySavings(context.Background(), req) @@ -1585,7 +1339,7 @@ func TestTestableStoreImplementsInterface(t *testing.T) { require.NoError(t, err) defer mock.Close() - var store AnalyticsStore = &testablePostgresAnalyticsStore{mock: mock} + var store AnalyticsStore = newTestableStore(mock) assert.NotNil(t, store) }) } @@ -1597,7 +1351,7 @@ func TestClose(t *testing.T) { require.NoError(t, err) defer mock.Close() - store := &testablePostgresAnalyticsStore{mock: mock} + store := newTestableStore(mock) err = store.Close() assert.NoError(t, err) }) diff --git a/internal/api/handler.go b/internal/api/handler.go index 6f621db64..24ec49e8a 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -34,12 +34,13 @@ type Handler struct { apiKey string // Cached API key corsAllowedOrigin string // CORS allowed origin rateLimiter RateLimiterInterface - emailNotifier email.SenderInterface // Optional: purchase approval emails - dashboardURL string // Base URL for approval/cancel links - analyticsClient AnalyticsClientInterface // Optional: analytics client (Postgres-backed in prod) - analyticsCollector AnalyticsCollectorInterface // Optional: Hourly collector - signer oidc.Signer // Optional: OIDC issuer signer (backed by cloud KMS) - issuerURL string // Canonical OIDC issuer URL (falls back to dashboardURL / request domain) + emailNotifier email.SenderInterface // Optional: purchase approval emails + dashboardURL string // Base URL for approval/cancel links + analyticsClient AnalyticsClientInterface // Optional: analytics client (Postgres-backed in prod) + analyticsCollector AnalyticsCollectorInterface // Optional: snapshot collector + analyticsSnapshots AnalyticsSnapshotStoreInterface // Optional: savings-snapshot time-series store + signer oidc.Signer // Optional: OIDC issuer signer (backed by cloud KMS) + issuerURL string // Canonical OIDC issuer URL (falls back to dashboardURL / request domain) awsCfgOnce sync.Once // guards one-time loading of the base AWS config awsCfg aws.Config // cached base AWS config (no region override) @@ -157,6 +158,7 @@ func NewHandler(cfg HandlerConfig) *Handler { dashboardURL: cfg.DashboardURL, analyticsClient: cfg.AnalyticsClient, analyticsCollector: cfg.AnalyticsCollector, + analyticsSnapshots: cfg.AnalyticsSnapshots, signer: cfg.OIDCSigner, issuerURL: cfg.OIDCIssuerURL, commitmentOpts: cfg.CommitmentOpts, diff --git a/internal/api/handler_analytics.go b/internal/api/handler_analytics.go index 84dfeb7d8..48d26d480 100644 --- a/internal/api/handler_analytics.go +++ b/internal/api/handler_analytics.go @@ -6,10 +6,91 @@ import ( "fmt" "time" + "github.com/LeanerCloud/CUDly/internal/analytics" "github.com/LeanerCloud/CUDly/internal/auth" "github.com/aws/aws-lambda-go/events" ) +// TrendsResponse is the savings-snapshot time-series for the Trends view: a +// monthly series (coverage %, committed spend, usage, realized savings) plus +// by-provider and by-service breakdowns over the requested window. Backed by +// the savings_snapshots store / materialized views (issues #1023 / #1033), +// distinct from the purchase_history-backed /history/analytics path. +type TrendsResponse struct { + Start string `json:"start"` + End string `json:"end"` + Months int `json:"months"` + Monthly []analytics.MonthlySummary `json:"monthly"` + Provider []analytics.ProviderBreakdown `json:"by_provider"` + Service []analytics.ServiceBreakdown `json:"by_service"` +} + +// getAnalyticsTrends handles GET /api/analytics/trends. It returns the +// historical savings-snapshot series scoped to the caller's allowed_accounts. +func (h *Handler) getAnalyticsTrends(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + session, err := h.requirePermission(ctx, req, "view", "purchases") + if err != nil { + return nil, err + } + + if h.analyticsSnapshots == nil { + // Mirror getHistoryAnalytics: 503 = feature intentionally unavailable. + return nil, NewClientError(503, "analytics snapshots not configured") + } + + accountID := params["account_id"] + // Enforce allowed_accounts scope BEFORE resolving filter ids so a scoped + // user can never widen to "all" (empty filters mean all-accessible). + if err := h.validateAnalyticsAccountScope(ctx, session, accountID); err != nil { + return nil, err + } + + start, end, err := parseDateRange(params["start"], params["end"]) + if err != nil { + return nil, err + } + + months := monthsBetween(start, end) + + // Resolve the requested account (top-bar chip UUID, or "" for all-accessible + // for an unrestricted session) into the dual-column filter inputs so rows + // carrying only the external account_id (cloud_account_id NULL) are matched. + accountUUIDs, accountExternalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, accountID) + + monthly, err := h.analyticsSnapshots.QueryMonthlyTotals(ctx, accountUUIDs, accountExternalIDsByProvider, months) + if err != nil { + return nil, fmt.Errorf("failed to query monthly totals: %w", err) + } + byProvider, err := h.analyticsSnapshots.QueryByProvider(ctx, accountUUIDs, accountExternalIDsByProvider, start, end) + if err != nil { + return nil, fmt.Errorf("failed to query provider breakdown: %w", err) + } + provider := params["provider"] + byService, err := h.analyticsSnapshots.QueryByService(ctx, accountUUIDs, accountExternalIDsByProvider, provider, start, end) + if err != nil { + return nil, fmt.Errorf("failed to query service breakdown: %w", err) + } + + return &TrendsResponse{ + Start: start.Format(time.RFC3339), + End: end.Format(time.RFC3339), + Months: months, + Monthly: monthly, + Provider: byProvider, + Service: byService, + }, nil +} + +// monthsBetween returns the inclusive count of month-buckets spanned by +// [start, end], at least 1. Used to bound the monthly_savings_summary query. +func monthsBetween(start, end time.Time) int { + months := (end.Year()-start.Year())*12 + int(end.Month()) - int(start.Month()) + 1 + if months < 1 { + return 1 + } + return months +} + // AnalyticsResponse represents the response for the analytics endpoint. type AnalyticsResponse struct { Start string `json:"start"` @@ -187,7 +268,7 @@ func parseDateRange(startStr, endStr string) (time.Time, time.Time, error) { // Try date-only format end, err = time.Parse("2006-01-02", endStr) if err != nil { - return time.Time{}, time.Time{}, fmt.Errorf("invalid end date format") + return time.Time{}, time.Time{}, NewClientError(400, "invalid end date format") } // Set to end of day end = end.Add(24*time.Hour - time.Second) @@ -203,14 +284,14 @@ func parseDateRange(startStr, endStr string) (time.Time, time.Time, error) { // Try date-only format start, err = time.Parse("2006-01-02", startStr) if err != nil { - return time.Time{}, time.Time{}, fmt.Errorf("invalid start date format") + return time.Time{}, time.Time{}, NewClientError(400, "invalid start date format") } } } // Validate range order. if start.After(end) { - return time.Time{}, time.Time{}, fmt.Errorf("start date must be before end date") + return time.Time{}, time.Time{}, NewClientError(400, "start date must be before end date") } // Cap the range to at most 366 days to prevent full-table-scan DoS via diff --git a/internal/api/handler_analytics_test.go b/internal/api/handler_analytics_test.go index 7253887bc..40e957002 100644 --- a/internal/api/handler_analytics_test.go +++ b/internal/api/handler_analytics_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/CUDly/internal/analytics" "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" @@ -413,25 +414,38 @@ func TestParseDateRange(t *testing.T) { assert.Equal(t, 23, end.Hour()) }) - t.Run("invalid start date format", func(t *testing.T) { + // CR #1049: malformed/ordered-date validation must surface as a 400 + // ClientError so handler.go does not map it to HTTP 500. The plain-error + // pre-fix code would have passed the substring asserts below but failed the + // IsClientError/400 asserts. + t.Run("invalid start date format returns 400 client error", func(t *testing.T) { _, _, err := parseDateRange("not-a-date", "") require.Error(t, err) assert.Contains(t, err.Error(), "invalid start date") + ce, ok := IsClientError(err) + require.True(t, ok, "invalid start date must return a ClientError, got %T", err) + assert.Equal(t, 400, ce.code) }) - t.Run("invalid end date format", func(t *testing.T) { + t.Run("invalid end date format returns 400 client error", func(t *testing.T) { _, _, err := parseDateRange("", "also-not-a-date") require.Error(t, err) assert.Contains(t, err.Error(), "invalid end date") + ce, ok := IsClientError(err) + require.True(t, ok, "invalid end date must return a ClientError, got %T", err) + assert.Equal(t, 400, ce.code) }) - t.Run("start after end returns error", func(t *testing.T) { + t.Run("start after end returns 400 client error", func(t *testing.T) { startStr := "2024-01-31T00:00:00Z" endStr := "2024-01-01T00:00:00Z" _, _, err := parseDateRange(startStr, endStr) require.Error(t, err) assert.Contains(t, err.Error(), "start date must be before end date") + ce, ok := IsClientError(err) + require.True(t, ok, "reversed range must return a ClientError, got %T", err) + assert.Equal(t, 400, ce.code) }) // Regression #414: unbounded date ranges caused full-table scans (DoS). @@ -467,3 +481,132 @@ func TestParseDateRange(t *testing.T) { assert.Equal(t, 400, ce.code) }) } + +// MockAnalyticsSnapshotStore implements AnalyticsSnapshotStoreInterface. +type MockAnalyticsSnapshotStore struct { + mock.Mock +} + +func (m *MockAnalyticsSnapshotStore) QuerySavings(ctx context.Context, req analytics.QueryRequest) ([]analytics.SavingsSnapshot, error) { + args := m.Called(ctx, req) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]analytics.SavingsSnapshot), args.Error(1) +} + +func (m *MockAnalyticsSnapshotStore) QueryMonthlyTotals(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, months int) ([]analytics.MonthlySummary, error) { + args := m.Called(ctx, accountUUIDs, accountExternalIDsByProvider, months) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]analytics.MonthlySummary), args.Error(1) +} + +func (m *MockAnalyticsSnapshotStore) QueryByProvider(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time) ([]analytics.ProviderBreakdown, error) { + args := m.Called(ctx, accountUUIDs, accountExternalIDsByProvider, start, end) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]analytics.ProviderBreakdown), args.Error(1) +} + +func (m *MockAnalyticsSnapshotStore) QueryByService(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, start, end time.Time) ([]analytics.ServiceBreakdown, error) { + args := m.Called(ctx, accountUUIDs, accountExternalIDsByProvider, provider, start, end) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]analytics.ServiceBreakdown), args.Error(1) +} + +// TestHandler_getAnalyticsTrends_Success_ScopesByAccount verifies the trends +// endpoint resolves the requested account UUID into the dual-column filter and +// passes it to every Query* call. +func TestHandler_getAnalyticsTrends_Success_ScopesByAccount(t *testing.T) { + ctx := context.Background() + accountUUID := "bbbbbbbb-1111-2222-3333-444444444444" + accountExternal := "999988887777" + + mockSnap := new(MockAnalyticsSnapshotStore) + wantUUIDs := []string{accountUUID} + wantExt := map[string][]string{"aws": {accountExternal}} + mockSnap.On("QueryMonthlyTotals", ctx, wantUUIDs, wantExt, mock.Anything). + Return([]analytics.MonthlySummary{{Provider: "aws", Service: "rds", TotalSavings: 100}}, nil) + mockSnap.On("QueryByProvider", ctx, wantUUIDs, wantExt, mock.Anything, mock.Anything). + Return([]analytics.ProviderBreakdown{{Provider: "aws", TotalSavings: 100}}, nil) + mockSnap.On("QueryByService", ctx, wantUUIDs, wantExt, "", mock.Anything, mock.Anything). + Return([]analytics.ServiceBreakdown{{Service: "rds", TotalSavings: 100}}, nil) + t.Cleanup(func() { mockSnap.AssertExpectations(t) }) + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: accountUUID, Name: "Account B", Provider: "aws", ExternalID: accountExternal}}, nil + } + + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsSnapshots: mockSnap, config: mockStore} + + result, err := handler.getAnalyticsTrends(ctx, req, map[string]string{"account_id": accountUUID}) + require.NoError(t, err) + resp, ok := result.(*TrendsResponse) + require.True(t, ok) + assert.Len(t, resp.Monthly, 1) + assert.Len(t, resp.Provider, 1) + assert.Len(t, resp.Service, 1) +} + +// TestHandler_getAnalyticsTrends_NoStore returns 503 when the snapshot store is +// not configured. +func TestHandler_getAnalyticsTrends_NoStore(t *testing.T) { + ctx := context.Background() + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth} + + _, err := handler.getAnalyticsTrends(ctx, req, map[string]string{}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 503, ce.code) +} + +// TestHandler_getAnalyticsTrends_ScopedUser_RequiresAccountID asserts a scoped +// user cannot issue an unscoped trends query, and the store is never called. +func TestHandler_getAnalyticsTrends_ScopedUser_RequiresAccountID(t *testing.T) { + ctx := context.Background() + mockSnap := new(MockAnalyticsSnapshotStore) + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "viewer-token").Return(&Session{UserID: "viewer-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "viewer-1", "view", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "viewer-1").Return([]string{"Production"}, nil) + + handler := &Handler{auth: mockAuth, analyticsSnapshots: mockSnap, config: new(MockConfigStore)} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer viewer-token"}} + + _, err := handler.getAnalyticsTrends(ctx, req, map[string]string{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "account_id is required") + mockSnap.AssertNotCalled(t, "QueryMonthlyTotals") +} + +// TestHandler_getAnalyticsTrends_ScopedUser_OutsideScope returns not-found when +// a scoped user requests an account outside their allowed_accounts. +func TestHandler_getAnalyticsTrends_ScopedUser_OutsideScope(t *testing.T) { + ctx := context.Background() + mockSnap := new(MockAnalyticsSnapshotStore) + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "viewer-token").Return(&Session{UserID: "viewer-1"}, nil) + mockAuth.On("HasPermissionAPI", ctx, "viewer-1", "view", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, "viewer-1").Return([]string{"Production"}, nil) + + mockStore := new(MockConfigStore) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: "other-acct", Name: "Staging", Provider: "aws", ExternalID: "111122223333"}}, nil + } + + handler := &Handler{auth: mockAuth, analyticsSnapshots: mockSnap, config: mockStore} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer viewer-token"}} + + _, err := handler.getAnalyticsTrends(ctx, req, map[string]string{"account_id": "other-acct"}) + require.Error(t, err) + mockSnap.AssertNotCalled(t, "QueryMonthlyTotals") +} diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index c0603f761..733cf9f9c 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -204,6 +204,14 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) } +func (m *MockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, asOf) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) +} + func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { args := m.Called(ctx, filter) if args.Get(0) == nil { diff --git a/internal/api/router.go b/internal/api/router.go index 9e3adfcf3..344db42c1 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -196,6 +196,9 @@ func (r *Router) registerRoutes() { {ExactPath: "/api/history/analytics", Method: "GET", Handler: r.getHistoryAnalyticsHandler, Auth: AuthUser}, {ExactPath: "/api/history/breakdown", Method: "GET", Handler: r.getHistoryBreakdownHandler, Auth: AuthUser}, + // Savings-snapshot time-series (Trends view) — scoped to allowed_accounts. + {ExactPath: "/api/analytics/trends", Method: "GET", Handler: r.getAnalyticsTrendsHandler, Auth: AuthUser}, + // Analytics collection endpoint {ExactPath: "/api/analytics/collect", Method: "POST", Handler: r.triggerAnalyticsCollectionHandler, Auth: AuthAdmin}, @@ -573,6 +576,10 @@ func (r *Router) getHistoryBreakdownHandler(ctx context.Context, req *events.Lam return r.h.getHistoryBreakdown(ctx, req, req.QueryStringParameters) } +func (r *Router) getAnalyticsTrendsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.getAnalyticsTrends(ctx, req, req.QueryStringParameters) +} + func (r *Router) triggerAnalyticsCollectionHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.triggerAnalyticsCollection(ctx, req, req.QueryStringParameters) } diff --git a/internal/api/types.go b/internal/api/types.go index 65ccf2522..f0ba78970 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -6,6 +6,7 @@ import ( "sync" "time" + "github.com/LeanerCloud/CUDly/internal/analytics" "github.com/LeanerCloud/CUDly/internal/commitmentopts" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/internal/credentials" @@ -61,6 +62,10 @@ type HandlerConfig struct { // Analytics configuration (optional) AnalyticsClient AnalyticsClientInterface AnalyticsCollector AnalyticsCollectorInterface + // AnalyticsSnapshots serves the savings-snapshot time-series (coverage %, + // utilization, committed spend, realized savings over time) backed by the + // savings_snapshots store. Optional; nil disables /api/analytics/trends. + AnalyticsSnapshots AnalyticsSnapshotStoreInterface // OIDCSigner is the cloud-agnostic signer that backs // /.well-known/openid-configuration and /.well-known/jwks.json. // Nil disables the OIDC issuer endpoints (they return 404). @@ -109,6 +114,20 @@ type AnalyticsCollectorInterface interface { Collect(ctx context.Context) error } +// AnalyticsSnapshotStoreInterface exposes the savings-snapshot time-series for +// the /api/analytics/trends endpoint. It is the read side of the now-wired +// internal/analytics collector. Scoping is the dual-column model: the handler +// resolves the requested account into accountUUIDs + accountExternalIDsByProvider +// (see resolveSingleAccountFilterIDs) and the store ORs both columns so rows +// carrying only one identifier are still matched. Both empty means "all" — the +// handler MUST enforce allowed_accounts scope before passing empty filters. +type AnalyticsSnapshotStoreInterface interface { + QuerySavings(ctx context.Context, req analytics.QueryRequest) ([]analytics.SavingsSnapshot, error) + QueryMonthlyTotals(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, months int) ([]analytics.MonthlySummary, error) + QueryByProvider(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, startDate, endDate time.Time) ([]analytics.ProviderBreakdown, error) + QueryByService(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, startDate, endDate time.Time) ([]analytics.ServiceBreakdown, error) +} + // HistoryDataPoint represents aggregated historical data type HistoryDataPoint struct { Timestamp time.Time `json:"timestamp"` diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 541a02c7a..e0186f26a 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -93,6 +93,14 @@ type StoreInterface interface { SavePurchaseHistory(ctx context.Context, record *PurchaseHistoryRecord) error GetPurchaseHistory(ctx context.Context, accountID string, limit int) ([]PurchaseHistoryRecord, error) GetAllPurchaseHistory(ctx context.Context, limit int) ([]PurchaseHistoryRecord, error) + // GetActivePurchaseHistory returns every purchase_history row whose commitment + // is still within its term at asOf (term > 0 AND timestamp + term years > asOf), + // across all accounts, newest-first. Unlike GetAllPurchaseHistory it is not + // row-capped: the analytics collector needs the complete active set, and + // filtering expired commitments in SQL keeps the result bounded by the number + // of live commitments rather than by all history ever recorded (so it cannot + // silently truncate older-but-still-active 1y/3y commitments). + GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]PurchaseHistoryRecord, error) // GetPurchaseHistoryFiltered reads purchase_history rows matching the // PurchaseHistoryFilter, newest-first, capped at filter.Limit. Each field is // applied independently and only when populated (see PurchaseHistoryFilter). diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b489e70d6..3f0919f79 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1411,6 +1411,27 @@ func (s *PostgresStore) GetAllPurchaseHistory(ctx context.Context, limit int) ([ return s.queryPurchaseHistory(ctx, query, limit) } +// GetActivePurchaseHistory retrieves every purchase_history row still within its +// commitment term at asOf, across all accounts. The active filter is pushed into +// SQL so the result is bounded by the number of live commitments (not by all +// history ever recorded), which is what the analytics collector needs and avoids +// silently truncating older-but-still-active 1y/3y commitments. term*8760 hours +// matches the collector's HoursPerYear (365*24) so the SQL and Go term windows +// agree. +func (s *PostgresStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]PurchaseHistoryRecord, error) { + query := ` + SELECT account_id, purchase_id, timestamp, provider, service, region, + resource_type, count, term, payment, upfront_cost, monthly_cost, + estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id + FROM purchase_history + WHERE term > 0 + AND timestamp + make_interval(hours => term * 8760) > $1 + ORDER BY timestamp DESC + ` + + return s.queryPurchaseHistory(ctx, query, asOf) +} + // appendAccountPredicate pulls the dual-column account-predicate arg-building // branch out of GetPurchaseHistoryFiltered to keep it under the cyclomatic limit. // diff --git a/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.down.sql b/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.down.sql new file mode 100644 index 000000000..f10cf50ad --- /dev/null +++ b/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.down.sql @@ -0,0 +1,108 @@ +-- Revert the materialized views to the account_id-only grain (000003 shape) +-- and restore the NOT NULL DEFAULT 0 columns + the original +-- drop_old_savings_partitions WHEN OTHERS handler. + +DROP MATERIALIZED VIEW IF EXISTS provider_savings_summary CASCADE; +DROP MATERIALIZED VIEW IF EXISTS daily_savings_trend CASCADE; +DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; + +CREATE MATERIALIZED VIEW monthly_savings_summary AS +SELECT + DATE_TRUNC('month', timestamp) as month, + account_id, + provider, + service, + SUM(total_savings) as total_savings, + AVG(coverage_percentage) as avg_coverage, + SUM(total_commitment) as total_commitment, + SUM(total_usage) as total_usage, + COUNT(*) as snapshot_count, + MAX(timestamp) as last_updated +FROM savings_snapshots +GROUP BY DATE_TRUNC('month', timestamp), account_id, provider, service; + +CREATE UNIQUE INDEX idx_monthly_savings_summary_unique + ON monthly_savings_summary(month, account_id, provider, service); + +CREATE MATERIALIZED VIEW daily_savings_trend AS +SELECT + DATE_TRUNC('day', timestamp) as day, + account_id, + provider, + SUM(total_savings) as daily_savings, + AVG(coverage_percentage) as avg_coverage, + COUNT(DISTINCT service) as service_count +FROM savings_snapshots +GROUP BY DATE_TRUNC('day', timestamp), account_id, provider; + +CREATE UNIQUE INDEX idx_daily_savings_trend_unique + ON daily_savings_trend(day, account_id, provider); + +CREATE MATERIALIZED VIEW provider_savings_summary AS +SELECT + provider, + account_id, + COUNT(DISTINCT service) as service_count, + SUM(total_savings) as total_savings, + SUM(total_commitment) as total_commitment, + AVG(coverage_percentage) as avg_coverage, + MAX(timestamp) as last_updated +FROM savings_snapshots +WHERE timestamp > NOW() - INTERVAL '90 days' +GROUP BY provider, account_id; + +CREATE UNIQUE INDEX idx_provider_savings_summary_unique + ON provider_savings_summary(provider, account_id); + +-- Restore NOT NULL DEFAULT 0 on the metric columns. NULLs are coerced to 0 +-- first so the NOT NULL re-add cannot fail. +UPDATE savings_snapshots SET total_usage = 0 WHERE total_usage IS NULL; +UPDATE savings_snapshots SET coverage_percentage = 0 WHERE coverage_percentage IS NULL; +ALTER TABLE savings_snapshots ALTER COLUMN total_usage SET DEFAULT 0.00; +ALTER TABLE savings_snapshots ALTER COLUMN total_usage SET NOT NULL; +ALTER TABLE savings_snapshots ALTER COLUMN coverage_percentage SET DEFAULT 0.00; +ALTER TABLE savings_snapshots ALTER COLUMN coverage_percentage SET NOT NULL; + +-- Refresh AFTER the data/schema are restored so the recreated views reflect the +-- coerced (non-NULL) values rather than a stale pre-restore state. +REFRESH MATERIALIZED VIEW monthly_savings_summary; +REFRESH MATERIALIZED VIEW daily_savings_trend; +REFRESH MATERIALIZED VIEW provider_savings_summary; + +-- Restore the original (WHEN OTHERS) drop_old_savings_partitions body. +CREATE OR REPLACE FUNCTION drop_old_savings_partitions(retention_months INTEGER DEFAULT 24) +RETURNS void AS $$ +DECLARE + partition_record RECORD; + partition_date DATE; + cutoff_date DATE; +BEGIN + cutoff_date := DATE_TRUNC('month', CURRENT_DATE) - (retention_months || ' months')::INTERVAL; + + FOR partition_record IN + SELECT tablename FROM pg_tables + WHERE schemaname = 'public' + AND tablename LIKE 'savings_snapshots_%' + AND tablename != 'savings_snapshots_default' + LOOP + BEGIN + partition_date := TO_DATE( + SUBSTRING(partition_record.tablename FROM '\d{4}_\d{2}'), + 'YYYY_MM' + ); + + IF partition_date < cutoff_date THEN + EXECUTE format('DROP TABLE IF EXISTS %I', partition_record.tablename); + RAISE NOTICE 'Dropped old partition: %', partition_record.tablename; + END IF; + EXCEPTION + WHEN OTHERS THEN + RAISE WARNING 'Could not process partition: %', partition_record.tablename; + END; + END LOOP; +END; +$$ LANGUAGE plpgsql; + +-- account_id stays VARCHAR(255): narrowing back to VARCHAR(20) could truncate +-- Azure/GCP ids written while this migration was applied, so the down keeps the +-- wider type (safe, additive). This is a deliberate non-symmetric down. diff --git a/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql b/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql new file mode 100644 index 000000000..ff4a894e0 --- /dev/null +++ b/internal/database/postgres/migrations/000067_analytics_snapshot_correctness.up.sql @@ -0,0 +1,220 @@ +-- ========================================== +-- ANALYTICS SNAPSHOT CORRECTNESS (wire-up prerequisites) +-- ========================================== +-- +-- Prepares savings_snapshots + its materialized views for the now-live +-- collector (issues #1023 / #1033). Three correctness fixes ship here; the +-- collector and Query* changes ship in the same PR: +-- +-- H4 account_id VARCHAR(20) is too small for Azure subscription IDs (36) and +-- GCP project IDs (<=30). Widen to VARCHAR(255) to match +-- cloud_accounts.external_id so non-AWS snapshot inserts stop erroring +-- with "value too long for type character varying(20)". +-- +-- H2 total_usage / coverage_percentage were NOT NULL DEFAULT 0. The collector +-- could only ever derive usage when the source recurring cost is present +-- and coverage when an on-demand baseline is present; coercing the absent +-- case to 0 drags AVG(coverage_percentage) toward zero (project rule +-- feedback_nullable_not_zero). Make both columns NULLABLE with no default +-- so "unknown" stays NULL and AVG/SUM skip it. +-- +-- H3 Scoping must key on cloud_account_id (the multi-tenant FK), not the +-- provider account string. The three materialized views grouped by +-- account_id only, so a cloud_account_id-scoped query could not read them. +-- Recreate the views carrying cloud_account_id alongside account_id. +-- +-- H5 total_savings / total_commitment / total_usage are point-in-time +-- run-rates (the collector now stores a monthly run-rate per snapshot), +-- not accrued totals, so SUM-ing them over a window double-counts by the +-- snapshot frequency: a $720/mo commitment summed over ~30 daily snapshots +-- read as ~$21,600, and changing the schedule changed the number. Aggregate +-- these columns with AVG so each period reports the representative monthly +-- run-rate, invariant to how often the collector runs. coverage stays AVG. +-- +-- Idempotent throughout: re-running on a partially-applied DB converges to the +-- target state rather than no-op'ing over a wrong column type (project rule +-- feedback_migration_full_restore). + +-- ------------------------------------------------------------------ +-- H4: widen account_id on the partitioned parent. +-- Postgres propagates the type change to all existing partitions. +-- ------------------------------------------------------------------ +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name = 'savings_snapshots' + AND column_name = 'account_id' + AND (character_maximum_length IS NULL OR character_maximum_length < 255) + AND data_type = 'character varying' + ) THEN + ALTER TABLE savings_snapshots + ALTER COLUMN account_id TYPE VARCHAR(255); + END IF; +END $$; + +-- ------------------------------------------------------------------ +-- H2: total_usage / coverage_percentage become NULLABLE, no DEFAULT. +-- Drop the NOT NULL and the DEFAULT 0 so absent metrics stay NULL. +-- Existing 0.00 rows are left as-is (those were emitted by the old +-- collector; they are indistinguishable from real zeros and the +-- materialized-view AVG already absorbed them. New rows write NULL). +-- ------------------------------------------------------------------ +ALTER TABLE savings_snapshots ALTER COLUMN total_usage DROP NOT NULL; +ALTER TABLE savings_snapshots ALTER COLUMN total_usage DROP DEFAULT; +ALTER TABLE savings_snapshots ALTER COLUMN coverage_percentage DROP NOT NULL; +ALTER TABLE savings_snapshots ALTER COLUMN coverage_percentage DROP DEFAULT; + +-- ------------------------------------------------------------------ +-- H3: recreate the materialized views carrying cloud_account_id so +-- cloud_account_id-scoped Query* can read the pre-aggregated rows. +-- DROP + CREATE because a materialized view's column list / GROUP BY +-- cannot be altered in place. Unique indexes are recreated for the +-- CONCURRENTLY refresh path. AVG(coverage_percentage) now skips NULLs +-- automatically (H2). +-- ------------------------------------------------------------------ +DROP MATERIALIZED VIEW IF EXISTS provider_savings_summary CASCADE; +DROP MATERIALIZED VIEW IF EXISTS daily_savings_trend CASCADE; +DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; + +CREATE MATERIALIZED VIEW monthly_savings_summary AS +SELECT + DATE_TRUNC('month', timestamp) as month, + account_id, + cloud_account_id, + provider, + service, + AVG(total_savings) as total_savings, + AVG(coverage_percentage) as avg_coverage, + AVG(total_commitment) as total_commitment, + AVG(total_usage) as total_usage, + COUNT(*) as snapshot_count, + MAX(timestamp) as last_updated +FROM savings_snapshots +GROUP BY DATE_TRUNC('month', timestamp), account_id, cloud_account_id, provider, service; + +-- cloud_account_id is nullable, so COALESCE it to the nil UUID inside the +-- unique index to keep the (month, account, provider, service) grain unique +-- under CONCURRENTLY refresh even when cloud_account_id IS NULL. +CREATE UNIQUE INDEX idx_monthly_savings_summary_unique + ON monthly_savings_summary( + month, account_id, + COALESCE(cloud_account_id, '00000000-0000-0000-0000-000000000000'::uuid), + provider, service); + +-- daily_savings_trend rolls up across services, so it keeps the legitimate SUM +-- across services but, per H5, replaces the SUM across time with an AVG: the +-- inner query sums the per-service run-rates into a provider total at each +-- collection timestamp, and the outer query averages those instant-totals over +-- the day so the trend is invariant to the collection frequency. +CREATE MATERIALIZED VIEW daily_savings_trend AS +SELECT + day, + account_id, + cloud_account_id, + provider, + AVG(ts_savings) as daily_savings, + AVG(ts_coverage) as avg_coverage, + MAX(ts_service_count) as service_count +FROM ( + SELECT + DATE_TRUNC('day', timestamp) as day, + timestamp, + account_id, + cloud_account_id, + provider, + SUM(total_savings) as ts_savings, + AVG(coverage_percentage) as ts_coverage, + COUNT(DISTINCT service) as ts_service_count + FROM savings_snapshots + GROUP BY DATE_TRUNC('day', timestamp), timestamp, account_id, cloud_account_id, provider +) per_ts +GROUP BY day, account_id, cloud_account_id, provider; + +CREATE UNIQUE INDEX idx_daily_savings_trend_unique + ON daily_savings_trend( + day, account_id, + COALESCE(cloud_account_id, '00000000-0000-0000-0000-000000000000'::uuid), + provider); + +-- provider_savings_summary also rolls up across services: keep the SUM across +-- services, AVG across time (H5). Inner query = per-timestamp provider totals, +-- outer query = average of those instant-totals over the 90-day window. +CREATE MATERIALIZED VIEW provider_savings_summary AS +SELECT + provider, + account_id, + cloud_account_id, + MAX(ts_service_count) as service_count, + AVG(ts_savings) as total_savings, + AVG(ts_commitment) as total_commitment, + AVG(ts_coverage) as avg_coverage, + MAX(timestamp) as last_updated +FROM ( + SELECT + provider, + account_id, + cloud_account_id, + timestamp, + SUM(total_savings) as ts_savings, + SUM(total_commitment) as ts_commitment, + AVG(coverage_percentage) as ts_coverage, + COUNT(DISTINCT service) as ts_service_count + FROM savings_snapshots + WHERE timestamp > NOW() - INTERVAL '90 days' + GROUP BY provider, account_id, cloud_account_id, timestamp +) per_ts +GROUP BY provider, account_id, cloud_account_id; + +CREATE UNIQUE INDEX idx_provider_savings_summary_unique + ON provider_savings_summary( + provider, account_id, + COALESCE(cloud_account_id, '00000000-0000-0000-0000-000000000000'::uuid)); + +-- Repopulate the freshly-created views non-concurrently (CONCURRENTLY cannot +-- run against a never-populated view). The runtime refresh function keeps +-- using CONCURRENTLY. +REFRESH MATERIALIZED VIEW monthly_savings_summary; +REFRESH MATERIALIZED VIEW daily_savings_trend; +REFRESH MATERIALIZED VIEW provider_savings_summary; + +-- ------------------------------------------------------------------ +-- M5: drop_old_savings_partitions swallowed every error via WHEN OTHERS, +-- downgrading lock-timeout / dependency / permission failures to a warning +-- so retention could silently fail. Narrow the handler to the expected +-- name-parse error and surface SQLERRM in the warning (N4). +-- ------------------------------------------------------------------ +CREATE OR REPLACE FUNCTION drop_old_savings_partitions(retention_months INTEGER DEFAULT 24) +RETURNS void AS $$ +DECLARE + partition_record RECORD; + partition_date DATE; + cutoff_date DATE; +BEGIN + cutoff_date := DATE_TRUNC('month', CURRENT_DATE) - (retention_months || ' months')::INTERVAL; + + FOR partition_record IN + SELECT tablename FROM pg_tables + WHERE schemaname = 'public' + AND tablename LIKE 'savings_snapshots_%' + AND tablename != 'savings_snapshots_default' + LOOP + BEGIN + partition_date := TO_DATE( + SUBSTRING(partition_record.tablename FROM '\d{4}_\d{2}'), + 'YYYY_MM' + ); + EXCEPTION + WHEN invalid_datetime_format OR datetime_field_overflow THEN + RAISE WARNING 'Skipping unparseable partition name %: %', + partition_record.tablename, SQLERRM; + CONTINUE; + END; + + IF partition_date < cutoff_date THEN + EXECUTE format('DROP TABLE IF EXISTS %I', partition_record.tablename); + RAISE NOTICE 'Dropped old partition: %', partition_record.tablename; + END IF; + END LOOP; +END; +$$ LANGUAGE plpgsql; diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index ec6b017ba..aedb23d6c 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -189,6 +189,15 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) } +// GetActivePurchaseHistory mocks the GetActivePurchaseHistory operation +func (m *MockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, asOf) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) +} + // GetPurchaseHistoryFiltered mocks the GetPurchaseHistoryFiltered operation (issue #701). func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { args := m.Called(ctx, filter) diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 86cd27dec..d15c6897f 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -352,6 +352,14 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) } +func (m *MockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, asOf) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) +} + func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { args := m.Called(ctx, filter) if args.Get(0) == nil { diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index dd3a2ac7e..1bfd3403d 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -177,6 +177,14 @@ func (m *MockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int) return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) } +func (m *MockConfigStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + args := m.Called(ctx, asOf) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]config.PurchaseHistoryRecord), args.Error(1) +} + func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { args := m.Called(ctx, filter) if args.Get(0) == nil { diff --git a/internal/server/analytics_collect.go b/internal/server/analytics_collect.go new file mode 100644 index 000000000..255268ee7 --- /dev/null +++ b/internal/server/analytics_collect.go @@ -0,0 +1,182 @@ +package server + +import ( + "context" + "fmt" + "log" + "os" + "strconv" + "time" + + "github.com/LeanerCloud/CUDly/internal/analytics" + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/database" +) + +// AnalyticsConfig holds the savings-snapshot collector knobs, read from env at +// startup and validated at the boundary (see Validate). +type AnalyticsConfig struct { + // Enabled gates the analytics_collect scheduled task. When false the task + // returns a "disabled" status without touching the DB. Default true. + Enabled bool + // RetentionMonths is how many months of snapshot partitions to keep before + // the retention job drops them. Default 24. Must be >= 1. + RetentionMonths int + // PartitionsAhead is how many future monthly partitions to keep provisioned + // ahead of the current month so inserts never fall into the catch-all + // default partition (M3). Default 3. Must be >= 1. + PartitionsAhead int +} + +const ( + defaultAnalyticsRetentionMonths = 24 + defaultAnalyticsPartitionsAhead = 3 + + // analyticsDDLTimeout bounds each long-running partition/retention/refresh + // DDL step. RDS Proxy does not honour a session statement_timeout, so a + // runaway DDL (e.g. a CONCURRENTLY refresh blocked on a lock) could hang the + // whole scheduled run indefinitely; a per-step deadline guarantees the + // pipeline makes forward progress or fails fast (06-N3). + analyticsDDLTimeout = 5 * time.Minute +) + +// LoadAnalyticsConfig reads the collector knobs from env, falling back to +// defaults for unset/blank values. Out-of-range or unparseable values are +// preserved as-is here so Validate can reject them with a clear message at +// startup (fail-fast at the boundary) rather than being silently clamped. +func LoadAnalyticsConfig() AnalyticsConfig { + return AnalyticsConfig{ + Enabled: getEnvBool("ANALYTICS_COLLECTION_ENABLED", true), + RetentionMonths: loadAnalyticsInt("ANALYTICS_RETENTION_MONTHS", defaultAnalyticsRetentionMonths), + PartitionsAhead: loadAnalyticsInt("ANALYTICS_PARTITIONS_AHEAD", defaultAnalyticsPartitionsAhead), + } +} + +// loadAnalyticsInt reads an integer collector knob from env. An unset/blank +// value falls back to defaultVal; a set-but-unparseable value is preserved as a +// 0 sentinel (rather than silently defaulting like getEnvInt) so Validate +// rejects the misconfiguration at startup instead of running with a default the +// operator never asked for (fail-fast at the boundary, feedback_strict_int_parse). +func loadAnalyticsInt(key string, defaultVal int) int { + raw := os.Getenv(key) + if raw == "" { + return defaultVal + } + val, err := strconv.Atoi(raw) + if err != nil { + return 0 // out-of-range sentinel: Validate() rejects it (must be >= 1) + } + return val +} + +// Validate rejects out-of-range analytics knobs so a misconfiguration fails +// fast at startup instead of silently producing a broken retention/partition +// policy at the first scheduled run. +func (c AnalyticsConfig) Validate() error { + if c.RetentionMonths < 1 { + return fmt.Errorf("ANALYTICS_RETENTION_MONTHS must be >= 1, got %d", c.RetentionMonths) + } + if c.PartitionsAhead < 1 { + return fmt.Errorf("ANALYTICS_PARTITIONS_AHEAD must be >= 1, got %d", c.PartitionsAhead) + } + return nil +} + +// withDDLTimeout runs an analytics DDL step that takes a single int arg under a +// bounded child context so it cannot hang the scheduled pipeline when no +// statement_timeout is enforced (e.g. under RDS Proxy, 06-N3). +func withDDLTimeout(ctx context.Context, fn func(context.Context, int) error, arg int) error { + stepCtx, cancel := context.WithTimeout(ctx, analyticsDDLTimeout) + defer cancel() + return fn(stepCtx, arg) +} + +// getEnvBool parses a boolean env var, returning defaultVal when unset or +// unparseable. Accepts the strconv.ParseBool truth set (1/t/true/...). +func getEnvBool(key string, defaultVal bool) bool { + if val := os.Getenv(key); val != "" { + if result, err := strconv.ParseBool(val); err == nil { + return result + } + } + return defaultVal +} + +// newAnalyticsCollector builds the savings-snapshot collector against the live +// DB connection. Kept here (not inline in reinitializeAfterConnect) so the +// app.go footprint is one localized call (eases the in-flight app.go rebase). +func newAnalyticsCollector(dbConn *database.Connection, configStore config.StoreInterface) (AnalyticsCollectorInterface, error) { + store := analytics.NewPostgresAnalyticsStore(dbConn) + return analytics.NewCollector(analytics.CollectorConfig{AnalyticsStore: store}, configStore) +} + +// handleCollectAnalytics runs the scheduled analytics pipeline end to end: +// ensure upcoming partitions exist, collect a snapshot across all tenants, +// apply retention, then refresh the materialized views over the fresh data. +// Each step is best-effort and recorded in the result so a single failure +// (e.g. a transient retention lock) doesn't abort the rest of the pipeline, +// but a hard failure still flips status to "partial" for observability. +func (app *Application) handleCollectAnalytics(ctx context.Context) (map[string]any, error) { + cfg := app.appConfig.Analytics + result := map[string]any{ + "status": "success", + "snapshots_written": false, + "partitions_ensured": false, + "partitions_dropped": false, + "views_refreshed": false, + } + + if !cfg.Enabled { + result["status"] = "disabled" + log.Println("Analytics collection disabled (ANALYTICS_COLLECTION_ENABLED=false)") + return result, nil + } + if app.AnalyticsCollector == nil || app.Analytics == nil { + result["status"] = "skipped" + log.Println("Analytics collector/store not available, skipping collection") + return result, nil + } + + // 1. Ensure upcoming partitions exist BEFORE writing so the snapshot lands + // in a real monthly partition rather than the catch-all default (M3). + if err := withDDLTimeout(ctx, app.Analytics.CreateFuturePartitions, cfg.PartitionsAhead); err != nil { + log.Printf("Warning: failed to ensure future partitions: %v", err) + result["status"] = "partial" + } else { + result["partitions_ensured"] = true + } + + // 2. Collect a snapshot across all tenants. + if err := app.AnalyticsCollector.Collect(ctx); err != nil { + // A cancelled context is terminal: stop the pipeline and surface it. + if ctx.Err() != nil { + return result, fmt.Errorf("analytics collection cancelled: %w", err) + } + log.Printf("Warning: analytics collection failed: %v", err) + result["status"] = "partial" + } else { + result["snapshots_written"] = true + } + + // 3. Apply retention. + if err := withDDLTimeout(ctx, app.Analytics.DropOldPartitions, cfg.RetentionMonths); err != nil { + log.Printf("Warning: failed to drop old partitions: %v", err) + result["status"] = "partial" + } else { + result["partitions_dropped"] = true + } + + // 4. Refresh materialized views over the fresh snapshot data. + refreshCtx, cancelRefresh := context.WithTimeout(ctx, analyticsDDLTimeout) + err := app.Analytics.RefreshMaterializedViews(refreshCtx) + cancelRefresh() + if err != nil { + log.Printf("Warning: failed to refresh materialized views: %v", err) + result["status"] = "partial" + } else { + result["views_refreshed"] = true + } + + log.Printf("Analytics collection complete: %v", result) + return result, nil +} diff --git a/internal/server/analytics_collect_test.go b/internal/server/analytics_collect_test.go new file mode 100644 index 000000000..8609bf642 --- /dev/null +++ b/internal/server/analytics_collect_test.go @@ -0,0 +1,72 @@ +package server + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/testutil" +) + +// TestLoadAnalyticsConfig_FailFastOnMalformedInt is the CR #1049 regression: +// a set-but-unparseable ANALYTICS_RETENTION_MONTHS / ANALYTICS_PARTITIONS_AHEAD +// must produce a config that Validate() rejects (fail-fast at startup) instead +// of silently falling back to the default. The pre-fix getEnvInt path returned +// the default for a bad value, so Validate() would have passed. +func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { + t.Run("unset uses defaults and validates", func(t *testing.T) { + t.Setenv("ANALYTICS_RETENTION_MONTHS", "") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "") + cfg := LoadAnalyticsConfig() + testutil.AssertEqual(t, defaultAnalyticsRetentionMonths, cfg.RetentionMonths) + testutil.AssertEqual(t, defaultAnalyticsPartitionsAhead, cfg.PartitionsAhead) + testutil.AssertNoError(t, cfg.Validate()) + }) + + t.Run("valid override is parsed", func(t *testing.T) { + t.Setenv("ANALYTICS_RETENTION_MONTHS", "12") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "6") + cfg := LoadAnalyticsConfig() + testutil.AssertEqual(t, 12, cfg.RetentionMonths) + testutil.AssertEqual(t, 6, cfg.PartitionsAhead) + testutil.AssertNoError(t, cfg.Validate()) + }) + + t.Run("malformed retention is rejected by Validate", func(t *testing.T) { + t.Setenv("ANALYTICS_RETENTION_MONTHS", "not-a-number") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "3") + cfg := LoadAnalyticsConfig() + testutil.AssertEqual(t, 0, cfg.RetentionMonths) // sentinel + testutil.AssertTrue(t, cfg.Validate() != nil, "malformed retention must fail Validate") + }) + + t.Run("malformed partitions-ahead is rejected by Validate", func(t *testing.T) { + t.Setenv("ANALYTICS_RETENTION_MONTHS", "24") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "12x") + cfg := LoadAnalyticsConfig() + testutil.AssertEqual(t, 0, cfg.PartitionsAhead) // sentinel + testutil.AssertTrue(t, cfg.Validate() != nil, "malformed partitions-ahead must fail Validate") + }) +} + +// TestHandleCollectAnalytics_DDLStepsAreBounded is the 06-N3 regression: each +// long-running partition/retention/refresh DDL step must run under a bounded +// context so a runaway statement cannot hang the scheduled run when no +// statement_timeout is enforced (RDS Proxy). The parent context here carries no +// deadline, so a deadline observed inside each step proves the per-step bound. +func TestHandleCollectAnalytics_DDLStepsAreBounded(t *testing.T) { + // Deliberately a deadline-free parent so an observed per-step deadline can + // only have come from the pipeline's own bounding (not inherited). + ctx := context.Background() + store := &mockAnalyticsStore{} + app := &Application{ + appConfig: ApplicationConfig{Analytics: AnalyticsConfig{Enabled: true, RetentionMonths: 24, PartitionsAhead: 3}}, + Analytics: store, + AnalyticsCollector: &mockAnalyticsCollector{}, + } + + _, err := app.handleCollectAnalytics(ctx) + testutil.AssertNoError(t, err) + testutil.AssertTrue(t, store.createFuturePartHadDeadline, "CreateFuturePartitions must run under a bounded context") + testutil.AssertTrue(t, store.dropOldPartHadDeadline, "DropOldPartitions must run under a bounded context") + testutil.AssertTrue(t, store.refreshHadDeadline, "RefreshMaterializedViews must run under a bounded context") +} diff --git a/internal/server/app.go b/internal/server/app.go index 7546f9f10..3126e1986 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -46,9 +46,13 @@ type Application struct { Auth *auth.Service RateLimiter api.RateLimiterInterface // Distributed rate limiter (DB-backed for multi-instance) Analytics AnalyticsStoreInterface // Analytics store for savings data - Version string - DB *database.Connection // PostgreSQL database connection - TaskLocker TaskLocker // Advisory lock for scheduled tasks (defaults to DB) + // AnalyticsCollector aggregates savings into snapshots on a schedule. + // Nil until reinitializeAfterConnect wires it; the collect task no-ops + // when nil so test builds without a DB stay quiet. + AnalyticsCollector AnalyticsCollectorInterface + Version string + DB *database.Connection // PostgreSQL database connection + TaskLocker TaskLocker // Advisory lock for scheduled tasks (defaults to DB) // Static file serving directory (from STATIC_DIR env var) staticDir string @@ -121,6 +125,10 @@ type ApplicationConfig struct { ScheduledTaskSecret string ScheduledTaskSecretName string IsLambda bool + + // Analytics snapshot collector knobs. See analytics_collect.go for + // defaults and boundary validation (LoadAnalyticsConfig). + Analytics AnalyticsConfig } // ExternalDeps holds pre-built external dependencies that require infrastructure @@ -261,6 +269,7 @@ func LoadApplicationConfig() ApplicationConfig { ScheduledTaskSecret: os.Getenv("SCHEDULED_TASK_SECRET"), ScheduledTaskSecretName: os.Getenv("SCHEDULED_TASK_SECRET_NAME"), IsLambda: isLambdaRuntime(), + Analytics: LoadAnalyticsConfig(), } } @@ -464,6 +473,10 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext func NewApplication(ctx context.Context) (*Application, error) { cfg := LoadApplicationConfig() + if err := cfg.Analytics.Validate(); err != nil { + return nil, fmt.Errorf("invalid analytics configuration: %w", err) + } + log.Printf("CUDly Server initializing, version: %s", cfg.Version) // Initialize configuration store (PostgreSQL) @@ -639,9 +652,15 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da log.Println("Initialized database-backed rate limiter for Lambda (distributed state)") } - // Initialize analytics store for savings data and materialized views + // Initialize analytics store for savings data and materialized views, plus + // the snapshot collector behind the scheduled analytics_collect task. app.Analytics = analytics.NewPostgresAnalyticsStore(dbConn) - log.Println("Initialized PostgreSQL analytics store") + collector, err := newAnalyticsCollector(dbConn, app.Config) + if err != nil { + return fmt.Errorf("failed to create analytics collector: %w", err) + } + app.AnalyticsCollector = collector + log.Println("Initialized PostgreSQL analytics store and snapshot collector") // Initialize credential store (AES-256-GCM encrypted credential blobs). encKey, encKeySource, err := loadAndGuardEncryptionKey(ctx, app.secretResolver) @@ -739,6 +758,8 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da EmailNotifier: app.Email, DashboardURL: app.appConfig.DashboardURL, AnalyticsClient: api.NewPostgresAnalyticsClient(dbConn), + AnalyticsCollector: app.AnalyticsCollector, + AnalyticsSnapshots: analytics.NewPostgresAnalyticsStore(dbConn), OIDCSigner: app.signer, OIDCIssuerURL: resolveOIDCIssuerURL(app.appConfig), CommitmentOpts: commitmentOpts, diff --git a/internal/server/handler.go b/internal/server/handler.go index 8b0b7773c..555974913 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -26,7 +26,14 @@ const ( TaskSendNotifications ScheduledTaskType = "send_notifications" TaskCleanupExpiredRecords ScheduledTaskType = "cleanup" TaskRefreshAnalytics ScheduledTaskType = "analytics_refresh" - TaskRIExchangeReshape ScheduledTaskType = "ri_exchange_reshape" + // TaskCollectAnalytics runs the savings-snapshot collector end to end: + // ensure upcoming partitions, collect a snapshot across all tenants, apply + // retention, and refresh the materialized views. Scheduled separately from + // TaskRefreshAnalytics (the legacy refresh-only task) so the snapshot + // ingestion cadence can differ from a pure view refresh. See issues + // #1023 / #1033. + TaskCollectAnalytics ScheduledTaskType = "analytics_collect" + TaskRIExchangeReshape ScheduledTaskType = "ri_exchange_reshape" // TaskReapStuckPurchases sweeps purchase_executions stuck in // approved/running longer than PURCHASE_APPROVED_REAP_AFTER and flips // them to "failed" via the existing TransitionExecutionStatus CAS. @@ -75,6 +82,8 @@ func (app *Application) dispatchTask(ctx context.Context, taskType ScheduledTask return app.handleCleanupExpiredRecords(ctx) case TaskRefreshAnalytics: return app.handleRefreshAnalytics(ctx) + case TaskCollectAnalytics: + return app.handleCollectAnalytics(ctx) case TaskRIExchangeReshape: return app.handleRIExchangeReshape(ctx) case TaskReapStuckPurchases: @@ -261,6 +270,8 @@ func ParseScheduledEvent(rawEvent json.RawMessage) (ScheduledTaskType, error) { return TaskCleanupExpiredRecords, nil case "analytics_refresh": return TaskRefreshAnalytics, nil + case "analytics_collect": + return TaskCollectAnalytics, nil case "ri_exchange_reshape": return TaskRIExchangeReshape, nil case "reap_stuck_purchases": diff --git a/internal/server/handler_coverage_test.go b/internal/server/handler_coverage_test.go index dcf01b3a8..1c1587154 100644 --- a/internal/server/handler_coverage_test.go +++ b/internal/server/handler_coverage_test.go @@ -48,13 +48,35 @@ func TestHandleCleanupExpiredRecords_NilAuthAndConfig(t *testing.T) { // ----- handleRefreshAnalytics ----- type mockAnalyticsStore struct { - refreshErr error + refreshErr error + createFuturePartErr error + dropOldPartErr error + createFuturePartMonths int + dropOldPartRetention int + // *HadDeadline capture whether each DDL step ran under a bounded context, so + // the 06-N3 fix (per-step timeouts under RDS Proxy) is positively asserted. + createFuturePartHadDeadline bool + dropOldPartHadDeadline bool + refreshHadDeadline bool } func (m *mockAnalyticsStore) RefreshMaterializedViews(ctx context.Context) error { + _, m.refreshHadDeadline = ctx.Deadline() return m.refreshErr } +func (m *mockAnalyticsStore) CreateFuturePartitions(ctx context.Context, monthsAhead int) error { + _, m.createFuturePartHadDeadline = ctx.Deadline() + m.createFuturePartMonths = monthsAhead + return m.createFuturePartErr +} + +func (m *mockAnalyticsStore) DropOldPartitions(ctx context.Context, retentionMonths int) error { + _, m.dropOldPartHadDeadline = ctx.Deadline() + m.dropOldPartRetention = retentionMonths + return m.dropOldPartErr +} + func TestHandleRefreshAnalytics_Success(t *testing.T) { ctx := testutil.TestContext(t) app := &Application{ @@ -361,3 +383,73 @@ func (m *mockConfigStoreForExchangeStale) SavePurchaseExecutionTx(ctx context.Co func (m *mockConfigStoreForExchangeStale) WithTx(_ context.Context, fn func(tx pgx.Tx) error) error { return fn(nil) } + +// ----- handleCollectAnalytics ----- + +type mockAnalyticsCollector struct { + collectErr error + calls int +} + +func (m *mockAnalyticsCollector) Collect(ctx context.Context) error { + m.calls++ + return m.collectErr +} + +func TestHandleCollectAnalytics_Disabled(t *testing.T) { + ctx := testutil.TestContext(t) + app := &Application{ + appConfig: ApplicationConfig{Analytics: AnalyticsConfig{Enabled: false, RetentionMonths: 24, PartitionsAhead: 3}}, + Analytics: &mockAnalyticsStore{}, + AnalyticsCollector: &mockAnalyticsCollector{}, + } + result, err := app.handleCollectAnalytics(ctx) + testutil.AssertNoError(t, err) + testutil.AssertEqual(t, "disabled", result["status"]) +} + +func TestHandleCollectAnalytics_SuccessRunsFullPipeline(t *testing.T) { + ctx := testutil.TestContext(t) + store := &mockAnalyticsStore{} + collector := &mockAnalyticsCollector{} + app := &Application{ + appConfig: ApplicationConfig{Analytics: AnalyticsConfig{Enabled: true, RetentionMonths: 18, PartitionsAhead: 4}}, + Analytics: store, + AnalyticsCollector: collector, + } + result, err := app.handleCollectAnalytics(ctx) + testutil.AssertNoError(t, err) + testutil.AssertEqual(t, "success", result["status"]) + testutil.AssertEqual(t, 1, collector.calls) + testutil.AssertEqual(t, 4, store.createFuturePartMonths) + testutil.AssertEqual(t, 18, store.dropOldPartRetention) +} + +func TestHandleCollectAnalytics_CollectErrorIsPartial(t *testing.T) { + ctx := testutil.TestContext(t) + app := &Application{ + appConfig: ApplicationConfig{Analytics: AnalyticsConfig{Enabled: true, RetentionMonths: 24, PartitionsAhead: 3}}, + Analytics: &mockAnalyticsStore{}, + AnalyticsCollector: &mockAnalyticsCollector{collectErr: errors.New("collect failed")}, + } + result, err := app.handleCollectAnalytics(ctx) + testutil.AssertNoError(t, err) // best-effort: logged, not propagated + testutil.AssertEqual(t, "partial", result["status"]) +} + +func TestHandleCollectAnalytics_NilCollectorSkips(t *testing.T) { + ctx := testutil.TestContext(t) + app := &Application{ + appConfig: ApplicationConfig{Analytics: AnalyticsConfig{Enabled: true, RetentionMonths: 24, PartitionsAhead: 3}}, + Analytics: &mockAnalyticsStore{}, + } + result, err := app.handleCollectAnalytics(ctx) + testutil.AssertNoError(t, err) + testutil.AssertEqual(t, "skipped", result["status"]) +} + +func TestAnalyticsConfig_Validate(t *testing.T) { + testutil.AssertNoError(t, AnalyticsConfig{RetentionMonths: 1, PartitionsAhead: 1}.Validate()) + testutil.AssertTrue(t, AnalyticsConfig{RetentionMonths: 0, PartitionsAhead: 1}.Validate() != nil, "retention < 1 must error") + testutil.AssertTrue(t, AnalyticsConfig{RetentionMonths: 1, PartitionsAhead: 0}.Validate() != nil, "partitions < 1 must error") +} diff --git a/internal/server/interfaces.go b/internal/server/interfaces.go index a374c210f..30cc6c4f4 100644 --- a/internal/server/interfaces.go +++ b/internal/server/interfaces.go @@ -35,7 +35,21 @@ type PurchaseManagerInterface interface { ReapStuckExecutions(ctx context.Context, reapAfter time.Duration) (*purchase.ReapResult, error) } -// AnalyticsStoreInterface defines the methods required for analytics storage +// AnalyticsStoreInterface defines the methods required for analytics storage. +// Beyond the materialized-view refresh, the scheduled analytics task also keeps +// monthly partitions provisioned ahead of time and applies retention. type AnalyticsStoreInterface interface { RefreshMaterializedViews(ctx context.Context) error + // CreateFuturePartitions ensures partitions exist for the current month + // plus monthsAhead months ahead (M3: partitions otherwise stop after the + // seeded months and every insert falls into the catch-all default). + CreateFuturePartitions(ctx context.Context, monthsAhead int) error + // DropOldPartitions drops partitions older than retentionMonths (retention). + DropOldPartitions(ctx context.Context, retentionMonths int) error +} + +// AnalyticsCollectorInterface aggregates current savings into a point-in-time +// snapshot row per (tenant, provider, service, region, commitment_type) bucket. +type AnalyticsCollectorInterface interface { + Collect(ctx context.Context) error } diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index a33012bdc..2725d9f73 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -99,6 +99,10 @@ func (m *mockConfigStoreForHealth) GetAllPurchaseHistory(ctx context.Context, li return nil, nil } +func (m *mockConfigStoreForHealth) GetActivePurchaseHistory(ctx context.Context, asOf time.Time) ([]config.PurchaseHistoryRecord, error) { + return nil, nil +} + func (m *mockConfigStoreForHealth) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) { return nil, nil } diff --git a/pkg/exchange/reshape_crossfamily_test.go b/pkg/exchange/reshape_crossfamily_test.go index 7d4eb7f60..30f6d372d 100644 --- a/pkg/exchange/reshape_crossfamily_test.go +++ b/pkg/exchange/reshape_crossfamily_test.go @@ -465,7 +465,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // m6i.xlarge: same "m" prefix (gen jump bonus), exact NF, high confidence. nearPerfect := OfferingOption{ - InstanceType: "m6i.xlarge", + InstanceType: "m6i.xlarge", EffectiveMonthlyCost: 85, // slightly more expensive NormalizationFactor: 8, SavingsAbs: floatPtr(220), @@ -473,7 +473,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // r5.xlarge: different prefix (no family gen bonus), same NF, no confidence. crossFamily := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 80, // same cost as source -- better raw price NormalizationFactor: 8, } @@ -497,13 +497,13 @@ func TestCompositeScore_SameArchOutranksCrossArch(t *testing.T) { } // c6i: same "c" prefix (family gen bonus), Intel x86 (same arch). sameArch := OfferingOption{ - InstanceType: "c6i.xlarge", + InstanceType: "c6i.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } // c6g: same "c" prefix (family gen bonus), Graviton ARM (cross arch). crossArch := OfferingOption{ - InstanceType: "c6g.xlarge", + InstanceType: "c6g.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } @@ -522,14 +522,14 @@ func TestCompositeScore_HighConfidenceOutranksLow(t *testing.T) { MonthlyCost: 70, } highConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(250), RecommendationCount: 4, } lowConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(10), @@ -550,13 +550,13 @@ func TestCompositeScore_AbsentSavingsIsNeutral(t *testing.T) { MonthlyCost: 70, } absentSavings := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: nil, // not supplied } lowConf := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(5), // explicitly low confidence diff --git a/terraform/modules/compute/aws/lambda/main.tf b/terraform/modules/compute/aws/lambda/main.tf index 836d126fd..418fdceca 100644 --- a/terraform/modules/compute/aws/lambda/main.tf +++ b/terraform/modules/compute/aws/lambda/main.tf @@ -600,3 +600,44 @@ resource "aws_lambda_permission" "eventbridge_reap_stuck_purchases" { principal = "events.amazonaws.com" source_arn = aws_cloudwatch_event_rule.reap_stuck_purchases[0].arn } + +# ============================================== +# EventBridge Rule for Savings-Snapshot Collection (#1023 / #1033) +# ============================================== +# +# Periodic run of the analytics_collect task: ensure upcoming monthly +# partitions exist, write a point-in-time savings snapshot across all +# tenants, apply retention, then refresh the materialized views. The +# handler is advisory-lock guarded so overlapping invocations are safe. + +resource "aws_cloudwatch_event_rule" "analytics_collect" { + count = var.enable_analytics_collect_schedule ? 1 : 0 + + name = "${var.stack_name}-analytics-collect" + description = "Trigger savings-snapshot analytics collection (issues #1023/#1033)" + schedule_expression = var.analytics_collect_schedule + + tags = var.tags +} + +resource "aws_cloudwatch_event_target" "analytics_collect" { + count = var.enable_analytics_collect_schedule ? 1 : 0 + + rule = aws_cloudwatch_event_rule.analytics_collect[0].name + target_id = "lambda" + arn = aws_lambda_function.main.arn + + input = jsonencode({ + action = "analytics_collect" + }) +} + +resource "aws_lambda_permission" "eventbridge_analytics_collect" { + count = var.enable_analytics_collect_schedule ? 1 : 0 + + statement_id = "AllowExecutionFromEventBridgeAnalyticsCollect" + action = "lambda:InvokeFunction" + function_name = aws_lambda_function.main.function_name + principal = "events.amazonaws.com" + source_arn = aws_cloudwatch_event_rule.analytics_collect[0].arn +} diff --git a/terraform/modules/compute/aws/lambda/variables.tf b/terraform/modules/compute/aws/lambda/variables.tf index 6c062a44b..e55c8508e 100644 --- a/terraform/modules/compute/aws/lambda/variables.tf +++ b/terraform/modules/compute/aws/lambda/variables.tf @@ -177,6 +177,18 @@ variable "reap_stuck_purchases_schedule" { default = "rate(5 minutes)" } +variable "enable_analytics_collect_schedule" { + description = "Enable the scheduled savings-snapshot analytics collection (issues #1023/#1033). When true, EventBridge periodically invokes the analytics_collect task (snapshot + partition maintenance + retention + view refresh)." + type = bool + default = true +} + +variable "analytics_collect_schedule" { + description = "EventBridge schedule for the analytics_collect task. The collector aggregates point-in-time savings snapshots, so a daily cadence captures a clean coverage/utilization time-series without excess write volume. rate() starts from deployment time; use cron() for fixed clock times." + type = string + default = "rate(1 day)" +} + variable "purchase_approved_reap_after" { description = "Threshold age for the stuck-purchase reaper. Any execution sitting in approved/running longer than this gets flipped to failed on the next sweep. Parsed via Go time.ParseDuration (e.g. \"10m\", \"15m\", \"1h\"). Empty string falls back to the in-code default (10m)." type = string