fix(azure): derive covered monthly cost instead of hardcoding 0 (GUI showed on-demand) - #1105
Merged
cristim merged 1 commit intoJun 8, 2026
Conversation
extractLegacy and extractModern in the Azure reservation recommendation converter hardcoded out.RecurringMonthlyCost = float64Ptr(0). That field is the covered/effective spend the GUI renders; with it pinned to 0 the GUI fell back to displaying OnDemandCost (the spend WITHOUT any reservation), so Azure recommendations showed on-demand cost instead of the committed cost. Derive the value from the payload via a shared deriveCoveredMonthlyCost helper: prefer the provider-reported TotalCostWithReservedInstances; reconstruct as OnDemandCost - NetSavings when the total is absent; return nil (never a fabricated 0) and log a warning when neither source is available, so an unavailable figure renders as data-not-available rather than a false free recurring charge. amountValuePtr unwraps Modern *Amount while preserving the absent-field nil signal so both shapes share the same logic. Regression tests assert the covered cost (not 0) for both Legacy and Modern shapes, the reconstruction path, and the nil-when-absent path; all six fail against the pre-fix hardcoded-0 code. Downstream compute/managedredis/synapse client tests are updated to expect the covered cost. Closes #1101
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR fixes hardcoded zero values in Azure reservation recommendation cost calculations. The ChangesAzure RecurringMonthlyCost Derivation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review Generated by Claude Code |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
cristim
added a commit
that referenced
this pull request
Jun 9, 2026
…1126) The Home dashboard showed a non-zero "current / committed" figure for Azure (e.g. $166) while the Coverage tab reported $0 / "No usage detected" for the same subscription. The two surfaces read different fields of the same active commitments, and the Coverage path silently dropped Azure rows. Root cause: getCoverageBreakdown summed only the recurring MonthlyCost and guarded it with `if p.MonthlyCost != nil`. Azure all-upfront RIs carry MonthlyCost == nil (no recurring charge at the commitment layer, documented on config.PurchaseHistoryRecord.MonthlyCost), so every such row was skipped and the provider collapsed to nil Services / nil OverallCoveragePct. The dashboard, by contrast, aggregates EstimatedSavings (always populated), so it still counted the commitment. PR #1105 only fixed the recommendation converter's RecurringMonthlyCost for future rows; it never touched the Coverage handler, so the divergence persisted. Fix: derive the effective covered monthly spend via a shared commitmentCoveredMonthly helper = recurring MonthlyCost (when present) + amortised upfront (UpfrontCost / (Term * MonthsPerYear)), matching the canonical effective-monthly formula already used by analytics.Collector and exchange_lookup. An all-upfront commitment now contributes its amortised upfront instead of being silently dropped, so Coverage reflects the same active commitments the dashboard does. A nil MonthlyCost is treated as a real $0 recurring component (not a fabricated total), and Term <= 0 is guarded against division by zero, mirroring the collector's skip-bad-term defence. Regression test TestHandler_getCoverageBreakdown_AzureAllUpfrontConsistency seeds an active Azure compute all-upfront RI (MonthlyCost nil, $1200 upfront / 1y = $100/mo) and asserts both that the dashboard primitive counts it AND that the Coverage tab now reports $100/mo covered (100% coverage) instead of nil. It fails pre-fix (azure.Services nil) and passes post-fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
providers/azure/internal/recommendations/converter.gohardcodedout.RecurringMonthlyCost = float64Ptr(0)in both extraction paths(
extractLegacy,extractModern).RecurringMonthlyCostis the field thefrontend renders as the covered/effective spend (what you pay WITH the
reservation). With it pinned to 0, the GUI fell back to displaying
OnDemandCost(CostWithNoReservedInstances, the spend WITHOUT anyreservation), so Azure reservation recommendations showed the on-demand spend
instead of the committed cost. The covered total was already in the payload as
TotalCostWithReservedInstancesbut was never propagated.Fix
Derive
RecurringMonthlyCostfrom the real data via a sharedderiveCoveredMonthlyCosthelper, used identically by both paths:TotalCostWithReservedInstances(authoritativecovered cost).
OnDemandCost - NetSavings(covered = on-demandminus net savings).
nil(renders as data-not-available)and log a warning. Never fabricate a 0 (which would falsely claim a free
recurring charge).
amountValuePtrunwraps Modern's*Amountto a*float64while preservingthe absent-field nil signal, so the Modern path feeds the same logic as
Legacy's bare
*float64inputs.OnDemandCost/EstimatedSavingsextractionis unchanged. The
ExpandPaymentVariantsall-upfront/monthly cashflow split isuntouched (separate, intentional mechanism). The figure is treated as a
lookback approximately monthly run-rate, consistent with the other cost fields.
Verification
converter_test.goassert the covered cost (not 0)for both Legacy and Modern shapes, the reconstruction path, and the
nil-when-absent path. All six fail against the pre-fix hardcoded-0 code
(confirmed: covered tests got 0 instead of 70/260, reconstruction got 0
instead of 75/320, nil tests got a 0-pointer instead of nil) and pass
after the fix.
compute/managedredis/synapseclient tests updated toexpect the covered cost.
go build ./...,go vet, andgo testfor the touched packages: green(197 tests pass across 4 packages). Changed files gofmt-clean.
Closes #1101
Related: #1022 (GCP converter sibling), #1021 (azure)
Summary by CodeRabbit