diff --git a/internal/analytics/collector.go b/internal/analytics/collector.go index 859ec423e..bea1eb39c 100644 --- a/internal/analytics/collector.go +++ b/internal/analytics/collector.go @@ -60,14 +60,15 @@ type aggregateData struct { commitment float64 commitmentUnknown bool // 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 + // bucket as the covered-usage proxy. usageUnknown is set as soon as one + // contributing row has a nil MonthlyCost, exactly like commitmentUnknown, + // so a bucket with any all-upfront commitment writes NULL usage rather + // than a partial sum marked as known (issue #1423). A known 0 stays 0 // (feedback_nullable_not_zero). - usage float64 - usageKnown bool - savings float64 - count int + usage float64 + usageUnknown bool + savings float64 + count int } // aggKey is the bucket identity. cloudAccountID is dereferenced (or "" when @@ -185,12 +186,13 @@ func aggregatePurchases(ctx context.Context, purchases []config.PurchaseHistoryR } else { 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 { + // H2: real covered usage from the recurring monthly cost. One nil + // MonthlyCost (e.g. AWS all-upfront) makes the whole bucket's usage + // unknown, rather than implicitly $0 for that row. + if p.MonthlyCost == nil { + agg.usageUnknown = true + } else { agg.usage += *p.MonthlyCost - agg.usageKnown = true } agg.count++ } @@ -208,7 +210,7 @@ func commitmentTypeFor(service string) string { } // buildSnapshots converts the aggregation map into snapshot rows. total_usage is -// nil when no contributing row carried a recurring cost; coverage_percentage is +// nil when any contributing row lacks 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 { @@ -219,7 +221,7 @@ func buildSnapshots(serviceMap map[string]*aggregateData, now time.Time) []Savin commitment = &agg.commitment } var usage *float64 - if agg.usageKnown { + if !agg.usageUnknown { u := agg.usage usage = &u } diff --git a/internal/analytics/nullable_purchase_cost_test.go b/internal/analytics/nullable_purchase_cost_test.go index 2f3b2318e..7fb9346e7 100644 --- a/internal/analytics/nullable_purchase_cost_test.go +++ b/internal/analytics/nullable_purchase_cost_test.go @@ -48,3 +48,52 @@ func TestCollectorUnknownUpfront(t *testing.T) { } } } + +// TestCollectorUnknownMonthlyCost covers issue #1423: one nil MonthlyCost in a +// snapshot row makes total_usage unknown instead of a partial sum stored as +// known, while a row whose costs are all known (including a known 0) keeps its +// real total. +func TestCollectorUnknownMonthlyCost(t *testing.T) { + cases := []struct { + name string + costs []*float64 + want *float64 + }{ + {"recurring then all-upfront", []*float64{new(float64(120)), nil}, nil}, + {"all-upfront then recurring", []*float64{nil, new(float64(120))}, nil}, + {"all unknown", []*float64{nil, nil}, nil}, + {"all known", []*float64{new(float64(120)), new(float64(30))}, new(float64(150))}, + {"known zero stays zero", []*float64{new(float64)}, new(float64)}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + purchases := make([]config.PurchaseHistoryRecord, len(tc.costs), len(tc.costs)+1) + for i, cost := range tc.costs { + purchases[i] = activeRecord("aws", "ec2", "us-east-1", 1, 10, 0) + purchases[i].MonthlyCost = cost + } + other := activeRecord("aws", "ec2", "us-east-1", 1, 20, 240) + other.AccountID = "other-account" + other.MonthlyCost = new(float64(45)) + purchases = append(purchases, other) + groups, _, _, err := aggregatePurchases(context.Background(), purchases, time.Now()) + require.NoError(t, err) + snapshots := buildSnapshots(groups, time.Now()) + require.Len(t, snapshots, 2) + for _, snapshot := range snapshots { + if snapshot.AccountID == other.AccountID { + require.NotNil(t, snapshot.TotalUsage, "an unknown cost must not leak into another row") + assert.InDelta(t, 45.0, *snapshot.TotalUsage, 0.001) + continue + } + assert.Equal(t, len(tc.costs), snapshot.Metadata["active_purchases"]) + if tc.want == nil { + assert.Nil(t, snapshot.TotalUsage) + continue + } + require.NotNil(t, snapshot.TotalUsage) + assert.InDelta(t, *tc.want, *snapshot.TotalUsage, 0.001) + } + }) + } +}