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/7] 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 046af0452b5d69dbfb601a965f6383e6dfdc3ba3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:52:19 +0200 Subject: [PATCH 2/7] fix(azure): derive Advisor recommendation fields from the payload and fail loud on truncation Only subscription-scoped reservation and Savings Plan Advisor recommendations are converted; resize and shutdown recommendations are no longer emitted as RI purchases with count 0. Commitment type, term, SKU and quantity come from the extended properties and a missing or unknown value is skipped with a recorded reason instead of defaulting. A page cap or page error now surfaces as a PartialResultError rather than a silently partial list. Refs #292 Refs #252 Co-Authored-By: Claude Opus 5.5 --- .../internal/recommendations/consumption.go | 8 +- providers/azure/pagination_cap_test.go | 16 +- providers/azure/recommendations.go | 167 ++++++++++++++---- .../azure/recommendations_advisor_test.go | 130 ++++++++++++++ providers/azure/recommendations_test.go | 102 +---------- 5 files changed, 275 insertions(+), 148 deletions(-) create mode 100644 providers/azure/recommendations_advisor_test.go diff --git a/providers/azure/internal/recommendations/consumption.go b/providers/azure/internal/recommendations/consumption.go index db25b833..39f909fd 100644 --- a/providers/azure/internal/recommendations/consumption.go +++ b/providers/azure/internal/recommendations/consumption.go @@ -300,7 +300,9 @@ func WithSkipLog(ctx context.Context) (context.Context, *SkipLog) { return context.WithValue(ctx, skipLogKey{}, l), l } -func recordSkip(ctx context.Context, service string, err error) { +// RecordSkip logs a dropped recommendation and records it in the context's +// SkipLog, if any. +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() @@ -336,7 +338,7 @@ func (e *PartialResultError) Error() string { func ExtractConsumptionOrSkip(ctx context.Context, service string, rec armconsumption.ReservationRecommendationClassification) *ExtractedFields { f, err := ExtractConsumption(rec) if err != nil { - recordSkip(ctx, service, err) + RecordSkip(ctx, service, err) return nil } return f @@ -354,7 +356,7 @@ func AppendConsumptionVariants(ctx context.Context, service string, recs []commo if IsCollectionFailure(err) { return recs, fmt.Errorf("azure %s: %w", service, err) } - recordSkip(ctx, service, err) + RecordSkip(ctx, service, err) return recs, nil } return append(recs, variants...), nil diff --git a/providers/azure/pagination_cap_test.go b/providers/azure/pagination_cap_test.go index 51c78f94..1edcb7d7 100644 --- a/providers/azure/pagination_cap_test.go +++ b/providers/azure/pagination_cap_test.go @@ -152,8 +152,8 @@ func TestGetRegions_PageCap(t *testing.T) { } func advisorPage(n int) armadvisor.RecommendationsClientListResponse { - field := "Microsoft.Compute/virtualMachines" - id := "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachines/vm" + string(rune('a'+n)) + field := "Microsoft.Subscriptions/subscriptions" + id := "/subscriptions/123/providers/Microsoft.Advisor/recommendations/rec" + string(rune('a'+n)) savings, currency := "1000.00", "USD" return armadvisor.RecommendationsClientListResponse{ResourceRecommendationBaseListResult: armadvisor.ResourceRecommendationBaseListResult{ Value: []*armadvisor.ResourceRecommendationBase{{ @@ -161,8 +161,12 @@ func advisorPage(n int) armadvisor.RecommendationsClientListResponse { Properties: &armadvisor.RecommendationProperties{ ImpactedField: &field, ExtendedProperties: map[string]*string{ - "annualSavingsAmount": &savings, - "savingsCurrency": ¤cy, + "annualSavingsAmount": &savings, + "savingsCurrency": ¤cy, + "reservedResourceType": strPtr("VirtualMachines"), + "term": strPtr("P1Y"), + "sku": strPtr("Standard_D2s_v3"), + "qty": strPtr("1"), }, }, }}, @@ -173,10 +177,10 @@ func TestCollectAdvisorRecommendations_PageCap(t *testing.T) { adapter := &RecommendationsClientAdapter{subscriptionID: "test-subscription"} params := common.RecommendationParams{} - t.Run("endless pager stops at the cap keeping partial results", func(t *testing.T) { + t.Run("endless pager stops at the cap with a truncation error and the partial results", func(t *testing.T) { pager := &fakePager[armadvisor.RecommendationsClientListResponse]{limit: 10000, page: advisorPage} recs, err := adapter.collectAdvisorRecommendations(context.Background(), params, pager) - require.NoError(t, err) + require.ErrorIs(t, err, errAdvisorTruncated) assert.Equal(t, maxAdvisorPages, pager.calls) assert.NotEmpty(t, recs) }) diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 083339e7..04b38b65 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -3,6 +3,7 @@ package azure import ( "context" + "errors" "fmt" "math" "strconv" @@ -209,7 +210,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p advisorFn = r.getAdvisorRecommendations } goService(&advisorErr, func() { - advisorRecs, advisorErr = advisorFn(gctx, *params) + advisorRecs, advisorErr = collectAdvisorOrSkip(gctx, advisorFn, *params) }) // Wait for all goroutines. g.Wait() always returns nil because every @@ -370,9 +371,28 @@ type AdvisorPager interface { NextPage(ctx context.Context) (armadvisor.RecommendationsClientListResponse, error) } +// errAdvisorTruncated marks an Advisor walk that stopped before the last +// page (page cap or page error); the recommendations gathered so far are +// returned with it. +var errAdvisorTruncated = errors.New("azure advisor: recommendation list is incomplete") + +// collectAdvisorOrSkip runs the Advisor collection and reports a truncated +// walk through the context's skip log, keeping the recommendations gathered so +// far, so GetRecommendations surfaces it as a *azrecs.PartialResultError. +// Any other error is returned unchanged. +func collectAdvisorOrSkip(ctx context.Context, fn func(context.Context, common.RecommendationParams) ([]common.Recommendation, error), params common.RecommendationParams) ([]common.Recommendation, error) { + recs, err := fn(ctx, params) + if errors.Is(err, errAdvisorTruncated) { + azrecs.RecordSkip(ctx, "advisor", err) + return recs, nil + } + return recs, err +} + // collectAdvisorRecommendations walks the Advisor pager. A canceled context -// is an error; a page error or hitting the page cap keeps what was collected -// so far with a warning (the intentional partial-OK path). +// is an error. Hitting the page cap or a page error returns the +// recommendations collected so far together with errAdvisorTruncated, never a +// bare partial result. func (r *RecommendationsClientAdapter) collectAdvisorRecommendations(ctx context.Context, params common.RecommendationParams, pager AdvisorPager) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) for pageIdx := 0; pager.More(); pageIdx++ { @@ -380,19 +400,13 @@ func (r *RecommendationsClientAdapter) collectAdvisorRecommendations(ctx context return nil, fmt.Errorf("azure advisor: context canceled during pagination: %w", err) } if pageIdx >= maxAdvisorPages { - logging.Warnf("Azure Advisor pagination cap (%d pages) reached after %d recommendations (partial results returned)", maxAdvisorPages, len(recommendations)) - break + return recommendations, fmt.Errorf("%w: pagination cap (%d pages) reached after %d recommendations", errAdvisorTruncated, maxAdvisorPages, len(recommendations)) } page, err := pager.NextPage(ctx) if err != nil { - // Advisor partial-OK is intentional: per-service reservation results - // (compute, database, etc.) remain useful on their own when Advisor - // pagination fails mid-stream. Warnf with the accumulated count so - // operators can assess the completeness of the Advisor slice. - logging.Warnf("Azure Advisor pagination error after %d recommendations (partial results returned): %v", len(recommendations), err) - break + return recommendations, fmt.Errorf("%w: page error after %d recommendations: %v", errAdvisorTruncated, len(recommendations), err) } - recommendations = r.appendAdvisorPageRecs(params, page, recommendations) + recommendations = r.appendAdvisorPageRecs(ctx, params, page, recommendations) } return recommendations, nil @@ -402,6 +416,7 @@ func (r *RecommendationsClientAdapter) collectAdvisorRecommendations(ctx context // and appends them to the provided slice. Pulled out of getAdvisorRecommendations // to keep that function under the cyclomatic limit. func (r *RecommendationsClientAdapter) appendAdvisorPageRecs( + ctx context.Context, params common.RecommendationParams, page armadvisor.RecommendationsClientListResponse, recommendations []common.Recommendation, @@ -410,8 +425,7 @@ func (r *RecommendationsClientAdapter) appendAdvisorPageRecs( if advisorRec.Properties == nil { continue } - // Convert Azure Advisor recommendation to our common format - rec := r.convertAdvisorRecommendation(advisorRec) + rec := r.convertAdvisorRecommendation(ctx, advisorRec) if rec != nil && shouldIncludeService(params, rec.Service) { // Preserve Advisor-provided EstimatedSavings when both OnDemandCost // and CommitmentCost are unset (zero), since ExpandPaymentVariants @@ -448,14 +462,56 @@ func resolveAdvisorRegion(advisorRec *armadvisor.ResourceRecommendationBase) str return "" } -// convertAdvisorRecommendation converts an Azure Advisor recommendation to common format. -func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(advisorRec *armadvisor.ResourceRecommendationBase) *common.Recommendation { +// Advisor ExtendedProperties keys. The SDK types ExtendedProperties as a bare +// map[string]*string, so the keys are not in the SDK. annualSavingsAmount, +// savingsCurrency, term and lookbackPeriod are the names Microsoft's Advisor +// Resource Graph samples read from properties.extendedProperties +// (learn.microsoft.com/azure/advisor/advisor-azure-resource-graph); +// reservedResourceType, recommendationSubCategory, sku and qty are the keys +// this collector has always relied on. +const ( + advisorKeySavings = "annualSavingsAmount" + advisorKeyCurrency = "savingsCurrency" + advisorKeyTerm = "term" + advisorKeySKU = "sku" + advisorKeyQty = "qty" + advisorKeyRRT = "reservedResourceType" +) + +// advisorPurchaseKind classifies an Advisor recommendation. Only +// subscription-scoped reservation (reservedResourceType) and Savings Plan +// recommendations are commitment purchases; resource-level recommendations +// such as resize or shutdown are not, and are ignored (ok=false, no error). +// A reservedResourceType this collector cannot map is recorded as a skip. +func advisorPurchaseKind(ctx context.Context, ext map[string]*string) (common.CommitmentType, string, bool) { + service := serviceFromExtendedProperties(ext) + if service == "" { + if v := ext[advisorKeyRRT]; v != nil && *v != "" { + azrecs.RecordSkip(ctx, "advisor", fmt.Errorf("unsupported %s %q", advisorKeyRRT, *v)) + } + return "", "", false + } + if v := ext["recommendationSubCategory"]; v != nil && strings.EqualFold(*v, "SavingsPlan") { + return common.CommitmentSavingsPlan, service, true + } + return common.CommitmentReservedInstance, service, true +} + +// convertAdvisorRecommendation converts an Azure Advisor commitment +// recommendation to common format. Payload problems are recorded in the +// context's skip log and yield nil. +// +// Payment option is left empty: the Advisor payload carries no payment-plan +// or price field, and Azure charges the same total for upfront and monthly +// billing, so ExpandPaymentVariants fans it out to both. CommitmentCost stays 0 +// and RecurringMonthlyCost nil (unknown) because the payload has no price. +func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(ctx context.Context, advisorRec *armadvisor.ResourceRecommendationBase) *common.Recommendation { if advisorRec.Properties == nil { return nil } - - service := extractServiceType(advisorRec) - if service == "" { + ext := advisorRec.Properties.ExtendedProperties + commitmentType, service, ok := advisorPurchaseKind(ctx, ext) + if !ok { return nil } @@ -463,36 +519,63 @@ func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(advisorRec * Provider: common.ProviderAzure, Service: common.ServiceType(service), Account: r.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Term: "1yr", - PaymentOption: "upfront", + CommitmentType: commitmentType, } - rec.Region = resolveAdvisorRegion(advisorRec) - if err := populateFromExtendedProperties(rec, advisorRec.Properties.ExtendedProperties); err != nil { - logging.Warnf("Azure Advisor %s recommendation in %q dropped: %v", service, rec.Region, err) + if err := populateFromExtendedProperties(rec, ext); err != nil { + azrecs.RecordSkip(ctx, "advisor", fmt.Errorf("%s recommendation in %q: %w", service, rec.Region, err)) return nil } return rec } -// populateFromExtendedProperties fills savings, SKU, term, and count from -// the Advisor recommendation's ExtendedProperties map. It returns an error -// when the savings figure is absent or unparsable, so the caller never ships -// a fabricated zero. +// populateFromExtendedProperties fills savings, SKU, term and count from the +// Advisor ExtendedProperties map and errors on anything absent or unknown: +// savings, a USD currency label and a known term are always required; a +// reservation also needs its SKU and a positive quantity. func populateFromExtendedProperties(rec *common.Recommendation, ext map[string]*string) error { - annualSavings, err := extFloat(ext, "annualSavingsAmount") + annualSavings, err := extFloat(ext, advisorKeySavings) if err != nil { return err } + if cur := extString(ext, advisorKeyCurrency, ""); !strings.EqualFold(cur, "USD") { + return fmt.Errorf("%s %q is not USD", advisorKeyCurrency, cur) + } rec.EstimatedSavings = annualSavings / 12 - rec.ResourceType = extString(ext, "sku", rec.ResourceType) - rec.Term = extString(ext, "term", rec.Term) - rec.Count = extInt(ext, "qty", rec.Count) + + if rec.Term, err = advisorTerm(ext); err != nil { + return err + } + if rec.CommitmentType == common.CommitmentSavingsPlan { + return nil + } + if rec.ResourceType = extString(ext, advisorKeySKU, ""); rec.ResourceType == "" { + return fmt.Errorf("%s is absent", advisorKeySKU) + } + qty, err := extInt(ext, advisorKeyQty) + if err != nil { + return err + } + rec.Count = qty return nil } +// advisorTerm maps the Advisor term ("P1Y"/"P3Y", or the already-normalised +// "1yr"/"3yr") to the common form and errors on a missing or unknown value. +func advisorTerm(ext map[string]*string) (string, error) { + switch t := extString(ext, advisorKeyTerm, ""); t { + case "P1Y", "1yr": + return "1yr", nil + case "P3Y", "3yr": + return "3yr", nil + case "": + return "", fmt.Errorf("%s is absent", advisorKeyTerm) + default: + return "", fmt.Errorf("unknown %s %q", advisorKeyTerm, t) + } +} + func extString(m map[string]*string, key, fallback string) string { if v, ok := m[key]; ok && v != nil && *v != "" { return *v @@ -518,13 +601,19 @@ func extFloat(m map[string]*string, key string) (float64, error) { return f, nil } -func extInt(m map[string]*string, key string, fallback int) int { - if v, ok := m[key]; ok && v != nil { - if n, err := strconv.Atoi(*v); err == nil { - return n - } +func extInt(m map[string]*string, key string) (int, error) { + v, ok := m[key] + if !ok || v == nil { + return 0, fmt.Errorf("%s is absent", key) } - return fallback + n, err := strconv.Atoi(*v) + if err != nil { + return 0, fmt.Errorf("%s %q is not an integer: %w", key, *v, err) + } + if n <= 0 { + return 0, fmt.Errorf("%s %q is not positive", key, *v) + } + return n, nil } // extractServiceType determines the service type from an Advisor recommendation. diff --git a/providers/azure/recommendations_advisor_test.go b/providers/azure/recommendations_advisor_test.go new file mode 100644 index 00000000..eb89f9da --- /dev/null +++ b/providers/azure/recommendations_advisor_test.go @@ -0,0 +1,130 @@ +package azure + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor" + "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" +) + +// Fixture-based: payloads are shaped from armadvisor.ResourceRecommendationBase +// with the extendedProperties keys documented in advisorKey* (no live Azure +// Advisor data was available). + +func advisorRec(impacted string, ext map[string]*string) *armadvisor.ResourceRecommendationBase { + return &armadvisor.ResourceRecommendationBase{ + Properties: &armadvisor.RecommendationProperties{ImpactedField: &impacted, ExtendedProperties: ext}, + } +} + +func reservationExt(over map[string]*string) map[string]*string { + ext := map[string]*string{ + "reservedResourceType": strPtr("VirtualMachines"), + "annualSavingsAmount": strPtr("1200"), + "savingsCurrency": strPtr("USD"), + "term": strPtr("P3Y"), + "sku": strPtr("Standard_D2s_v3"), + "qty": strPtr("2"), + "region": strPtr("eastus"), + } + for k, v := range over { + if v == nil { + delete(ext, k) + } else { + ext[k] = v + } + } + return ext +} + +const subscriptionScope = "Microsoft.Subscriptions/subscriptions" + +func TestConvertAdvisor_DerivesFieldsFromPayload(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + + ri := a.convertAdvisorRecommendation(context.Background(), advisorRec(subscriptionScope, reservationExt(nil))) + require.NotNil(t, ri) + assert.Equal(t, common.CommitmentReservedInstance, ri.CommitmentType) + assert.Equal(t, "3yr", ri.Term, "term comes from the payload, not a 1yr default") + assert.Equal(t, "Standard_D2s_v3", ri.ResourceType) + assert.Equal(t, 2, ri.Count) + assert.Equal(t, 100.0, ri.EstimatedSavings) + assert.Empty(t, ri.PaymentOption, "the payload carries no payment plan; variants are fanned out later") + + sp := a.convertAdvisorRecommendation(context.Background(), advisorRec(subscriptionScope, map[string]*string{ + "recommendationSubCategory": strPtr("SavingsPlan"), + "annualSavingsAmount": strPtr("2400"), + "savingsCurrency": strPtr("USD"), + "term": strPtr("P1Y"), + })) + require.NotNil(t, sp) + assert.Equal(t, common.CommitmentSavingsPlan, sp.CommitmentType) + assert.Equal(t, "1yr", sp.Term) +} + +func TestConvertAdvisor_NonPurchaseRecommendationsAreIgnored(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + // Resize/shutdown: resource-scoped, no reservedResourceType. + ctx, skips := azrecs.WithSkipLog(context.Background()) + rightsize := advisorRec("Microsoft.Compute/virtualMachines", map[string]*string{ + "annualSavingsAmount": strPtr("1000"), "savingsCurrency": strPtr("USD"), + }) + assert.Nil(t, a.convertAdvisorRecommendation(ctx, rightsize)) + assert.Empty(t, skips.Entries(), "a non-purchase recommendation is not a skip") +} + +func TestConvertAdvisor_UnknownOrMissingValuesAreSkippedLoudly(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + for name, over := range map[string]map[string]*string{ + "unknown term": {"term": strPtr("P5Y")}, + "missing term": {"term": nil}, + "non-USD": {"savingsCurrency": strPtr("EUR")}, + "missing currency": {"savingsCurrency": nil}, + "missing sku": {"sku": nil}, + "missing qty": {"qty": nil}, + "zero qty": {"qty": strPtr("0")}, + "junk qty": {"qty": strPtr("two")}, + "unknown rrt": {"reservedResourceType": strPtr("Quantum")}, + "missing savings": {"annualSavingsAmount": nil}, + } { + t.Run(name, func(t *testing.T) { + ctx, skips := azrecs.WithSkipLog(context.Background()) + assert.Nil(t, a.convertAdvisorRecommendation(ctx, advisorRec(subscriptionScope, reservationExt(over)))) + assert.Len(t, skips.Entries(), 1) + }) + } +} + +func TestAppendAdvisorPageRecs_VariantsKeepCostUnknown(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + page := armadvisor.RecommendationsClientListResponse{ResourceRecommendationBaseListResult: armadvisor.ResourceRecommendationBaseListResult{ + Value: []*armadvisor.ResourceRecommendationBase{advisorRec(subscriptionScope, reservationExt(nil))}, + }} + recs := a.appendAdvisorPageRecs(context.Background(), common.RecommendationParams{}, page, nil) + require.Len(t, recs, 2) + for i, payment := range []string{"upfront", "monthly"} { + assert.Equal(t, payment, recs[i].PaymentOption) + assert.Equal(t, 100.0, recs[i].EstimatedSavings) + assert.Nil(t, recs[i].RecurringMonthlyCost, "no price in the payload: unknown, not zero") + } +} + +func TestGetRecommendations_AdvisorTruncationIsSurfaced(t *testing.T) { + overrideServiceClientFns(t, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}) + a := &RecommendationsClientAdapter{cred: &mockAzureTokenCredential{}, subscriptionID: "sub"} + a.getAdvisorRecsFn = func(ctx context.Context, p common.RecommendationParams) ([]common.Recommendation, error) { + pager := &fakePager[armadvisor.RecommendationsClientListResponse]{limit: 10000, page: advisorPage} + return a.collectAdvisorRecommendations(ctx, p, pager) + } + recs, err := a.GetRecommendations(context.Background(), &common.RecommendationParams{}) + assert.NotEmpty(t, recs, "what was collected is kept") + var partial *azrecs.PartialResultError + require.True(t, errors.As(err, &partial), "a capped Advisor walk must not look complete: %v", err) + assert.Contains(t, partial.Skipped[0], "pagination cap") +} diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index d049d1e0..8a8c13c3 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -430,35 +430,6 @@ func TestExtractServiceType_NilImpactedField(t *testing.T) { assert.Equal(t, "", result) } -func TestConvertAdvisorRecommendation(t *testing.T) { - adapter := &RecommendationsClientAdapter{ - subscriptionID: "test-subscription", - } - - impactedField := "Microsoft.Compute/virtualMachines" - resourceID := "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachines/vm1" - - rec := &armadvisor.ResourceRecommendationBase{ - ID: &resourceID, - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &impactedField, - ExtendedProperties: map[string]*string{ - "annualSavingsAmount": strPtr("1000.00"), - "savingsCurrency": strPtr("USD"), - }, - }, - } - - result := adapter.convertAdvisorRecommendation(rec) - require.NotNil(t, result) - assert.Equal(t, common.ProviderAzure, result.Provider) - assert.Equal(t, common.ServiceCompute, result.Service) - assert.Equal(t, "test-subscription", result.Account) - assert.Equal(t, common.CommitmentReservedInstance, result.CommitmentType) - assert.Equal(t, "1yr", result.Term) - assert.Equal(t, "upfront", result.PaymentOption) -} - func TestConvertAdvisorRecommendation_NilProperties(t *testing.T) { adapter := &RecommendationsClientAdapter{ subscriptionID: "test-subscription", @@ -468,7 +439,7 @@ func TestConvertAdvisorRecommendation_NilProperties(t *testing.T) { Properties: nil, } - result := adapter.convertAdvisorRecommendation(rec) + result := adapter.convertAdvisorRecommendation(context.Background(), rec) assert.Nil(t, result) } @@ -484,50 +455,10 @@ func TestConvertAdvisorRecommendation_UnknownService(t *testing.T) { }, } - result := adapter.convertAdvisorRecommendation(rec) + result := adapter.convertAdvisorRecommendation(context.Background(), rec) assert.Nil(t, result) } -// A real zero saving must survive conversion, while an absent or unparsable -// annualSavingsAmount must drop the recommendation instead of shipping $0. -func TestConvertAdvisorRecommendation_ZeroVersusAbsentSavings(t *testing.T) { - adapter := &RecommendationsClientAdapter{subscriptionID: "test-subscription"} - impactedField := "Microsoft.Compute/virtualMachines" - mkRec := func(ext map[string]*string) *armadvisor.ResourceRecommendationBase { - return &armadvisor.ResourceRecommendationBase{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &impactedField, - ExtendedProperties: ext, - }, - } - } - - zero := adapter.convertAdvisorRecommendation(mkRec(map[string]*string{"annualSavingsAmount": strPtr("0")})) - require.NotNil(t, zero, "a real zero saving must be kept") - assert.Equal(t, 0.0, zero.EstimatedSavings) - - populated := adapter.convertAdvisorRecommendation(mkRec(map[string]*string{"annualSavingsAmount": strPtr("1200")})) - require.NotNil(t, populated) - assert.Equal(t, 100.0, populated.EstimatedSavings) - - for name, ext := range map[string]map[string]*string{ - "key absent": {"sku": strPtr("Standard_D2s_v3")}, - "nil value": {"annualSavingsAmount": nil}, - "nil map": nil, - "thousands separator": {"annualSavingsAmount": strPtr("1,234.00")}, - "empty string": {"annualSavingsAmount": strPtr("")}, - "NaN": {"annualSavingsAmount": strPtr("NaN")}, - "nan": {"annualSavingsAmount": strPtr("nan")}, - "Inf": {"annualSavingsAmount": strPtr("Inf")}, - "-Inf": {"annualSavingsAmount": strPtr("-Inf")}, - "negative": {"annualSavingsAmount": strPtr("-1200")}, - } { - t.Run(name, func(t *testing.T) { - assert.Nil(t, adapter.convertAdvisorRecommendation(mkRec(ext))) - }) - } -} - func strPtr(s string) *string { return &s } @@ -861,32 +792,3 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { assert.Contains(t, services, common.ServiceNoSQL, "cosmosdb recs must be present despite db error") assert.NotContains(t, services, common.ServiceRelationalDB, "db recs must be absent when db errors") } - -// The Advisor path must keep the pre-existing expansion: Advisor supplies only -// a monthly saving (no on-demand or commitment figure), which survives onto -// both variants with no recurring cost and no scaling applied. -func TestAppendAdvisorPageRecs_ExpansionUnchanged(t *testing.T) { - adapter := &RecommendationsClientAdapter{subscriptionID: "test-subscription"} - impactedField := "Microsoft.Compute/virtualMachines" - page := armadvisor.RecommendationsClientListResponse{ - ResourceRecommendationBaseListResult: armadvisor.ResourceRecommendationBaseListResult{ - Value: []*armadvisor.ResourceRecommendationBase{{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &impactedField, - ExtendedProperties: map[string]*string{"annualSavingsAmount": strPtr("1200"), "sku": strPtr("Standard_D2s_v3"), "term": strPtr("3yr"), "qty": strPtr("2")}, - }, - }}, - }, - } - - recs := adapter.appendAdvisorPageRecs(common.RecommendationParams{}, page, nil) - require.Len(t, recs, 2) - for i, payment := range []string{"upfront", "monthly"} { - assert.Equal(t, payment, recs[i].PaymentOption) - assert.Equal(t, 100.0, recs[i].EstimatedSavings) - assert.Zero(t, recs[i].OnDemandCost) - assert.Zero(t, recs[i].CommitmentCost) - assert.Zero(t, recs[i].SavingsPercentage) - assert.Nil(t, recs[i].RecurringMonthlyCost) - } -} From 267341580154af6505946e0946f3848631c6f970 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:54:56 +0200 Subject: [PATCH 3/7] fix(azure): drop Advisor Savings Plan rows the Benefit Recommendations client covers Co-Authored-By: Claude Opus 5.5 --- providers/azure/recommendations.go | 29 ++++++++++++++++++- .../azure/recommendations_advisor_test.go | 26 +++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 04b38b65..c5b272a3 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -246,7 +246,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // credential failure -- the same hazard the savingsplans exclusion // prevents. When getAdvisorRecommendations is changed to propagate // hard errors, flip this flag back to true. - serviceResult{"advisor", advisorRecs, advisorErr, false}) + serviceResult{"advisor", dropCoveredAdvisorSavingsPlans(advisorRecs, spRecs), advisorErr, false}) return withSkips(merged, err, skips) } @@ -264,6 +264,33 @@ func withSkips(recs []common.Recommendation, err error, skips *azrecs.SkipLog) ( return recs, nil } +// dropCoveredAdvisorSavingsPlans removes Advisor Savings Plan rows that the +// savings-plan client (Benefit Recommendations API) already covers, matched on +// (account, term). Advisor's row carries only a saving, no commitment amount, +// so the Benefit Recommendations row is the better record of the same advice. +// Matching on a key, not dropping all Advisor SP rows, keeps Advisor's advice +// when that client returns nothing for an account and term. +func dropCoveredAdvisorSavingsPlans(advisor, sp []common.Recommendation) []common.Recommendation { + type key struct{ account, term string } + covered := make(map[key]bool) + for _, r := range sp { + if r.CommitmentType == common.CommitmentSavingsPlan { + covered[key{r.Account, r.Term}] = true + } + } + if len(covered) == 0 { + return advisor + } + out := make([]common.Recommendation, 0, len(advisor)) + for _, r := range advisor { + if r.CommitmentType == common.CommitmentSavingsPlan && covered[key{r.Account, r.Term}] { + continue + } + out = append(out, r) + } + return out +} + // serviceResult bundles a per-service collection outcome for the deterministic // merge in mergeServiceResults. Extracted into a helper so GetRecommendations // stays under the cyclomatic-complexity gate after the post-Wait ctx.Err() diff --git a/providers/azure/recommendations_advisor_test.go b/providers/azure/recommendations_advisor_test.go index eb89f9da..872e28e2 100644 --- a/providers/azure/recommendations_advisor_test.go +++ b/providers/azure/recommendations_advisor_test.go @@ -128,3 +128,29 @@ func TestGetRecommendations_AdvisorTruncationIsSurfaced(t *testing.T) { require.True(t, errors.As(err, &partial), "a capped Advisor walk must not look complete: %v", err) assert.Contains(t, partial.Skipped[0], "pagination cap") } + +func TestGetRecommendations_AdvisorSavingsPlanDedupedAgainstBenefitRecs(t *testing.T) { + sp := func(account, term string, from string) common.Recommendation { + return common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute, + CommitmentType: common.CommitmentSavingsPlan, Account: account, Term: term, Region: from} + } + benefit := sp("sub", "1yr", "benefit") + run := func(spRecs []common.Recommendation, advisor []common.Recommendation) []common.Recommendation { + overrideServiceClientFns(t, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{recs: spRecs}) + a := &RecommendationsClientAdapter{cred: &mockAzureTokenCredential{}, subscriptionID: "sub", + getAdvisorRecsFn: func(context.Context, common.RecommendationParams) ([]common.Recommendation, error) { + return advisor, nil + }} + recs, err := a.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + return recs + } + advisorRI := common.Recommendation{CommitmentType: common.CommitmentReservedInstance, Account: "sub", Term: "1yr", Region: "advisor-ri"} + + got := run([]common.Recommendation{benefit}, []common.Recommendation{sp("sub", "1yr", "advisor"), sp("sub", "3yr", "advisor"), advisorRI}) + assert.ElementsMatch(t, []common.Recommendation{benefit, sp("sub", "3yr", "advisor"), advisorRI}, got, + "the covered (account, term) Advisor SP row is dropped; other terms and RI rows stay") + + got = run(nil, []common.Recommendation{sp("sub", "1yr", "advisor")}) + assert.Len(t, got, 1, "Advisor SP advice is kept when the benefit source returns nothing") +} From 2de6006558f307aa739e96094d5f8637112a71d6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:35:00 +0200 Subject: [PATCH 4/7] 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") +} From 462df7e8812616cdb8eb25ec8331c9f53a64d05d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:35:59 +0200 Subject: [PATCH 5/7] fix(azure): ignore unmapped Advisor reservation types and tidy the Advisor converter Advisor routinely returns reservations for services this collector does not price; recording them as skips made nearly every subscription partial. Savings Plan rows now use ServiceSavingsPlansAll, the savings amount guard tests are restored, and the now-unused extractServiceType is removed. Co-Authored-By: Claude Opus 5.5 --- providers/azure/recommendations.go | 40 ++--------- .../azure/recommendations_advisor_test.go | 27 ++++++- providers/azure/recommendations_test.go | 71 ------------------- 3 files changed, 31 insertions(+), 107 deletions(-) diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index c5b272a3..62f0ded3 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -509,12 +509,14 @@ const ( // subscription-scoped reservation (reservedResourceType) and Savings Plan // recommendations are commitment purchases; resource-level recommendations // such as resize or shutdown are not, and are ignored (ok=false, no error). -// A reservedResourceType this collector cannot map is recorded as a skip. +// An unmapped reservedResourceType is ignored (debug log), not a skip. func advisorPurchaseKind(ctx context.Context, ext map[string]*string) (common.CommitmentType, string, bool) { service := serviceFromExtendedProperties(ext) if service == "" { if v := ext[advisorKeyRRT]; v != nil && *v != "" { - azrecs.RecordSkip(ctx, "advisor", fmt.Errorf("unsupported %s %q", advisorKeyRRT, *v)) + // Advisor routinely returns reservations for services this + // collector does not price; they are not ours to convert. + logging.Debugf("azure advisor: ignoring %s %q", advisorKeyRRT, *v) } return "", "", false } @@ -643,38 +645,6 @@ func extInt(m map[string]*string, key string) (int, error) { return n, nil } -// extractServiceType determines the service type from an Advisor recommendation. -// First checks ImpactedField (resource-scoped), then falls back to -// ExtendedProperties (subscription-scoped reservations/savings plans). -func extractServiceType(rec *armadvisor.ResourceRecommendationBase) string { - if rec.Properties == nil { - return "" - } - if svc := serviceFromImpactedField(rec.Properties.ImpactedField); svc != "" { - return svc - } - return serviceFromExtendedProperties(rec.Properties.ExtendedProperties) -} - -// serviceFromImpactedField maps Azure resource namespace to service type. -func serviceFromImpactedField(field *string) string { - if field == nil { - return "" - } - f := *field - switch { - case strings.Contains(f, "Microsoft.Compute"): - return string(common.ServiceCompute) - case strings.Contains(f, "Microsoft.Sql"): - return string(common.ServiceRelationalDB) - case strings.Contains(f, "Microsoft.Cache"): - return string(common.ServiceCache) - case strings.Contains(f, "Microsoft.DBforMySQL"), strings.Contains(f, "Microsoft.DBforPostgreSQL"): - return string(common.ServiceRelationalDB) - } - return "" -} - // serviceFromExtendedProperties resolves service type for subscription-scoped // recommendations where ImpactedField is "Microsoft.Subscriptions/subscriptions". func serviceFromExtendedProperties(ext map[string]*string) string { @@ -693,7 +663,7 @@ func serviceFromExtendedProperties(ext map[string]*string) string { } if subcat, ok := ext["recommendationSubCategory"]; ok && subcat != nil { if strings.EqualFold(*subcat, "SavingsPlan") { - return string(common.ServiceCompute) + return string(common.ServiceSavingsPlansAll) } } return "" diff --git a/providers/azure/recommendations_advisor_test.go b/providers/azure/recommendations_advisor_test.go index 872e28e2..e0ffcd5f 100644 --- a/providers/azure/recommendations_advisor_test.go +++ b/providers/azure/recommendations_advisor_test.go @@ -65,6 +65,7 @@ func TestConvertAdvisor_DerivesFieldsFromPayload(t *testing.T) { })) require.NotNil(t, sp) assert.Equal(t, common.CommitmentSavingsPlan, sp.CommitmentType) + assert.Equal(t, common.ServiceSavingsPlansAll, sp.Service) assert.Equal(t, "1yr", sp.Term) } @@ -90,7 +91,6 @@ func TestConvertAdvisor_UnknownOrMissingValuesAreSkippedLoudly(t *testing.T) { "missing qty": {"qty": nil}, "zero qty": {"qty": strPtr("0")}, "junk qty": {"qty": strPtr("two")}, - "unknown rrt": {"reservedResourceType": strPtr("Quantum")}, "missing savings": {"annualSavingsAmount": nil}, } { t.Run(name, func(t *testing.T) { @@ -154,3 +154,28 @@ func TestGetRecommendations_AdvisorSavingsPlanDedupedAgainstBenefitRecs(t *testi got = run(nil, []common.Recommendation{sp("sub", "1yr", "advisor")}) assert.Len(t, got, 1, "Advisor SP advice is kept when the benefit source returns nothing") } + +func TestConvertAdvisor_UnmappedReservedResourceTypeIsIgnoredNotSkipped(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + ctx, skips := azrecs.WithSkipLog(context.Background()) + got := a.convertAdvisorRecommendation(ctx, advisorRec(subscriptionScope, reservationExt(map[string]*string{"reservedResourceType": strPtr("CosmosDb")}))) + assert.Nil(t, got) + assert.Empty(t, skips.Entries(), "a service we do not price must not make every subscription look partial") +} + +func TestConvertAdvisor_RejectsBadSavingsAmounts(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + for name, v := range map[string]*string{ + "nil": nil, "thousands separator": strPtr("1,234.00"), "empty": strPtr(""), + "NaN": strPtr("NaN"), "Inf": strPtr("Inf"), "-Inf": strPtr("-Inf"), "negative": strPtr("-1200"), + } { + t.Run(name, func(t *testing.T) { + ctx, skips := azrecs.WithSkipLog(context.Background()) + assert.Nil(t, a.convertAdvisorRecommendation(ctx, advisorRec(subscriptionScope, reservationExt(map[string]*string{"annualSavingsAmount": v})))) + assert.Len(t, skips.Entries(), 1) + }) + } + zero := a.convertAdvisorRecommendation(context.Background(), advisorRec(subscriptionScope, reservationExt(map[string]*string{"annualSavingsAmount": strPtr("0")}))) + require.NotNil(t, zero, "a real zero saving is kept") + assert.Equal(t, 0.0, zero.EstimatedSavings) +} diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index 8a8c13c3..f2d7ad0c 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -359,77 +359,6 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel "GetRecommendations must propagate the parent ctx error after g.Wait()") } -func TestExtractServiceType(t *testing.T) { - tests := []struct { - name string - impactedField string - expected string - }{ - { - name: "Microsoft.Compute returns Compute", - impactedField: "Microsoft.Compute/virtualMachines", - expected: string(common.ServiceCompute), - }, - { - name: "Microsoft.Sql returns RelationalDB", - impactedField: "Microsoft.Sql/servers", - expected: string(common.ServiceRelationalDB), - }, - { - name: "Microsoft.Cache returns Cache", - impactedField: "Microsoft.Cache/Redis", - expected: string(common.ServiceCache), - }, - { - name: "Microsoft.DBforMySQL returns RelationalDB", - impactedField: "Microsoft.DBforMySQL/servers", - expected: string(common.ServiceRelationalDB), - }, - { - name: "Microsoft.DBforPostgreSQL returns RelationalDB", - impactedField: "Microsoft.DBforPostgreSQL/servers", - expected: string(common.ServiceRelationalDB), - }, - { - name: "Unknown resource type returns empty", - impactedField: "Microsoft.Storage/storageAccounts", - expected: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - rec := &armadvisor.ResourceRecommendationBase{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &tt.impactedField, - }, - } - result := extractServiceType(rec) - assert.Equal(t, tt.expected, result) - }) - } -} - -func TestExtractServiceType_NilProperties(t *testing.T) { - // Test with nil properties - rec := &armadvisor.ResourceRecommendationBase{ - Properties: nil, - } - result := extractServiceType(rec) - assert.Equal(t, "", result) -} - -func TestExtractServiceType_NilImpactedField(t *testing.T) { - // Test with nil impacted field - rec := &armadvisor.ResourceRecommendationBase{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: nil, - }, - } - result := extractServiceType(rec) - assert.Equal(t, "", result) -} - func TestConvertAdvisorRecommendation_NilProperties(t *testing.T) { adapter := &RecommendationsClientAdapter{ subscriptionID: "test-subscription", From 23e587d39260967f2569680fc4f681be0a1612f6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 23:07:31 +0200 Subject: [PATCH 6/7] fix(azure): collect Synapse recommendations in the provider fan-out (#382) * fix(azure): collect Synapse recommendations in the provider fan-out The Synapse client was complete and tested but never called from GetRecommendations. Search and Managed Redis stay out: Search has no valid Consumption resource type and no reservation prices (#197), and Managed Redis queries the same RedisCache resource type as the cache client, so wiring it would emit the same recommendations twice. Refs #47 Co-Authored-By: Claude Opus 5.5 * fix(azure): repair merge of the Synapse wiring onto the Advisor branch Co-Authored-By: Claude Opus 5.5 * test(azure): stub the Synapse client in the adapter fan-out tests Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- providers/azure/recommendations.go | 37 ++++++++++++++----- .../azure/recommendations_advisor_test.go | 15 ++++++++ providers/azure/recommendations_test.go | 12 ++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 62f0ded3..ee898e8a 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -22,6 +22,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cosmosdb" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/database" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/savingsplans" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/synapse" ) // serviceRecsGetter is the narrow interface satisfied by each per-service @@ -59,6 +60,9 @@ var ( newSavingsPlansClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { return savingsplans.NewClient(cred, sub, region) } + newSynapseClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { + return synapse.NewClient(cred, sub, region) + } ) // RecommendationsClientAdapter aggregates Azure reservation recommendations across all services. @@ -106,17 +110,17 @@ func NewRecommendationsClientAdapter(cred azcore.TokenCredential, subscriptionID // (see known_issues/10_azure_provider.md CRITICAL "Recommendation converters // ignore the API response entirely" for the matching converter work). // -// All six service calls run concurrently under errgroup. Each goroutine captures +// All seven service calls run concurrently under errgroup. Each goroutine captures // its own error and returns nil to the group so that a single service failure // does not cancel sibling calls. Results are appended in a deterministic order -// (compute → database → cache → cosmosdb → savingsplans → advisor) after all goroutines finish. +// (compute → database → cache → cosmosdb → synapse → savingsplans → advisor) after all goroutines finish. func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { if params == nil { return nil, fmt.Errorf("params cannot be nil") } var ( - computeRecs, dbRecs, cacheRecs, cosmosRecs, advisorRecs, spRecs []common.Recommendation - computeErr, dbErr, cacheErr, cosmosErr, advisorErr, spErr error + computeRecs, dbRecs, cacheRecs, cosmosRecs, advisorRecs, spRecs, synapseRecs []common.Recommendation + computeErr, dbErr, cacheErr, cosmosErr, advisorErr, spErr, synapseErr error ) g, gctx := errgroup.WithContext(ctx) @@ -154,6 +158,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p includeCache := shouldIncludeService(*params, common.ServiceCache) includeCosmos := shouldIncludeService(*params, common.ServiceNoSQL) includeSP := shouldIncludeService(*params, common.ServiceSavingsPlansAll) + includeSynapse := shouldIncludeService(*params, common.ServiceDataWarehouse) // Compute (VM) recommendations — subscription-wide. if includeCompute { @@ -187,6 +192,14 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p }) } + // Synapse (SqlDataWarehouse) recommendations — subscription-wide. Search and + // Managed Redis are deliberately not collected here: see #47. + if includeSynapse { + goService(&synapseErr, func() { + synapseRecs, synapseErr = newSynapseClientFn(r.cred, r.subscriptionID, "").GetRecommendations(gctx, params) + }) + } + // Savings Plans — Azure has no stable public API for SP purchase // recommendations (Benefits Recommendations API is still in preview). // The call returns an empty slice so the service appears in the fan-out @@ -205,10 +218,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // // getAdvisorRecsFn defaults to r.getAdvisorRecommendations and may be // replaced per-adapter in tests to avoid real ARM network calls. - advisorFn := r.getAdvisorRecsFn - if advisorFn == nil { - advisorFn = r.getAdvisorRecommendations - } + advisorFn := r.advisorFn() goService(&advisorErr, func() { advisorRecs, advisorErr = collectAdvisorOrSkip(gctx, advisorFn, *params) }) @@ -229,6 +239,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p serviceResult{"database", dbRecs, dbErr, includeDB}, serviceResult{"cache", cacheRecs, cacheErr, includeCache}, serviceResult{"cosmosdb", cosmosRecs, cosmosErr, includeCosmos}, + serviceResult{"synapse", synapseRecs, synapseErr, includeSynapse}, // The savingsplans client is a stub that unconditionally returns // ([], nil) until the Benefits Recommendations API stabilizes (see // services/savingsplans Client.GetRecommendations). Counting its @@ -291,6 +302,14 @@ func dropCoveredAdvisorSavingsPlans(advisor, sp []common.Recommendation) []commo return out } +// advisorFn returns the injected Advisor collector, or the real one. +func (r *RecommendationsClientAdapter) advisorFn() func(context.Context, common.RecommendationParams) ([]common.Recommendation, error) { + if r.getAdvisorRecsFn != nil { + return r.getAdvisorRecsFn + } + return r.getAdvisorRecommendations +} + // serviceResult bundles a per-service collection outcome for the deterministic // merge in mergeServiceResults. Extracted into a helper so GetRecommendations // stays under the cyclomatic-complexity gate after the post-Wait ctx.Err() @@ -310,7 +329,7 @@ type serviceResult struct { // mergeServiceResults logs per-service errors (matches the previous sequential // behavior where each error was logged inline via logging.Warnf) and appends // successful results in the order the slice is passed — callers must preserve -// the canonical compute → database → cache → cosmosdb → savingsplans → advisor +// the canonical compute → database → cache → cosmosdb → synapse → savingsplans → advisor // order so that order-sensitive consumers remain stable. The advisor entry's // error is logged via logging.Errorf to match the pre-parallelisation severity. // diff --git a/providers/azure/recommendations_advisor_test.go b/providers/azure/recommendations_advisor_test.go index e0ffcd5f..f31c191c 100644 --- a/providers/azure/recommendations_advisor_test.go +++ b/providers/azure/recommendations_advisor_test.go @@ -155,6 +155,21 @@ func TestGetRecommendations_AdvisorSavingsPlanDedupedAgainstBenefitRecs(t *testi assert.Len(t, got, 1, "Advisor SP advice is kept when the benefit source returns nothing") } +func TestGetRecommendations_CollectsSynapseForDataWarehouse(t *testing.T) { + overrideServiceClientFns(t, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}) + want := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceDataWarehouse} + newSynapseClientFn = newFakeFn(&fakeServiceClient{recs: []common.Recommendation{want}}) + a := &RecommendationsClientAdapter{cred: &mockAzureTokenCredential{}, subscriptionID: "sub", getAdvisorRecsFn: noopAdvisorFn} + + recs, err := a.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{want}, recs) + + recs, err = a.GetRecommendations(context.Background(), &common.RecommendationParams{Service: common.ServiceCompute}) + require.NoError(t, err) + assert.Empty(t, recs, "synapse is skipped when another service is requested") +} + func TestConvertAdvisor_UnmappedReservedResourceTypeIsIgnoredNotSkipped(t *testing.T) { a := &RecommendationsClientAdapter{subscriptionID: "sub"} ctx, skips := azrecs.WithSkipLog(context.Background()) diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index f2d7ad0c..11dadace 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -207,7 +207,9 @@ func overrideServiceClientFns(t *testing.T, compute, database, cache, cosmos, sp origCache := newCacheClientFn origCosmos := newCosmosDBClientFn origSP := newSavingsPlansClientFn + origSynapse := newSynapseClientFn t.Cleanup(func() { + newSynapseClientFn = origSynapse newComputeClientFn = origCompute newDatabaseClientFn = origDatabase newCacheClientFn = origCache @@ -219,6 +221,7 @@ func overrideServiceClientFns(t *testing.T, compute, database, cache, cosmos, sp newCacheClientFn = newFakeFn(cache) newCosmosDBClientFn = newFakeFn(cosmos) newSavingsPlansClientFn = newFakeFn(sp) + newSynapseClientFn = newFakeFn(&fakeServiceClient{}) } func TestRecommendationsClientAdapter_GetRecommendationsForService(t *testing.T) { @@ -580,6 +583,7 @@ func TestGetRecommendations_Parallelism(t *testing.T) { origDatabase := newDatabaseClientFn origCache := newCacheClientFn origCosmos := newCosmosDBClientFn + origSynapse := newSynapseClientFn origSP := newSavingsPlansClientFn t.Cleanup(func() { newComputeClientFn = origCompute @@ -587,6 +591,7 @@ func TestGetRecommendations_Parallelism(t *testing.T) { newCacheClientFn = origCache newCosmosDBClientFn = origCosmos newSavingsPlansClientFn = origSP + newSynapseClientFn = origSynapse }) rec := func(svc common.ServiceType) common.Recommendation { @@ -599,6 +604,7 @@ func TestGetRecommendations_Parallelism(t *testing.T) { newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceCache)}}) newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceNoSQL)}}) newSavingsPlansClientFn = noopFake + newSynapseClientFn = newFakeFn(&fakeServiceClient{}) adapter := &RecommendationsClientAdapter{ cred: &mockAzureTokenCredential{}, @@ -631,6 +637,7 @@ func TestGetRecommendations_OrderPreservation(t *testing.T) { origDatabase := newDatabaseClientFn origCache := newCacheClientFn origCosmos := newCosmosDBClientFn + origSynapse := newSynapseClientFn origSP := newSavingsPlansClientFn t.Cleanup(func() { newComputeClientFn = origCompute @@ -638,6 +645,7 @@ func TestGetRecommendations_OrderPreservation(t *testing.T) { newCacheClientFn = origCache newCosmosDBClientFn = origCosmos newSavingsPlansClientFn = origSP + newSynapseClientFn = origSynapse }) makeRec := func(svc common.ServiceType) common.Recommendation { @@ -652,6 +660,7 @@ func TestGetRecommendations_OrderPreservation(t *testing.T) { newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: 20 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceCache)}}) newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: 10 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}}) newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{}) + newSynapseClientFn = newFakeFn(&fakeServiceClient{}) adapter := &RecommendationsClientAdapter{ cred: &mockAzureTokenCredential{}, @@ -682,6 +691,7 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { origDatabase := newDatabaseClientFn origCache := newCacheClientFn origCosmos := newCosmosDBClientFn + origSynapse := newSynapseClientFn origSP := newSavingsPlansClientFn t.Cleanup(func() { newComputeClientFn = origCompute @@ -689,6 +699,7 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { newCacheClientFn = origCache newCosmosDBClientFn = origCosmos newSavingsPlansClientFn = origSP + newSynapseClientFn = origSynapse }) makeRec := func(svc common.ServiceType) common.Recommendation { @@ -701,6 +712,7 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceCache)}}) newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}}) newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{}) + newSynapseClientFn = newFakeFn(&fakeServiceClient{}) adapter := &RecommendationsClientAdapter{ cred: &mockAzureTokenCredential{}, From 676b9e842ee5e74d0433d7aada1a85858fcb3c02 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 23:14:50 +0200 Subject: [PATCH 7/7] fix(azure): satisfy lint in the Advisor converter Co-Authored-By: Claude Opus 5.5 --- providers/azure/recommendations.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index ee898e8a..4c9d79e7 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -284,20 +284,20 @@ func withSkips(recs []common.Recommendation, err error, skips *azrecs.SkipLog) ( func dropCoveredAdvisorSavingsPlans(advisor, sp []common.Recommendation) []common.Recommendation { type key struct{ account, term string } covered := make(map[key]bool) - for _, r := range sp { - if r.CommitmentType == common.CommitmentSavingsPlan { - covered[key{r.Account, r.Term}] = true + for i := range sp { + if sp[i].CommitmentType == common.CommitmentSavingsPlan { + covered[key{sp[i].Account, sp[i].Term}] = true } } if len(covered) == 0 { return advisor } out := make([]common.Recommendation, 0, len(advisor)) - for _, r := range advisor { - if r.CommitmentType == common.CommitmentSavingsPlan && covered[key{r.Account, r.Term}] { + for i := range advisor { + if advisor[i].CommitmentType == common.CommitmentSavingsPlan && covered[key{advisor[i].Account, advisor[i].Term}] { continue } - out = append(out, r) + out = append(out, advisor[i]) } return out } @@ -450,7 +450,7 @@ func (r *RecommendationsClientAdapter) collectAdvisorRecommendations(ctx context } page, err := pager.NextPage(ctx) if err != nil { - return recommendations, fmt.Errorf("%w: page error after %d recommendations: %v", errAdvisorTruncated, len(recommendations), err) + return recommendations, fmt.Errorf("%w: page error after %d recommendations: %w", errAdvisorTruncated, len(recommendations), err) } recommendations = r.appendAdvisorPageRecs(ctx, params, page, recommendations) } @@ -529,7 +529,7 @@ const ( // recommendations are commitment purchases; resource-level recommendations // such as resize or shutdown are not, and are ignored (ok=false, no error). // An unmapped reservedResourceType is ignored (debug log), not a skip. -func advisorPurchaseKind(ctx context.Context, ext map[string]*string) (common.CommitmentType, string, bool) { +func advisorPurchaseKind(ext map[string]*string) (common.CommitmentType, string, bool) { service := serviceFromExtendedProperties(ext) if service == "" { if v := ext[advisorKeyRRT]; v != nil && *v != "" { @@ -558,7 +558,7 @@ func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(ctx context. return nil } ext := advisorRec.Properties.ExtendedProperties - commitmentType, service, ok := advisorPurchaseKind(ctx, ext) + commitmentType, service, ok := advisorPurchaseKind(ext) if !ok { return nil }