Skip to content

feat(azure/savings-plans): wire SP rec collection into scheduler (closes #593) - #809

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/593-wave5
Jun 6, 2026
Merged

feat(azure/savings-plans): wire SP rec collection into scheduler (closes #593)#809
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/593-wave5

Conversation

@cristim

@cristim cristim commented May 28, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds savingsplans goroutine to the GetRecommendations fan-out in providers/azure/recommendations.go, mirroring the AWS SP slot in providers/aws/recommendations/client.go GetAllRecommendations
  • The SP client's GetRecommendations intentionally returns an empty slice (Azure Benefits Recommendations API is still in preview); the goroutine slot is reserved so the scheduler picks up data the moment the upstream API stabilises, without needing a second wire-up PR
  • Adds TestGetRecommendations_SavingsPlansServiceIncluded to pin that shouldIncludeService lets ServiceSavingsPlans through on both empty-params (all-services) and explicit-service sweeps

Test plan

  • go build ./... passes
  • go test github.com/LeanerCloud/CUDly/providers/azure/... passes (664 tests)
  • New test TestGetRecommendations_SavingsPlansServiceIncluded covers empty-params, explicit-SP, and cross-service cases

Summary by CodeRabbit

Release Notes

  • New Features
    • Added Azure Savings Plans to recommendation retrieval, expanding coverage to six Azure services (Compute, Database, Cache, CosmosDB, Savings Plans, and Advisor).

Add savingsplans goroutine to the GetRecommendations fan-out in
providers/azure/recommendations.go, mirroring the AWS SP wiring in
providers/aws/recommendations/client.go GetAllRecommendations.

The SP client's GetRecommendations intentionally returns an empty
slice (Azure Benefits Recommendations API is still in preview), so
no scheduler change is needed -- the goroutine slot is reserved and
will start surfacing data once the upstream API stabilises.

Also add TestGetRecommendations_SavingsPlansServiceIncluded to pin
that shouldIncludeService passes ServiceSavingsPlans through on both
empty-params (all-services) and explicit-service sweeps.
@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/this-quarter Within the quarter impact/many Affects most users effort/m Days type/feat New capability labels May 28, 2026
@cristim

cristim commented May 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place.

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR extends Azure GetRecommendations to include Savings Plans as a sixth service in the multi-service fan-out pattern. A new goroutine conditionally fetches Savings Plans recommendations when requested, results are merged alongside existing services in deterministic order, and a table-driven test validates the service inclusion logic.

Changes

Savings Plans service integration

Layer / File(s) Summary
Savings Plans fan-out implementation
providers/azure/recommendations.go
Import Savings Plans client; extend concurrency variables to track spRecs/spErr; add conditional goroutine to fetch Savings Plans recommendations; include Savings Plans in deterministic result merge before Advisor; update method comment to document expanded canonical ordering (compute → database → cache → cosmosdb → savingsplans → advisor).
Savings Plans service inclusion test
providers/azure/recommendations_test.go
New table-driven test TestGetRecommendations_SavingsPlansServiceIncluded asserts that shouldIncludeService includes ServiceSavingsPlans for empty params or explicit ServiceSavingsPlans selection, and excludes it when a different service is requested.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • LeanerCloud/CUDly#723: Updates the frontend Savings Plans detection and bucketing to recognize the "savingsplans" slug added by this backend fan-out extension.
  • LeanerCloud/CUDly#592: Introduces the Azure Savings Plans service client and registers common.ServiceSavingsPlans in the provider factory, forming the infrastructure this PR extends.
  • LeanerCloud/CUDly#259: Previous modification to GetRecommendations fan-out and deterministic result merging that established the multi-service concurrency pattern extended here.

Poem

A rabbit hops through recommendation lanes,
Adding Savings Plans to the six-service refrains,
Goroutines fan-out with deterministic grace,
Merging results in canonical place. 🐰💰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(azure/savings-plans): wire SP rec collection into scheduler' directly summarizes the main change: adding Savings Plans recommendation collection to the scheduler fan-out in Azure.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/593-wave5

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim

cristim commented May 30, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 38 minutes and 53 seconds before sending another message.

@cristim

cristim commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Verification note: providers/azure/services/savingsplans/client.go GetRecommendations currently returns a hardcoded empty slice (the Azure Benefits Recommendations API is in preview), so the new scheduler goroutine wires a no-op path - no Azure SP recommendation can reach the user yet, and the test only asserts shouldIncludeService(), not an end-to-end rec. That is fine as a reserved slot, but issue #593 asks for actual SP rec collection plus the commitmentopts SP probe equivalent. Recommend NOT auto-closing #593 on merge (scope this PR to 'reserve the goroutine slot' and keep #593 open for the real implementation when the preview API stabilizes), or split a follow-up issue. Leaving for a human to decide.

@cristim

cristim commented Jun 6, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit 39d63f3 into feat/multicloud-web-frontend Jun 6, 2026
3 of 4 checks passed
@cristim
cristim deleted the fix/593-wave5 branch July 27, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/feat New capability urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant