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
3 changes: 3 additions & 0 deletions internal/analytics/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,9 @@ func (m *mockConfigStore) GetRecommendationsFreshness(_ context.Context) (*confi
func (m *mockConfigStore) SetRecommendationsCollectionError(_ context.Context, _ string) error {
return nil
}
func (m *mockConfigStore) SetRecommendationsCollectionPartial(_ context.Context, _ *config.RecommendationsCollectionPartial) error {
return nil
}
func (m *mockConfigStore) MarkCollectionStarted(_ context.Context) (string, bool, error) {
return "mock-owner-token", true, nil
}
Expand Down
58 changes: 58 additions & 0 deletions internal/api/handler_recommendations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,54 @@ func TestHandler_getRecommendationsFreshness(t *testing.T) {
assert.WithinDuration(t, now, *got.LastCollectedAt, time.Second)
})

// Issue #87: the freshness payload is what the dashboard reads, so a
// partial sweep must reach it as a structured, sanitized field.
t.Run("surfaces last_collection_partial", func(t *testing.T) {
mockStore := new(MockConfigStore)
now := time.Now().UTC()
partial := &config.RecommendationsCollectionPartial{
Providers: []config.PartialProviderSweep{{
Provider: "azure",
IncompleteAccounts: 1,
AttemptedScopes: 10,
FailedScopes: 9,
Reasons: map[string]int{config.PartialReasonThrottled: 9},
}},
}
mockStore.On("GetRecommendationsFreshness", ctx).
Return(&config.RecommendationsFreshness{LastCollectedAt: &now, LastCollectionPartial: partial}, nil)

handler := &Handler{config: mockStore, apiKey: "test-key"}
req := &events.LambdaFunctionURLRequest{
Headers: map[string]string{"x-api-key": "test-key"},
}

got, err := handler.getRecommendationsFreshness(ctx, req)
require.NoError(t, err)
body, err := json.Marshal(got)
require.NoError(t, err)
assert.JSONEq(t, `{"providers":[{"provider":"azure","incomplete_accounts":1,"attempted_scopes":10,`+
`"failed_scopes":9,"failed_details":0,"reasons":{"throttled":9}}]}`,
string(mustField(t, body, "last_collection_partial")))
})

t.Run("complete collection serializes last_collection_partial as null", func(t *testing.T) {
mockStore := new(MockConfigStore)
mockStore.On("GetRecommendationsFreshness", ctx).Return(&config.RecommendationsFreshness{}, nil)

handler := &Handler{config: mockStore, apiKey: "test-key"}
req := &events.LambdaFunctionURLRequest{
Headers: map[string]string{"x-api-key": "test-key"},
}

got, err := handler.getRecommendationsFreshness(ctx, req)
require.NoError(t, err)
body, err := json.Marshal(got)
require.NoError(t, err)
assert.Equal(t, "null", string(mustField(t, body, "last_collection_partial")),
"the key must always be present so the frontend can rely on it")
})

t.Run("surfaces store error", func(t *testing.T) {
mockStore := new(MockConfigStore)
mockStore.On("GetRecommendationsFreshness", ctx).Return(nil, errors.New("db down"))
Expand Down Expand Up @@ -766,3 +814,13 @@ func TestConfidenceBucketFor(t *testing.T) {
})
}
}

// mustField returns the raw JSON of a top-level key, failing if it is absent.
func mustField(t *testing.T, body []byte, key string) json.RawMessage {
t.Helper()
var fields map[string]json.RawMessage
require.NoError(t, json.Unmarshal(body, &fields))
raw, ok := fields[key]
require.True(t, ok, "response must carry %q", key)
return raw
}
92 changes: 92 additions & 0 deletions internal/api/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,29 @@ paths:
'403':
$ref: '#/components/responses/Forbidden'

/api/recommendations/freshness:
get:
operationId: getRecommendationsFreshness
tags: [Recommendations]
summary: Get the recommendations cache freshness and last collection outcome
description: >
Returns when recommendations were last collected, the most recent
collection error, whether a collection is in flight, and whether the
last collection was partial (some subscriptions, projects or scopes
could not be queried, so the stored recommendations under-state the
real opportunity).
responses:
'200':
description: Freshness state
content:
application/json:
schema:
$ref: '#/components/schemas/RecommendationsFreshness'
'401':
$ref: '#/components/responses/Unauthorized'
'403':
$ref: '#/components/responses/Forbidden'

# ---- Purchase Plans -----------------------------------------------------
/api/plans:
get:
Expand Down Expand Up @@ -3196,6 +3219,75 @@ components:
items:
type: string

RecommendationsFreshness:
type: object
required: [last_collected_at, last_collection_error, last_collection_started_at, last_collection_partial]
properties:
last_collected_at:
type: string
format: date-time
nullable: true
description: When recommendations were last persisted; null on a cold start.
last_collection_error:
type: string
nullable: true
description: Error from the most recent collection when a provider failed; null otherwise.
last_collection_started_at:
type: string
format: date-time
nullable: true
description: Set while a collection is in flight.
last_collection_partial:
nullable: true
description: >
Non-null when the most recent persisted collection kept rows from
at least one incomplete sweep. Cleared by the next collection whose
sweeps are all complete. Carries counts and reason categories only.
allOf:
- $ref: '#/components/schemas/RecommendationsCollectionPartial'

RecommendationsCollectionPartial:
type: object
required: [providers]
properties:
providers:
type: array
items:
$ref: '#/components/schemas/PartialProviderSweep'

PartialProviderSweep:
type: object
required: [provider, incomplete_accounts, attempted_scopes, failed_scopes, failed_details, reasons]
properties:
provider:
type: string
enum: [aws, azure, gcp]
incomplete_accounts:
type: integer
description: Accounts (or the ambient host identity) whose sweep was incomplete.
attempted_scopes:
type: integer
description: >
Scopes (Azure subscriptions, GCP projects) the incomplete sweeps
were asked to cover; 0 when the provider does not report it (AWS).
failed_scopes:
type: integer
description: Scopes that could not be queried.
failed_details:
type: integer
description: Individual recommendation details rejected inside answering scopes (AWS).
reasons:
type: object
description: >
Failure count per reason category. Keys are drawn from a closed
set: permission_denied, throttled, timeout, canceled, not_found,
provider_unavailable, other.
additionalProperties:
type: integer
example:
permission_denied: 2
throttled: 1

# -- Plans --------------------------------------------------------------
PlansResponse:
type: object
Expand Down
4 changes: 4 additions & 0 deletions internal/config/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,10 @@ type StoreInterface interface {
ListStoredRecommendations(ctx context.Context, filter RecommendationFilter) ([]RecommendationRecord, error)
GetRecommendationsFreshness(ctx context.Context) (*RecommendationsFreshness, error)
SetRecommendationsCollectionError(ctx context.Context, errMsg string) error
// SetRecommendationsCollectionPartial records (or, with nil, clears) the
// most recent collection's incomplete-sweep summary. Upsert/Replace
// clear it, so a later complete collection drops the marker.
SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error
// MarkCollectionStarted atomically sets last_collection_started_at = now
// only when no in-flight collection is running (last_collection_started_at IS NULL
// OR older than 5 minutes), and stamps a freshly generated owner token
Expand Down
63 changes: 56 additions & 7 deletions internal/config/store_postgres_recommendations.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ func (s *PostgresStore) ReplaceRecommendations(ctx context.Context, collectedAt

if _, err := tx.Exec(ctx, `
UPDATE recommendations_state
SET last_collected_at = $1,
last_collection_error = NULL
SET last_collected_at = $1,
last_collection_error = NULL,
last_collection_partial = NULL
WHERE id = 1
`, collectedAt); err != nil {
return fmt.Errorf("failed to update state: %w", err)
Expand Down Expand Up @@ -109,8 +110,9 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t

if _, err := tx.Exec(ctx, `
UPDATE recommendations_state
SET last_collected_at = $1,
last_collection_error = NULL
SET last_collected_at = $1,
last_collection_error = NULL,
last_collection_partial = NULL
WHERE id = 1
`, collectedAt); err != nil {
return fmt.Errorf("failed to update state: %w", err)
Expand Down Expand Up @@ -430,28 +432,49 @@ func (s *PostgresStore) ListStoredRecommendations(ctx context.Context, filter Re

// GetRecommendationsFreshness returns the singleton freshness row. The
// table is seeded with id=1 by the migration so a row always exists;
// LastCollectedAt, LastCollectionError, and LastCollectionStartedAt can be NULL.
// LastCollectedAt, LastCollectionError, LastCollectionStartedAt and
// LastCollectionPartial can be NULL.
func (s *PostgresStore) GetRecommendationsFreshness(ctx context.Context) (*RecommendationsFreshness, error) {
var (
lastCollectedAt *time.Time
lastCollectionError *string
lastCollectionStartedAt *time.Time
lastCollectionPartial []byte
)
err := s.db.QueryRow(ctx, `
SELECT last_collected_at, last_collection_error, last_collection_started_at
SELECT last_collected_at, last_collection_error, last_collection_started_at, last_collection_partial
FROM recommendations_state
WHERE id = 1
`).Scan(&lastCollectedAt, &lastCollectionError, &lastCollectionStartedAt)
`).Scan(&lastCollectedAt, &lastCollectionError, &lastCollectionStartedAt, &lastCollectionPartial)
if err != nil {
return nil, fmt.Errorf("failed to read recommendations_state: %w", err)
}
return &RecommendationsFreshness{
LastCollectedAt: lastCollectedAt,
LastCollectionError: lastCollectionError,
LastCollectionStartedAt: lastCollectionStartedAt,
LastCollectionPartial: decodeCollectionPartial(lastCollectionPartial),
}, nil
}

// decodeCollectionPartial decodes the last_collection_partial JSONB column.
// NULL decodes to nil (the last collection was complete). The column is only
// ever written by SetRecommendationsCollectionPartial via json.Marshal, so an
// undecodable value means the row was edited by hand; it is logged and read
// as nil rather than failing the whole freshness read, which the scheduler's
// cache-TTL check and every dashboard load depend on.
func decodeCollectionPartial(raw []byte) *RecommendationsCollectionPartial {
if len(raw) == 0 {
return nil
}
var partial RecommendationsCollectionPartial
if err := json.Unmarshal(raw, &partial); err != nil {
logging.Warnf("recommendations_state.last_collection_partial is not valid JSON; ignoring it: %v", err)
return nil
}
return &partial
}

// SetRecommendationsCollectionError records the most recent collection's
// error message without touching last_collected_at or
// last_collection_started_at. Used by the scheduler when a collect fails
Expand All @@ -478,6 +501,32 @@ func (s *PostgresStore) SetRecommendationsCollectionError(ctx context.Context, e
return nil
}

// SetRecommendationsCollectionPartial records the most recent collection's
// incomplete-sweep summary (issue #87), or clears it when partial is nil.
// Like SetRecommendationsCollectionError it is called by the scheduler AFTER
// UpsertRecommendations has reset the column to NULL, so a complete
// collection leaves it cleared and only a partial one re-sets it. It touches
// no other column; in particular it must not clear last_collection_started_at
// (see SetRecommendationsCollectionError, issue #261).
func (s *PostgresStore) SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error {
var raw []byte
if partial != nil {
encoded, err := json.Marshal(partial)
if err != nil {
return fmt.Errorf("failed to encode collection partial: %w", err)
}
raw = encoded
}
if _, err := s.db.Exec(ctx, `
UPDATE recommendations_state
SET last_collection_partial = $1::jsonb
WHERE id = 1
`, raw); err != nil {
return fmt.Errorf("failed to set collection partial: %w", err)
}
return nil
}

// MarkCollectionStarted atomically sets last_collection_started_at = NOW()
// only when no in-flight collection is currently running. The WHERE clause
// treats a started_at older than 5 minutes as stale (the scheduler Lambda
Expand Down
22 changes: 22 additions & 0 deletions internal/config/store_postgres_recommendations_partial_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package config

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestDecodeCollectionPartial(t *testing.T) {
assert.Nil(t, decodeCollectionPartial(nil), "NULL means the last collection was complete")

got := decodeCollectionPartial([]byte(`{"providers":[{"provider":"azure","incomplete_accounts":1,"attempted_scopes":3,"failed_scopes":2,"failed_details":0,"reasons":{"throttled":2}}]}`))
require.NotNil(t, got)
assert.Equal(t, []PartialProviderSweep{{
Provider: "azure", IncompleteAccounts: 1, AttemptedScopes: 3, FailedScopes: 2,
Reasons: map[string]int{PartialReasonThrottled: 2},
}}, got.Providers)

assert.Nil(t, decodeCollectionPartial([]byte(`not json`)),
"an undecodable diagnostic must not fail the freshness read")
}
57 changes: 57 additions & 0 deletions internal/config/store_postgres_recommendations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,63 @@ func TestPostgresStore_Freshness_RoundTrip(t *testing.T) {
assert.Equal(t, "aws failed", *fr.LastCollectionError)
}

// TestPostgresStore_CollectionPartial_RecordedAndCleared pins issue #87: a
// partial sweep's summary round-trips through recommendations_state, survives
// an error write (both can be true of one run), and is cleared by the next
// Upsert or Replace -- i.e. by the next persisted collection, which re-sets
// it only if that collection was partial too.
func TestPostgresStore_CollectionPartial_RecordedAndCleared(t *testing.T) {
ctx := context.Background()
store, cleanup := setupRecommendationsStore(ctx, t)
defer cleanup()

fr, err := store.GetRecommendationsFreshness(ctx)
require.NoError(t, err)
assert.Nil(t, fr.LastCollectionPartial, "a fresh deployment reads as complete")

partial := &config.RecommendationsCollectionPartial{
Providers: []config.PartialProviderSweep{{
Provider: "azure",
IncompleteAccounts: 1,
AttemptedScopes: 10,
FailedScopes: 9,
Reasons: map[string]int{config.PartialReasonPermissionDenied: 9},
}},
}
recordPartial := func() {
t.Helper()
require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, partial))
got, getErr := store.GetRecommendationsFreshness(ctx)
require.NoError(t, getErr)
require.Equal(t, partial, got.LastCollectionPartial)
}

recordPartial()
require.NoError(t, store.SetRecommendationsCollectionError(ctx, "gcp: failed"))
fr, err = store.GetRecommendationsFreshness(ctx)
require.NoError(t, err)
assert.Equal(t, partial, fr.LastCollectionPartial, "recording an error must not drop the partial marker")

now := time.Now().UTC().Truncate(time.Second)
require.NoError(t, store.UpsertRecommendations(ctx, now, nil, nil))
fr, err = store.GetRecommendationsFreshness(ctx)
require.NoError(t, err)
assert.Nil(t, fr.LastCollectionPartial, "the next persisted collection must clear the marker")
assert.Nil(t, fr.LastCollectionError)

recordPartial()
require.NoError(t, store.ReplaceRecommendations(ctx, now.Add(time.Minute), nil))
fr, err = store.GetRecommendationsFreshness(ctx)
require.NoError(t, err)
assert.Nil(t, fr.LastCollectionPartial, "a full resync must clear the marker too")

recordPartial()
require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, nil))
fr, err = store.GetRecommendationsFreshness(ctx)
require.NoError(t, err)
assert.Nil(t, fr.LastCollectionPartial, "a nil partial clears the column")
}

// TestPostgresStore_UpsertRecommendations_StoresAllTermVariants pins
// the broadened-natural-key behavior from migration 000032: when
// Azure returns multiple `(term, payment)` variants for the same
Expand Down
Loading
Loading