Skip to content

fix(api/dashboard): tighten committed_monthly aggregation (status + unit + account scope) #927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Jun 3, 2026
Merged

fix(api/dashboard): tighten committed_monthly aggregation (status + unit + account scope) #927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
120 changes: 85 additions & 35 deletions internal/api/handler_dashboard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun
return nil, err
}

effectiveAccountID, err := resolveDashboardAccountID(params)
legacyAccountID, cloudAccountUUIDs, err := resolveDashboardAccountScope(params)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -51,7 +51,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun

totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey)
targetCoverage := h.resolveTargetCoverage(ctx)
activeCommitments, committedMonthly, ytdSavings := h.calculateCommitmentMetrics(ctx, effectiveAccountID)
activeCommitments, committedMonthly, ytdSavings := h.calculateCommitmentMetrics(ctx, legacyAccountID, cloudAccountUUIDs)

return &DashboardSummaryResponse{
PotentialMonthlySavings: totalSavings,
Expand All @@ -65,26 +65,36 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun
}, nil
}

// resolveDashboardAccountID chooses the single account_id passed into the
// (legacy, single-account) commitment metrics calculation:
// - one ID in `account_ids` → use it
// - multiple IDs → empty (no filter; logged as observability)
// - none → fall back to the legacy singular `account_id` param
func resolveDashboardAccountID(params map[string]string) (string, error) {
accountIDs, err := parseAccountIDs(params["account_ids"])
if err != nil {
return "", NewClientError(400, err.Error())
// resolveDashboardAccountScope parses the account filter params and returns
// the two disjoint representations used by commitment-metrics fetching:
//
// - legacyAccountID: the singular `account_id` param (cloud-provider account
// number, e.g. "123456789012"), passed as-is to GetPurchaseHistory when
// no UUID-based filter is present. Empty string means "no legacy filter".
//
// - cloudAccountUUIDs: UUIDs from the `account_ids` param, matched against
// purchase_history.cloud_account_id via GetPurchaseHistoryFiltered. Non-nil
// (even if empty) when `account_ids` was present and valid.
//
// Exactly one of legacyAccountID or cloudAccountUUIDs is meaningful per call:
// when account_ids is supplied, legacyAccountID is set to "" so the caller
// uses the UUID path; when only account_id is supplied, cloudAccountUUIDs is
// nil so the caller uses the legacy path; when neither is supplied, both are
// zero-valued and the caller fetches all history.
func resolveDashboardAccountScope(params map[string]string) (legacyAccountID string, cloudAccountUUIDs []string, err error) {
uuids, parseErr := parseAccountIDs(params["account_ids"])
if parseErr != nil {
return "", nil, NewClientError(400, parseErr.Error())
}

switch len(accountIDs) {
case 1:
return accountIDs[0], nil
case 0:
return params["account_id"], nil
default:
logging.Infof("dashboard: multi-account filter requested (%d accounts); returning unfiltered metrics until per-account breakdown is implemented", len(accountIDs))
return "", nil
if len(uuids) > 0 {
// UUID-based multi-account filter — takes precedence over legacy param.
return "", uuids, nil
}

// No UUID filter: fall back to the legacy singular cloud-provider account
// number. Empty string means "fetch all accounts" (no filter).
return params["account_id"], nil, nil
}

// filterDashboardRecommendations applies the session's allowed_accounts filter
Expand Down Expand Up @@ -396,45 +406,85 @@ func commitmentExpiry(p config.PurchaseHistoryRecord) time.Time {
return p.Timestamp.Add(termDuration)
}

// isActiveCommitment reports whether the purchase's term has not yet
// expired as of `now`. The boundary is strict (After): a commitment is
// active right up to the instant its term ends. Same predicate shared
// by the dashboard aggregate and the per-commitment inventory endpoint.
// isActiveCommitment reports whether the purchase is active: its term has not
// yet expired as of `now` AND its status is one of the successful terminal
// states ("" for DB-backed rows where the column is unpersisted, or
// "completed"). Rows synthesised from failed/cancelled/expired executions
// carry a non-empty status other than "completed" and are excluded so they
// do not inflate the committed_monthly KPI. The boundary is strict (After):
// a commitment is active right up to the instant its term ends.
//
// Same predicate shared by the dashboard aggregate and the per-commitment
// inventory endpoint. Status values: see PurchaseHistoryRecord.Status doc.
func isActiveCommitment(p config.PurchaseHistoryRecord, now time.Time) bool {
// Status is unpersisted (dynamodbav:"-"); DB rows always read back as "".
// Synthesised rows set it to "failed", "expired", "cancelled", "pending",
// "notified", "approved", "running", or "paused". Only "" and "completed"
// represent a commitment that is actually live on the provider.
if p.Status != "" && p.Status != "completed" {
return false
}
return !now.After(commitmentExpiry(p))
}

// calculateCommitmentMetrics calculates active commitments and savings from purchase history
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, accountID string) (activeCommitments int, committedMonthly, ytdSavings float64) {
// Get purchase history (last 1000 purchases should be sufficient)
purchases, err := h.config.GetPurchaseHistory(ctx, accountID, 1000)
// calculateCommitmentMetrics aggregates active-commitment counts and monthly
// savings from purchase history. The fetch strategy depends on the filter:
//
// - cloudAccountUUIDs non-empty: GetPurchaseHistoryFiltered scoped to those
// cloud_account_id UUIDs (multi-account UUID filter from `account_ids`).
// - legacyAccountID non-empty: GetPurchaseHistory filtered by account_id
// (cloud-provider account number, e.g. "123456789012"; legacy single-account
// filter from the `account_id` param).
// - both empty: GetAllPurchaseHistory (no account filter).
//
// EstimatedSavings on purchase_history rows is always written in monthly units
// (populated from PurchaseExecution.EstimatedSavings which derives from
// recommendation monthly savings at purchase time — see SavePurchaseHistory
// and the purchase manager). No unit normalisation is needed.
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountID string, cloudAccountUUIDs []string) (activeCommitments int, committedMonthly, ytdSavings float64) {
const fetchLimit = 1000

var (
purchases []config.PurchaseHistoryRecord
err error
)

switch {
case len(cloudAccountUUIDs) > 0:
// Multi-account UUID filter: match purchase_history.cloud_account_id.
purchases, err = h.config.GetPurchaseHistoryFiltered(ctx, "", cloudAccountUUIDs, nil, nil, fetchLimit)
case legacyAccountID != "":
// Legacy single-account filter: match purchase_history.account_id.
purchases, err = h.config.GetPurchaseHistory(ctx, legacyAccountID, fetchLimit)
default:
// No account filter: fetch across all accounts.
purchases, err = h.config.GetAllPurchaseHistory(ctx, fetchLimit)
}

if err != nil {
// Log error but don't fail the request
// Log error but don't fail the dashboard request.
return 0, 0, 0
}

// Get current time from context or use now
currentTime := time.Now()
yearStart := time.Date(currentTime.Year(), 1, 1, 0, 0, 0, 0, time.UTC)

for _, p := range purchases {
if !isActiveCommitment(p, currentTime) {
continue // Skip expired commitments
continue
}

// Count active commitments
activeCommitments++

// Add to committed monthly (EstimatedSavings is typically monthly)
// EstimatedSavings is in monthly units (see doc comment above).
committedMonthly += p.EstimatedSavings

// Calculate YTD savings (savings accumulated since year start)
// YTD savings: count from year start or from purchase date, whichever
// is later, using a 30-day month approximation (same as original).
if p.Timestamp.Before(yearStart) {
// Purchase made before this year, count full year so far
monthsSinceYearStart := int(currentTime.Sub(yearStart).Hours() / (24 * 30))
ytdSavings += p.EstimatedSavings * float64(monthsSinceYearStart)
} else {
// Purchase made this year, count from purchase date
monthsSincePurchase := int(currentTime.Sub(p.Timestamp).Hours() / (24 * 30))
ytdSavings += p.EstimatedSavings * float64(monthsSincePurchase)
}
Expand Down
88 changes: 78 additions & 10 deletions internal/api/handler_dashboard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ func TestHandler_getDashboardSummary(t *testing.T) {

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil)
mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil)
mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
// No account_id / account_ids filter → calculateCommitmentMetrics uses GetAllPurchaseHistory.
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)

mockAuth, req := adminDashboardReq(ctx)
handler := &Handler{
Expand Down Expand Up @@ -117,7 +118,7 @@ func TestHandler_getDashboardSummary_PerAccountCoverageScalesSavings(t *testing.

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil)
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(&config.ServiceConfig{
Provider: "aws", Service: "rds", Enabled: true, Coverage: 100,
}, nil)
Expand Down Expand Up @@ -160,7 +161,7 @@ func TestHandler_getDashboardSummary_ZeroCoverageInServiceConfigFallsThroughToFu

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil)
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
// Global ServiceConfig has Coverage=0 (zero-value — operator never set it).
mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(&config.ServiceConfig{
Provider: "aws", Service: "rds", Enabled: true, Coverage: 0,
Expand Down Expand Up @@ -688,7 +689,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123")
activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
Expand All @@ -701,7 +702,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123")
activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
Expand All @@ -725,7 +726,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123")
activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 100.0, committedMonthly)
Expand All @@ -750,7 +751,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123")
activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

// Should skip expired commitments
assert.Equal(t, 0, activeCommitments)
Expand All @@ -775,11 +776,78 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123")
activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 50.0, committedMonthly)
})

// --- Bug fix tests ---

// Status filter: a row with Status="failed" must NOT be counted as an active
// commitment. The invariant: isActiveCommitment rejects any non-empty status
// that isn't "completed".
t.Run("failed status row is excluded from committedMonthly", func(t *testing.T) {
mockStore := new(MockConfigStore)
purchaseTime := time.Now().AddDate(0, -3, 0)
purchases := []config.PurchaseHistoryRecord{
{
Timestamp: purchaseTime,
Term: 1,
EstimatedSavings: 200.0,
Status: "failed",
},
{
Timestamp: purchaseTime,
Term: 1,
EstimatedSavings: 50.0,
// Status "" = completed DB row — must be counted
},
}
mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return(purchases, nil)

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

// Only the status="" row counts; the failed row must be excluded.
assert.Equal(t, 1, activeCommitments,
"failed commitment must not increment activeCommitments")
assert.Equal(t, 50.0, committedMonthly,
"failed commitment's savings must not appear in committedMonthly")
})

// Multi-account scope: a UUID-filtered request must use GetPurchaseHistoryFiltered
// scoped to the supplied cloud_account_id UUIDs. Rows from account C must NOT
// appear when the filter contains only A and B.
t.Run("multi-account UUID filter routes to GetPurchaseHistoryFiltered", func(t *testing.T) {
mockStore := new(MockConfigStore)
purchaseTime := time.Now().AddDate(0, -2, 0)

accountAID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"
accountBID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"

// Only rows for A and B are returned by the scoped query — the store
// enforces the filter; the handler must NOT call GetPurchaseHistory.
purchasesAB := []config.PurchaseHistoryRecord{
{CloudAccountID: &accountAID, Timestamp: purchaseTime, Term: 1, EstimatedSavings: 100.0},
{CloudAccountID: &accountBID, Timestamp: purchaseTime, Term: 1, EstimatedSavings: 150.0},
}
uuids := []string{accountAID, accountBID}
mockStore.On("GetPurchaseHistoryFiltered", ctx, "", uuids, (*time.Time)(nil), (*time.Time)(nil), 1000).
Return(purchasesAB, nil)
// GetPurchaseHistory must not be called — no On registration so it
// would panic if accidentally invoked.
t.Cleanup(func() { mockStore.AssertExpectations(t) })

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "", uuids)

assert.Equal(t, 2, activeCommitments)
assert.Equal(t, 250.0, committedMonthly,
"only accounts A and B must contribute; account C rows must not appear")
})
}

func TestHandler_calculateCurrentCoverage(t *testing.T) {
Expand Down Expand Up @@ -827,7 +895,7 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) {

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil)
mockStore.On("GetGlobalConfig", ctx).Return(nil, nil)
mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)

mockAuth, req := adminDashboardReq(ctx)
handler := &Handler{
Expand All @@ -851,7 +919,7 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) {

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil)
mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil)
mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)

mockAuth, req := adminDashboardReq(ctx)
handler := &Handler{
Expand Down
4 changes: 2 additions & 2 deletions internal/api/handler_per_account_perms_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,8 @@ func TestPerAccountPerms_DashboardSummary_AggregatesAllowedSubsetOnly(t *testing
// Guard against future code paths that resolve service configs before filtering:
// stub rds so an unexpected GetServiceConfig call doesn't panic the test.
mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return((*config.ServiceConfig)(nil), nil)
// calculateCommitmentMetrics calls GetPurchaseHistory for YTD/committed totals.
mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
// calculateCommitmentMetrics calls GetAllPurchaseHistory when no account filter is set.
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) {
return permsAccountList(), nil
}
Expand Down
3 changes: 2 additions & 1 deletion internal/api/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,8 @@ func TestHandler_HandleRequest_GetDashboardSummary(t *testing.T) {

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil)
mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil)
mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)
// No account_id / account_ids filter → calculateCommitmentMetrics uses GetAllPurchaseHistory.
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil)

handler := &Handler{
scheduler: mockScheduler,
Expand Down
Loading