Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions internal/analytics/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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++
}
Expand All @@ -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 {
Expand All @@ -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
}
Expand Down
49 changes: 49 additions & 0 deletions internal/analytics/nullable_purchase_cost_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
}
Loading