fix(recommendations/aws): flush stale on_demand_cost cache + parser visibility (closes #321)#324
Merged
cristim merged 3 commits intoMay 6, 2026
Conversation
… 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
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.
📝 WalkthroughWalkthroughAdds a Postgres migration that nulls ChangesAWS Cache Invalidation & Parser Observability
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sql (1)
1-5: Consider adding a no-op statement for consistency with similar intentional no-op migrations.The rationale for not rolling back is sound and matches the pattern in
000046_invalidate_monthly_cost_cache.down.sql, which has identical intent. While golang-migrate accepts comment-only down migrations (as000046demonstrates), migrations marked as deliberate no-ops in this codebase includeSELECT 1;for explicitness (see000006_ensure_admin_user.down.sql). Adding it here would improve consistency:-- No rollback for the payload rewrite: the original AWS rows lacking -- on_demand_cost were themselves stale (pre-PR-#312 data). Reverting -- monthly_cost from null back to those stale values would re-introduce -- the very bug this migration fixes. The correct values will be written -- on the next scheduled collection. SELECT 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sql` around lines 1 - 5, Add an explicit no-op SQL statement to the down migration to match the repository pattern: in the migration 000049_invalidate_aws_recs_missing_on_demand_cost.down.sql, append a deliberate no-op (e.g., SELECT 1;) after the existing comment so the file is not comment-only and is consistent with other intentional no-op migrations like 000046_invalidate_monthly_cost_cache.down.sql and 000006_ensure_admin_user.down.sql.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/aws/recommendations/parser_ri.go`:
- Around line 130-136: Replace the fmt.Printf call used when
EstimatedMonthlyOnDemandCost is nil in the RI recommendation handling with
log.Printf to match parser_sp.go behavior and ensure stderr/timestamped logging;
update the warning call that references rec.Service and rec.Account to use
log.Printf, and add "log" to the file imports if it's not already imported
(target the nil-check block that currently calls fmt.Printf in parser_ri.go).
---
Nitpick comments:
In
`@internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sql`:
- Around line 1-5: Add an explicit no-op SQL statement to the down migration to
match the repository pattern: in the migration
000049_invalidate_aws_recs_missing_on_demand_cost.down.sql, append a deliberate
no-op (e.g., SELECT 1;) after the existing comment so the file is not
comment-only and is consistent with other intentional no-op migrations like
000046_invalidate_monthly_cost_cache.down.sql and
000006_ensure_admin_user.down.sql.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21d26fce-661e-4dba-9ec1-e77854046463
📒 Files selected for processing (4)
internal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.down.sqlinternal/database/postgres/migrations/000049_invalidate_aws_recs_missing_on_demand_cost.up.sqlproviders/aws/recommendations/parser_ri.goproviders/aws/recommendations/parser_sp.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/aws/recommendations/parser_ri.go (1)
127-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing warning coverage for zero/parse-failure still hides fallback cases.
On Line 127 onward, warnings are emitted only when the pointer is nil. If parsing fails (non-nil but invalid string) or parses to
0,rec.OnDemandCoststill effectively drives fallback behavior without visibility.Proposed patch
if details.EstimatedMonthlyOnDemandCost != nil { - if onDemand, err := strconv.ParseFloat(*details.EstimatedMonthlyOnDemandCost, 64); err == nil { + if onDemand, err := strconv.ParseFloat(*details.EstimatedMonthlyOnDemandCost, 64); err == nil { rec.OnDemandCost = onDemand + if onDemand == 0 { + log.Printf("WARNING: EstimatedMonthlyOnDemandCost is 0 for RI recommendation (service=%s, account=%s) — Effective %% may use reconstruction fallback", rec.Service, rec.Account) + } + } else { + log.Printf("WARNING: failed to parse EstimatedMonthlyOnDemandCost=%q for RI recommendation (service=%s, account=%s): %v", *details.EstimatedMonthlyOnDemandCost, rec.Service, rec.Account, err) } } else { // EstimatedMonthlyOnDemandCost absent from AWS CE response — OnDemandCost // will be 0 and the scheduler's nonZeroPtr will store nil, causing the // frontend to fall back to the reconstruction formula. Log so operators🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/recommendations/parser_ri.go` around lines 127 - 137, The current logic only logs when details.EstimatedMonthlyOnDemandCost is nil, but it silently ignores parse errors and zero values which also trigger the reconstruction fallback; update the block around details.EstimatedMonthlyOnDemandCost/strconv.ParseFloat so that you log a warning (including the raw string and parse error if any) when parsing fails or when the parsed onDemand == 0, and only suppress the warning when parsing succeeds and onDemand > 0; reference the existing symbols rec.OnDemandCost, details.EstimatedMonthlyOnDemandCost, and strconv.ParseFloat so the change is applied in that parsing branch and include context (rec.Service, rec.Account) in the log message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@providers/aws/recommendations/parser_ri.go`:
- Around line 127-137: The current logic only logs when
details.EstimatedMonthlyOnDemandCost is nil, but it silently ignores parse
errors and zero values which also trigger the reconstruction fallback; update
the block around details.EstimatedMonthlyOnDemandCost/strconv.ParseFloat so that
you log a warning (including the raw string and parse error if any) when parsing
fails or when the parsed onDemand == 0, and only suppress the warning when
parsing succeeds and onDemand > 0; reference the existing symbols
rec.OnDemandCost, details.EstimatedMonthlyOnDemandCost, and strconv.ParseFloat
so the change is applied in that parsing branch and include context
(rec.Service, rec.Account) in the log message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff2c3328-a8fe-4173-8d69-b06f775366d6
📒 Files selected for processing (1)
providers/aws/recommendations/parser_ri.go
This was referenced May 6, 2026
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
on_demand_costin their JSONB payload. Setsmonthly_cost = nullfor those rows so the frontend renders—until the next scheduler tick repopulates with correct values. Mirrors fix(db/azure): flush stale monthly_cost cache + Azure recurring cost (closes #252) #256's pattern for Azure (000046_invalidate_monthly_cost_cache).parseAWSCostDetails(parser_ri.go) andparseSavingsPlanDetail(parser_sp.go) when the AWS CE API fields that feedOnDemandCostare absent — so operators can detect the reconstruction fallback path in production logs.Closes
Closes #321
Follow-up deferred
The optional frontend change (
effectiveSavingsPctreturning null on AWS reconstruction fallback) has been deferred to #323. Decision should wait until production data confirms the migration succeeded andon_demand_costis repopulated. This avoids conflicts with concurrent agents modifyingfrontend/src/recommendations.tsfor issues #317/#318/#319/#320.Runbook note
Deploy order:
migrate.go).on_demand_cost.Diagnosis SQL (from issue #321):
After migration: rows lacking
on_demand_costwill havemonthly_cost = null.After next scheduler tick: all AWS rows should have
on_demand_costpopulated.Summary by CodeRabbit