fix(db/azure): flush stale monthly_cost cache + Azure recurring cost (closes #252)#256
Merged
Merged
Conversation
…gap 1) Pre-PR-#254 rows in the recommendations table store monthly_cost as a literal JSON 0 (old float64 field). After PR #254 changed the field to *float64, reading those rows produces a pointer-to-zero, which the API re-emits as 0 — indistinguishable from a legitimate all-upfront zero. Migration 000046 truncates the recommendations table and resets last_collected_at to NULL, which triggers a synchronous cold-start collect on the next ListRecommendations call. The table is a pure cache (scheduler re-fetches from cloud APIs on every collect), so TRUNCATE has no data-loss risk.
…ons (closes #252) Azure Reservation recommendations are always all-upfront (single payment, no recurring monthly charge). PR #254 added RecurringMonthlyCost to the common.Recommendation struct and wired it through the scheduler but never updated the Azure parsers, so all 16 Azure recs still returned monthly_cost null (renders as "—" after cache refresh) instead of the accurate value 0 ("$0 — no recurring charge"). Changes: - providers/azure/internal/recommendations: add RecurringMonthlyCost *float64 to ExtractedFields with float64Ptr(0) set in both extractLegacy and extractModern. All Azure recs are all-upfront by API convention. - providers/azure/services/{compute,cache,database,cosmosdb}: pass RecurringMonthlyCost: f.RecurringMonthlyCost through to common.Recommendation. - Tests: assert RecurringMonthlyCost is non-nil pointer-to-zero for both Legacy and Modern fixtures in converter_test.go and for the compute converter in client_test.go. GCP CUDs: RecurringMonthlyCost left nil — the GCP Recommender API does not expose commitment cost per period (only savings), so nil ("—") is the accurate frontend rendering.
|
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 (9)
📝 WalkthroughWalkthroughThis PR fixes a bug where monthly cost data was hardcoded to ChangesMonthly Cost Data Cleanup & Azure RecurringMonthlyCost
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
cristim
added
type/bug
…write
Azure recommendation collection alone takes >60s and exceeds the API Lambda
timeout — a forced cold-start collect (triggered by TRUNCATE + resetting
last_collected_at) would cause a 502 on the first request after deploy.
Switch to a targeted in-place payload rewrite:
UPDATE recommendations
SET payload = jsonb_set(payload, '{monthly_cost}', 'null'::jsonb)
WHERE (payload->>'monthly_cost')::text = '0';
This converts stale pre-PR-#254 zeros to null so the frontend renders "—"
(honest: data not yet collected) rather than "$0" (misleading: implies $0
recurring). Existing rows stay in place; the daily scheduler tick writes
correct values on its next run.
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
CodeRabbit review pass 1 — summaryCodeRabbit's first review found zero actionable items. 🎉 Pre-merge warnings (both dismissed — out of scope):
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a new review now. [review] |
6 tasks
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>
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
Follow-up to PR #254 which fixed the Go struct types and AWS parsers but left two gaps that keep the deployed app returning `monthly_cost: 0` for all 33 recs (17 AWS, 16 Azure):
Gap 1 — Stale cache (migration 000046): Pre-#254 rows in the `recommendations` table store `monthly_cost` as a literal JSON `0` (from the old `float64` field). Reading those rows post-deploy produces a pointer-to-zero, which the API re-emits as 0 — indistinguishable from a legitimate $0 recurring charge.
Fix: targeted in-place JSONB rewrite (not TRUNCATE) — sets `monthly_cost = null` for all rows where the current value is 0. This causes the frontend to render "—" (honest: data not yet collected) rather than "$0" (misleading). Existing rows stay intact; the daily scheduled collector will overwrite them with correct values on its next run.
Why not TRUNCATE + cold-start? Azure collection alone exceeds the API Lambda's 60s timeout (#257). A forced cold-start collect on the next request would cause a 502. Filed #257 as a separate perf issue.
Gap 2 — Azure parsers (ExtractedFields + 4 converters): PR #254 never updated the Azure parsers. Azure Reservation recommendations are always all-upfront (single payment, no recurring monthly charge), so `RecurringMonthlyCost` should be a non-nil `*float64` pointing to 0.0 — meaning "no recurring charge" rather than nil ("data not available"). Added `RecurringMonthlyCost *float64` to `ExtractedFields`, set to `float64Ptr(0)` in both `extractLegacy` and `extractModern`, and wired it through all four active service converters (compute, cache, database, cosmosdb).
GCP: Left nil — the GCP Recommender API does not expose commitment cost per period. Frontend renders "—" which is accurate.
Related: #257 (Azure Lambda timeout — separate perf issue, not fixed here)
Test plan
Closes #252
Summary by CodeRabbit
Bug Fixes
New Features