From 83f14ed73a0d6199092f221ef1246748d66010ba Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:49:30 +0200 Subject: [PATCH 1/2] fix(azure): surface skipped consumption recommendations as a typed partial-result error A non-USD billing currency or an unpriceable SKU was dropped with only a log line, so an EUR or GBP account looked like it had no savings. The skips are now recorded on a context-carried SkipLog and returned from GetRecommendations as *PartialResultError alongside the recommendations that did succeed. Closes #362 Co-Authored-By: Claude Opus 5.5 --- .../internal/recommendations/consumption.go | 65 ++++++++++++++++--- .../recommendations/consumption_test.go | 7 +- providers/azure/recommendations.go | 18 ++++- providers/azure/recommendations_skips_test.go | 61 +++++++++++++++++ providers/azure/services/cache/client.go | 2 +- providers/azure/services/compute/client.go | 2 +- providers/azure/services/cosmosdb/client.go | 2 +- providers/azure/services/database/client.go | 2 +- .../azure/services/managedredis/client.go | 4 +- providers/azure/services/search/client.go | 4 +- providers/azure/services/synapse/client.go | 6 +- 11 files changed, 150 insertions(+), 23 deletions(-) create mode 100644 providers/azure/recommendations_skips_test.go diff --git a/providers/azure/internal/recommendations/consumption.go b/providers/azure/internal/recommendations/consumption.go index bd20cacf..db25b833 100644 --- a/providers/azure/internal/recommendations/consumption.go +++ b/providers/azure/internal/recommendations/consumption.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strings" + "sync" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" @@ -284,30 +285,76 @@ func ExpandConsumptionVariants(ctx context.Context, base common.Recommendation, return []common.Recommendation{upfront, monthly}, nil } +// SkipLog collects the recommendations a collection dropped. It is attached +// to the collection context by WithSkipLog and is safe for concurrent use. +type SkipLog struct { + mu sync.Mutex + entries []string +} + +type skipLogKey struct{} + +// WithSkipLog returns a context carrying a fresh SkipLog and that log. +func WithSkipLog(ctx context.Context) (context.Context, *SkipLog) { + l := &SkipLog{} + return context.WithValue(ctx, skipLogKey{}, l), l +} + +func recordSkip(ctx context.Context, service string, err error) { + logging.Errorf("azure %s: skipping recommendation: %v", service, err) + if l, ok := ctx.Value(skipLogKey{}).(*SkipLog); ok { + l.mu.Lock() + l.entries = append(l.entries, fmt.Sprintf("%s: %v", service, err)) + l.mu.Unlock() + } +} + +// Entries returns a copy of the recorded skip reasons. +func (l *SkipLog) Entries() []string { + l.mu.Lock() + defer l.mu.Unlock() + return append([]string(nil), l.entries...) +} + +// PartialResultError reports that a collection dropped recommendations it +// could not scale or price (non-USD billing currency, unpriceable SKU, ...). +// The recommendations that did succeed are returned alongside it; callers that +// want them use errors.As and keep the slice, callers that treat any error as +// a failed run see the problem instead of an empty-looking success. +type PartialResultError struct { + Skipped []string +} + +func (e *PartialResultError) Error() string { + return fmt.Sprintf("azure recommendations: %d recommendation(s) skipped: %s", len(e.Skipped), strings.Join(e.Skipped, "; ")) +} + // ExtractConsumptionOrSkip wraps ExtractConsumption for the service -// converters: a recommendation that cannot be scaled is logged as an error -// and reported as nil, so one bad recommendation does not fail the collection. -func ExtractConsumptionOrSkip(service string, rec armconsumption.ReservationRecommendationClassification) *ExtractedFields { +// converters: a recommendation that cannot be scaled is logged, recorded in +// the context's SkipLog (see WithSkipLog) and reported as nil, so one bad +// recommendation does not abort the collection but is still surfaced. +func ExtractConsumptionOrSkip(ctx context.Context, service string, rec armconsumption.ReservationRecommendationClassification) *ExtractedFields { f, err := ExtractConsumption(rec) if err != nil { - logging.Errorf("azure %s: skipping recommendation: %v", service, err) + recordSkip(ctx, service, err) return nil } return f } // AppendConsumptionVariants appends the variants of base to recs. A -// recommendation the catalog has no usable price for is logged as an error and -// skipped, leaving recs unchanged. A price lookup that fails to complete -// returns the error (see IsCollectionFailure): the caller must fail the -// collection rather than return a silently partial result. +// recommendation the catalog has no usable price for is logged, recorded in +// the context's SkipLog and skipped, leaving recs unchanged. A price lookup +// that fails to complete returns the error (see IsCollectionFailure): the +// caller must fail the collection rather than return a silently partial +// result. func AppendConsumptionVariants(ctx context.Context, service string, recs []common.Recommendation, base common.Recommendation, p *Pricer) ([]common.Recommendation, error) { variants, err := ExpandConsumptionVariants(ctx, base, p) if err != nil { if IsCollectionFailure(err) { return recs, fmt.Errorf("azure %s: %w", service, err) } - logging.Errorf("azure %s: skipping recommendation: %v", service, err) + recordSkip(ctx, service, err) return recs, nil } return append(recs, variants...), nil diff --git a/providers/azure/internal/recommendations/consumption_test.go b/providers/azure/internal/recommendations/consumption_test.go index ef02654f..7c357ae6 100644 --- a/providers/azure/internal/recommendations/consumption_test.go +++ b/providers/azure/internal/recommendations/consumption_test.go @@ -231,12 +231,15 @@ func TestAppendConsumptionVariants_SkipsOnlyTheUnpriceable(t *testing.T) { bad.ResourceType = "Standard_Bad" p := NewPricer("compute", stub.lookup) + ctx, skips := WithSkipLog(context.Background()) var recs []common.Recommendation - recs, err := AppendConsumptionVariants(context.Background(), "compute", recs, bad, p) + recs, err := AppendConsumptionVariants(ctx, "compute", recs, bad, p) require.NoError(t, err, "an unresolvable price skips only that recommendation") - recs, err = AppendConsumptionVariants(context.Background(), "compute", recs, good, p) + recs, err = AppendConsumptionVariants(ctx, "compute", recs, good, p) require.NoError(t, err) require.Len(t, recs, 2) + require.Len(t, skips.Entries(), 1, "the skipped SKU is recorded, not only logged") + assert.Contains(t, skips.Entries()[0], "Standard_Bad") assert.Equal(t, "Standard_D2as_v4", recs[0].ResourceType) } diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 788bd331..083339e7 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -119,6 +119,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p ) g, gctx := errgroup.WithContext(ctx) + gctx, skips := azrecs.WithSkipLog(gctx) // Each per-service goroutine is a leaf — it issues the actual ARM / // pricing-API call. The shared semaphore on gctx (set by the scheduler) @@ -223,7 +224,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p return nil, err } - return mergeServiceResults(serviceResult{"compute", computeRecs, computeErr, includeCompute}, + merged, err := mergeServiceResults(serviceResult{"compute", computeRecs, computeErr, includeCompute}, serviceResult{"database", dbRecs, dbErr, includeDB}, serviceResult{"cache", cacheRecs, cacheErr, includeCache}, serviceResult{"cosmosdb", cosmosRecs, cosmosErr, includeCosmos}, @@ -245,6 +246,21 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // prevents. When getAdvisorRecommendations is changed to propagate // hard errors, flip this flag back to true. serviceResult{"advisor", advisorRecs, advisorErr, false}) + return withSkips(merged, err, skips) +} + +// withSkips attaches the collection's skipped recommendations (non-USD +// currency, unpriceable SKU) to a merge result as a *azrecs.PartialResultError +// returned alongside the recommendations that did succeed, so an EUR/GBP +// account does not look like "no savings". +func withSkips(recs []common.Recommendation, err error, skips *azrecs.SkipLog) ([]common.Recommendation, error) { + if err != nil { + return nil, err + } + if skipped := skips.Entries(); len(skipped) > 0 { + return recs, &azrecs.PartialResultError{Skipped: skipped} + } + return recs, nil } // serviceResult bundles a per-service collection outcome for the deterministic diff --git a/providers/azure/recommendations_skips_test.go b/providers/azure/recommendations_skips_test.go new file mode 100644 index 00000000..6e1763a0 --- /dev/null +++ b/providers/azure/recommendations_skips_test.go @@ -0,0 +1,61 @@ +package azure + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" +) + +// consumptionFake runs the shared skip-aware extraction over fixed payloads, +// the way the real service converters do. +type consumptionFake struct { + recs []armconsumption.ReservationRecommendationClassification +} + +func (f *consumptionFake) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { + var out []common.Recommendation + for _, r := range f.recs { + if fields := azrecs.ExtractConsumptionOrSkip(ctx, "compute", r); fields != nil { + out = append(out, common.Recommendation{Service: common.ServiceCompute, ResourceType: fields.ResourceType}) + } + } + return out, nil +} + +// Fixture-based: modern payloads built from armconsumption types. +func TestGetRecommendations_NonUSDSkipIsSurfaced(t *testing.T) { + eur := mocks.BuildModernReservationRecommendation( + mocks.WithModernCosts(191, 116, 75), mocks.WithModernCurrency("EUR")) + usd := mocks.BuildModernReservationRecommendation( + mocks.WithModernCosts(191, 116, 75), mocks.WithModernSKUName("Standard_D2as_v4")) + + run := func(recs ...armconsumption.ReservationRecommendationClassification) ([]common.Recommendation, error) { + overrideServiceClientFns(t, &consumptionFake{recs: recs}, + &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}) + a := &RecommendationsClientAdapter{cred: &mockAzureTokenCredential{}, subscriptionID: "sub", getAdvisorRecsFn: noopAdvisorFn} + return a.GetRecommendations(context.Background(), &common.RecommendationParams{Service: common.ServiceCompute}) + } + + recs, err := run(eur) + assert.Empty(t, recs) + var partial *azrecs.PartialResultError + require.True(t, errors.As(err, &partial), "an all-EUR account must not look like zero savings: %v", err) + assert.Contains(t, partial.Skipped[0], "EUR") + + recs, err = run(eur, usd) + assert.Len(t, recs, 1, "the priceable recommendation is still returned") + require.True(t, errors.As(err, &partial)) + assert.Len(t, partial.Skipped, 1) + + recs, err = run(usd) + require.NoError(t, err) + assert.Len(t, recs, 1) +} diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 3f2acdb3..eaa11643 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -599,7 +599,7 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe // Premium-tier SKU; otherwise stays 0 (zero means "unknown", not // "definitely zero shards" — see the redisSKUEntry godoc). func (c *Client) convertAzureRedisRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { - f := azrecs.ExtractConsumptionOrSkip("cache", azureRec) + f := azrecs.ExtractConsumptionOrSkip(ctx, "cache", azureRec) if f == nil { return nil } diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index b2bf543a..f0434075 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -756,7 +756,7 @@ func (c *Client) fetchAzurePricing(ctx context.Context, filter string) (*AzureRe // sources (consumption usage records, dedicated-host inventory) and // remain unpopulated — out of scope for this issue. func (c *Client) convertAzureVMRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { - f := azrecs.ExtractConsumptionOrSkip("compute", azureRec) + f := azrecs.ExtractConsumptionOrSkip(ctx, "compute", azureRec) if f == nil { return nil } diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 608f3e0a..3c7befc3 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -617,7 +617,7 @@ func (c *Client) fetchAzurePricing(ctx context.Context, filter string) (*AzureRe // subscription has zero Cosmos accounts, multiple Cosmos accounts with // different API types (ambiguous), or the listing fails. func (c *Client) convertAzureCosmosRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { - f := azrecs.ExtractConsumptionOrSkip("cosmosdb", azureRec) + f := azrecs.ExtractConsumptionOrSkip(ctx, "cosmosdb", azureRec) if f == nil { return nil } diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index fdaae6dc..c41c9f00 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -632,7 +632,7 @@ func (c *Client) fetchAzurePricing(ctx context.Context, filter string) (*AzureRe // lazily-cached subscription-wide server/managed-instance lists; // both stay empty when the fetch fails or the subscription is ambiguous. func (c *Client) convertAzureSQLRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { - f := azrecs.ExtractConsumptionOrSkip("database", azureRec) + f := azrecs.ExtractConsumptionOrSkip(ctx, "database", azureRec) if f == nil { return nil } diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index d394dedf..982697d6 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -544,8 +544,8 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe // convertRecommendation converts an Azure Consumption API recommendation to the common format. // Returns nil when the input is nil or cannot be parsed (e.g. an unsupported SDK Kind). -func (c *Client) convertRecommendation(_ context.Context, rec armconsumption.ReservationRecommendationClassification) *common.Recommendation { - f := recommendations.ExtractConsumptionOrSkip("managedredis", rec) +func (c *Client) convertRecommendation(ctx context.Context, rec armconsumption.ReservationRecommendationClassification) *common.Recommendation { + f := recommendations.ExtractConsumptionOrSkip(ctx, "managedredis", rec) if f == nil { return nil } diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 2e3b2629..297bf643 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -495,9 +495,9 @@ func (c *Client) fetchAzurePricing(ctx context.Context, filter string) (*AzureRe } // convertAzureSearchRecommendation converts Azure Search reservation recommendation to common format. -func (c *Client) convertAzureSearchRecommendation(_ context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { +func (c *Client) convertAzureSearchRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { // Extract fields from Azure recommendation using the shared converter - extracted := azrecs.ExtractConsumptionOrSkip("search", azureRec) + extracted := azrecs.ExtractConsumptionOrSkip(ctx, "search", azureRec) if extracted == nil { return nil } diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index 49e481f8..f8ab06e9 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -177,7 +177,7 @@ func (c *Client) GetRecommendations(ctx context.Context, _ *common.Recommendatio // belong to the client's region (all of them when no region is set). func (c *Client) appendRegionRecommendations(ctx context.Context, pricer *recommendations.Pricer, recs []common.Recommendation, page []armconsumption.ReservationRecommendationClassification) ([]common.Recommendation, error) { for _, rec := range page { - converted := c.convertSynapseRecommendation(rec) + converted := c.convertSynapseRecommendation(ctx, rec) if converted == nil { continue } @@ -488,8 +488,8 @@ func (c *Client) getSynapsePricing(ctx context.Context, sku, region string, term // convertSynapseRecommendation converts an Azure reservation recommendation // to the common Recommendation format. -func (c *Client) convertSynapseRecommendation(azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { - f := recommendations.ExtractConsumptionOrSkip("synapse", azureRec) +func (c *Client) convertSynapseRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { + f := recommendations.ExtractConsumptionOrSkip(ctx, "synapse", azureRec) if f == nil { return nil } From 2de6006558f307aa739e96094d5f8637112a71d6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:35:00 +0200 Subject: [PATCH 2/2] fix(azure): keep a subscription's recommendations when its error is a partial result PartialResultError was only reachable from the internal package and the multi-subscription merge counted any error as a failed subscription, dropping its recommendations. Export the type and AsPartialResult, give it a Partial() method, and aggregate skips across subscriptions. Co-Authored-By: Claude Opus 5.5 --- .../internal/recommendations/consumption.go | 4 +++ .../recommendations_multi_subscription.go | 29 +++++++++++++++++++ ...recommendations_multi_subscription_test.go | 20 +++++++++++++ 3 files changed, 53 insertions(+) diff --git a/providers/azure/internal/recommendations/consumption.go b/providers/azure/internal/recommendations/consumption.go index db25b833..36da7a5b 100644 --- a/providers/azure/internal/recommendations/consumption.go +++ b/providers/azure/internal/recommendations/consumption.go @@ -325,6 +325,10 @@ type PartialResultError struct { Skipped []string } +// Partial marks the error as one that accompanies usable recommendations; the +// cross-provider partial-result check matches on this method. +func (e *PartialResultError) Partial() bool { return true } + func (e *PartialResultError) Error() string { return fmt.Sprintf("azure recommendations: %d recommendation(s) skipped: %s", len(e.Skipped), strings.Join(e.Skipped, "; ")) } diff --git a/providers/azure/recommendations_multi_subscription.go b/providers/azure/recommendations_multi_subscription.go index 68147573..4672de0e 100644 --- a/providers/azure/recommendations_multi_subscription.go +++ b/providers/azure/recommendations_multi_subscription.go @@ -14,6 +14,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" ) // newSubscriptionRecommendationsClientFn builds the per-subscription @@ -51,6 +52,20 @@ type SubscriptionFailure struct { // that a retry might clear. var ErrSubscriptionNotAccessible = errors.New("subscription is not accessible to the authenticated principal") +// PartialResultError reports that a collection dropped recommendations it +// could not scale or price (non-USD currency, unpriceable SKU, truncated +// Advisor walk) while returning the rest. Use AsPartialResult to detect it. +type PartialResultError = azrecs.PartialResultError + +// AsPartialResult returns the *PartialResultError in err's chain, or nil. +func AsPartialResult(err error) *PartialResultError { + var partial *PartialResultError + if errors.As(err, &partial) { + return partial + } + return nil +} + // PartialSubscriptionFailureError reports that an org-wide fan-out completed // with some subscriptions queried successfully and others not. // @@ -344,7 +359,18 @@ func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Reco out := make([]common.Recommendation, 0, total) failed := make([]SubscriptionFailure, 0, len(subs)+len(unmatched)) + var skipped []string for i, err := range errs { + if partial := AsPartialResult(err); partial != nil { + // The subscription answered; some recommendations were dropped. + // Keep its recommendations and aggregate what was dropped. + out = append(out, results[i]...) + for _, s := range partial.Skipped { + skipped = append(skipped, subs[i].subscriptionID+": "+s) + } + logging.Warnf("Azure subscription %s recommendations incomplete: %v", subs[i].subscriptionID, err) + continue + } if err != nil { failed = append(failed, SubscriptionFailure{SubscriptionID: subs[i].subscriptionID, Err: err}) logging.Warnf("Azure subscription %s recommendations: %v", subs[i].subscriptionID, err) @@ -362,6 +388,9 @@ func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Reco attempted := len(subs) + len(unmatched) if len(failed) == 0 { + if len(skipped) > 0 { + return out, &PartialResultError{Skipped: skipped} + } return out, nil } if len(failed) == attempted { diff --git a/providers/azure/recommendations_multi_subscription_test.go b/providers/azure/recommendations_multi_subscription_test.go index 79b7f19c..17723346 100644 --- a/providers/azure/recommendations_multi_subscription_test.go +++ b/providers/azure/recommendations_multi_subscription_test.go @@ -459,3 +459,23 @@ func TestMultiSubscriptionRecommendationsClient_GetRecommendationsForService_Pas assert.Equal(t, common.ServiceCompute, fake1.gotParams.Service) assert.Equal(t, common.ServiceCompute, fake2.gotParams.Service) } + +// Reviewer probe for go#366: two subscriptions, each with one recommendation +// and one skipped one. A partial result must keep the recommendations and +// aggregate the skips, not be counted as a failed subscription. +func TestMultiSubscriptionRecommendationsClient_PartialResultKeepsRecommendations(t *testing.T) { + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": {recs: []common.Recommendation{{Account: "sub-1"}}, err: &PartialResultError{Skipped: []string{"compute: EUR"}}}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2"}}, err: &PartialResultError{Skipped: []string{"compute: bad sku"}}}, + }) + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, twoTestAccounts()) + require.NoError(t, err) + + recs, err := client.GetAllRecommendations(context.Background()) + assert.Len(t, recs, 2) + partial := AsPartialResult(err) + require.NotNil(t, partial, "got %v", err) + assert.ElementsMatch(t, []string{"sub-1: compute: EUR", "sub-2: compute: bad sku"}, partial.Skipped) + assert.True(t, partial.Partial()) + assert.Nil(t, AsPartialSubscriptionFailure(err), "a partial result is not a failed subscription") +}