fix(azure): surface skipped consumption recommendations as a typed partial-result error

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,80 @@ 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
+}
+
+// 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, "; "))
+}
+
 // 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

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)
 }
 

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

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 {

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")
+}

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)
+}

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
 	}

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
 	}

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
 	}

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
 	}

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
 	}

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
 	}

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
 	}