fix(recommendations/aws): plumb on_demand_cost for canonical Effective Savings % (closes #303)#312
Merged
cristim merged 1 commit intoMay 5, 2026
Conversation
closes #303) The AWS Savings Plans parser did not populate common.Recommendation.OnDemandCost, so effectiveSavingsPct() fell back to reconstructing the on-demand denominator from monthly_cost + savings + amortized. For SP rows monthly_cost reflects only the recurring commitment charge, not the full on-demand baseline, so the reconstruction is inaccurate. Fix: derive OnDemandCost from CurrentAverageHourlyOnDemandSpend × 730 in parseSavingsPlanDetail — the same field AWS Cost Explorer uses internally when computing EstimatedSavingsPercentage. The scheduler's existing nonZeroPtr helper (added in #277) converts 0 → nil so the frontend falls back to reconstruction when the field is absent, preserving backward compatibility. The AWS RI parser already populated OnDemandCost (from EstimatedMonthlyOnDemandCost via parseAWSCostDetails) and the scheduler already plumbs it through to RecommendationRecord — both landed in #277. This commit closes the SP gap and adds the AWS-specific frontend and provider regression tests the AC list requires.
|
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 (3)
📝 WalkthroughWalkthroughThis PR plumbs the AWS provider-supplied ChangesAWS On-Demand Cost Plumbing
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 |
cristim
added
priority/p1
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
cristim
added a commit
that referenced
this pull request
May 6, 2026
…isibility (closes #321) (#324) * fix(db/aws): flush stale recs cache missing on_demand_cost (migration 000049) PR #312 added on_demand_cost population for AWS SP rows but shipped no cache-invalidation migration. Pre-#312 AWS rows in recommendations.payload lack the on_demand_cost key, so the frontend falls back to the broken reconstruction formula (which double-counts amortization), rendering implausibly high Effective % values (e.g. ~86% instead of ~22%). Migration 000049 follows #256's pattern: set monthly_cost to null on AWS rows that lack on_demand_cost, so the frontend renders "—" until the next scheduler tick repopulates with correct values. The WHERE clause is scoped strictly to AWS rows missing the field (idempotent on re-run). Closes #321 * fix(recommendations/aws): warn-log when on-demand cost inputs are nil Add structured warn-logs in the RI and SP parsers when the AWS CE API fields that populate OnDemandCost are absent from the response: - parseAWSCostDetails (parser_ri.go): logs when EstimatedMonthlyOnDemandCost is nil, which causes OnDemandCost=0 → stored as nil by the scheduler's nonZeroPtr helper → frontend reconstruction fallback. - parseSavingsPlanDetail (parser_sp.go): logs when CurrentAverageHourlyOnDemandSpend is nil, same downstream effect. Both logs follow the respective file's existing logging style (fmt.Printf for parser_ri.go, log.Printf for parser_sp.go). The logs make it observable when the fallback path is taken, aiding diagnosis for issue #321 and future regression detection. * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
cristim
added a commit
that referenced
this pull request
May 11, 2026
…irectly (closes #330) (#331) * feat(frontend/recs): On-Demand Monthly column uses provider on_demand_cost directly (closes #330) The On-Demand Monthly column now returns the provider-supplied `r.on_demand_cost` directly instead of reconstructing the value from `monthly_cost + savings + (upfront_cost / months_in_term)`. When `on_demand_cost` is missing, undefined, or `0`, the cell renders the existing em-dash sentinel. PR #322 originally scoped the column to a *reconstruction-only* helper ("the column's purpose is to display the reconstructed denominator so users can verify the formula against the raw fields visible in the same row"). In practice, post-#277/#312/#324 every supported provider (AWS, Azure, GCP) plumbs `on_demand_cost` through to the frontend, and `effectiveSavingsPct` already prefers the provider value when present (its `hasOnDemand` branch). The two columns therefore disagreed for any row where the formula reconstruction drifted from the provider's billed price (Azure all-upfront RIs, AWS Capacity Reservation discounts, partial-day proration, list-price rounding). The product decision: surface the **authoritative** number from the provider, not a teaching-tool reconstruction. Users who want to verify the formula can still compute `monthly_cost + savings + upfront_cost/(term*12)` from the row's other columns. Aligning this column with `effectiveSavingsPct`'s denominator means the two never disagree. Changes: * `frontend/src/recommendations.ts::onDemandMonthly` simplified to: `return r.on_demand_cost > 0 ? r.on_demand_cost : null` — drops the `monthly_cost`/`term`/`upfront_cost` arithmetic entirely. * Updated the function's doc comment to reflect the new contract (the prior comment explicitly said "does NOT use on_demand_cost" and is now exactly inverted). * Updated the `SORTABLE_NUMERIC_COLUMNS.on_demand_monthly` preamble — the null-rendering condition is now "missing on_demand_cost" rather than "null monthly_cost / term=0". Tests: * Replaced the seven reconstruction-arithmetic tests in the `describe('onDemandMonthly')` block with five tests covering the new contract: provider value used directly, null/undefined/0 return null, supporting fields ignored when on_demand_cost is positive. * Updated three column-rendering tests in `describe('Monthly Cost + Effective % column rendering')` to set `on_demand_cost` on the fixture so the rendered value matches the new logic — the em-dash and filter regression tests still cover the same semantics, just with clearer fixture intent. `effectiveSavingsPct` keeps its existing reconstruction fallback unchanged — it's the canonical Effective Savings % calculation and must stay backward-compatible with legacy cached rows that pre-date #312/#324. Verification: - `npx tsc --noEmit` clean - `npm test` clean (1578 passed / 0 failed across 42 suites) * fix(frontend/recs): cover explicit-undefined + freshen filter comment (CR pass on PR #331) Addresses both items from CodeRabbit review 4244506117 on PR #331: 1. **Actionable** — `frontend/src/__tests__/recommendations.test.ts`: the single "undefined / missing" test only exercised the delete-the-property branch. TypeScript callers can also pass `on_demand_cost: undefined` explicitly (strict-null safety pattern), and the type system distinguishes "field absent" from "field present with value undefined" — so a regression that uses `'on_demand_cost' in r` instead of `r.on_demand_cost > 0` would be caught by one but not the other. Split into two tests: one for explicit undefined, one for the deleted-property case, with comments explaining why both matter. 2. **Nitpick** — `frontend/src/recommendations.ts:1085-1088`: the parenthetical "(null monthly_cost or term=0)" described the pre-#330 reconstruction's null conditions. Refresh to match the new contract: "missing or zero on_demand_cost — see onDemandMonthly() for the contract". Adds a back-reference so a future reader can find the source of truth. Verification: - `npx tsc --noEmit` clean - `npm test` clean (1579 passed / 0 failed across 42 suites — +1 test)
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.
Summary
OnDemandCostfor AWS Savings Plans recommendations by computing it fromCurrentAverageHourlyOnDemandSpend × 730inparseSavingsPlanDetail— the same field AWS Cost Explorer uses internally forEstimatedSavingsPercentage.parseAWSCostDetails → EstimatedMonthlyOnDemandCost) and the scheduler already forwarded it (both landed in fix(recommendations): plumb provider on_demand_cost through to frontend (closes #274) #277); this PR closes the SP gap.TestParseSavingsPlanDetail_OnDemandCostinparser_sp_additional_test.go(SP provider regression test, AC Fix AWS RI purchase: details assertion + reservation ID rules #4).on_demand_cost preference (#274)describe block (AC AWS + Azure: add read-only sanity checks + AWS RI exchange #3).Acceptance criteria check
on_demand_costfrom the provider all the way toRecommendationRecordand the API payload — RI: existingparseAWSCostDetails; SP: this PR.effectiveSavingsPct()resolves to the same value as the underlying provider report for AWS RIs and Savings Plans — formula unchanged from bug(frontend/recs): Effective % shows implausible values (85.9% on Azure 1y RI, 54% on savings=$0 row) #274; this PR ensures the provider-supplied denominator reaches the frontend for SP.frontend/src/__tests__/recommendations.test.tspinning the expected effective % for at least one AWS RI fixture and one SP fixture — added in this PR.OnDemandCostis populated —TestParseRecommendationDetail_WithAccountAndCosts(RI, pre-existing) +TestParseSavingsPlanDetail_OnDemandCost(SP, this PR).Reference
Pattern mirrors PR #277 (commit
66a993fa5) which fixed the same bug for Azure 1y RIs.Closes #303
Summary by CodeRabbit
Bug Fixes
Tests