feat(ladder): fail-loud stale/insufficient baseline series detection (L4) - #1365
Merged
Conversation
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesDaily baseline validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OnDemandSeriesSource
participant AWSLadder.GetUsageBaseline
participant validateInWindowCoverage
participant validateSeriesFreshness
participant extractValues
OnDemandSeriesSource->>AWSLadder.GetUsageBaseline: return []DailyPoint
AWSLadder.GetUsageBaseline->>validateInWindowCoverage: validate UTC-day coverage
validateInWindowCoverage-->>AWSLadder.GetUsageBaseline: coverage result
AWSLadder.GetUsageBaseline->>validateSeriesFreshness: validate chronology and age
validateSeriesFreshness-->>AWSLadder.GetUsageBaseline: freshness result
AWSLadder.GetUsageBaseline->>extractValues: extract USDPerHour values
extractValues-->>AWSLadder.GetUsageBaseline: []float64
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ladder/baseline.go`:
- Around line 154-160: The freshness validation around latestDate in
providers/aws/ladder/baseline.go lines 154-160 must normalize the latest data
point and today to UTC midnight, reject future latest dates, and compute ageDays
from their calendar-day difference instead of elapsed hours. Update the boundary
test in providers/aws/ladder/ladder_test.go lines 873-887 to construct the test
date from UTC midnight using AddDate(0, 0, -maxSeriesAgeDays), eliminating time
dependence.
- Around line 94-102: Update the coverage check in GetUsageBaseline to count
only unique points whose dates fall within the requested lookback window, or
validate the covered calendar span directly before applying minRequired. Ensure
points outside the window cannot satisfy the lookback coverage threshold while
preserving the existing error behavior and message context.
In `@providers/aws/ladder/ladder_test.go`:
- Around line 873-887: The TestGetUsageBaseline_AgeExactlyAtBoundary_NoError
setup currently depends on the current UTC hour when calculating end. Construct
end from the expected UTC calendar date directly, offset by maxSeriesAgeDays
from today, so the generated series is exactly the boundary age regardless of
when the test runs; leave the assertions and point generation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 773944d5-4fa1-4986-9fdc-44d59323d471
📒 Files selected for processing (3)
providers/aws/ladder/baseline.goproviders/aws/ladder/interfaces.goproviders/aws/ladder/ladder_test.go
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Address CodeRabbit review on the stale-series guard (PR #1365): 1. Coverage check counted TOTAL points, so a series with enough points smeared across more than lookbackDays days (e.g. 27 points over 53 days) passed despite sparse in-window coverage. New validateInWindowCoverage counts only points whose date falls within [today-lookbackDays, today] and requires that count >= lookbackDays - maxMissingDays; fail-loud otherwise. 2. Freshness derived "days old" from elapsed hours (floor(hours/24)), making the result time-of-day dependent and contradicting the calendar-day contract. Age is now the whole-UTC-calendar-day difference between today and the latest point, both normalized via the new utcDay helper. today is computed once in GetUsageBaseline and shared by both the coverage window and the freshness check for consistent date math. 3. Test hardening: the freshness/stale/recent date helpers built dates via hour arithmetic before truncation, which selected the wrong calendar day between 00:00-00:59 UTC and intermittently failed. All date-based test fixtures now build UTC calendar days directly. New regression test: TestGetUsageBaseline_InsufficientInWindowCoverage_ReturnsError -- 27 total points, only 20 in-window -> error (fails on the old total-count check, passes now).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Introduce two named-constant guards in GetUsageBaseline that fail loud
when the CE on-demand series cannot be trusted:
maxMissingDays = 3 -- coverage check: series must have >= lookbackDays-3
points (CE lags ~24-48 h; up to 3 absent days ok)
maxSeriesAgeDays = 3 -- freshness check: most-recent DailyPoint must be
within 3 calendar days of now; older means the feed
stalled or the date range is wrong
Both conditions return an explicit error (not a zeroed/nil baseline) so
the engine config is counted Errored and surfaced to the operator rather
than silently trusting a stale low-water clamp.
Interface change: onDemandSeriesSource.GetOnDemandSeries now returns
[]DailyPoint{Date, USDPerHour} instead of []float64, giving
GetUsageBaseline the Date field it needs for the freshness check. L2
(real CE wiring) will implement this updated signature. Float64 values
are extracted via extractValues() before passing to the unchanged
validateSeries / nearestRankPercentile helpers.
Regression tests added:
TestGetUsageBaseline_StaleSeries_ReturnsError -- tail > maxSeriesAgeDays old
TestGetUsageBaseline_SparseSeries_ReturnsError -- too few points for lookback
TestGetUsageBaseline_FreshAndCompleteSeries_NoError -- happy path unbroken
…s check Fable review hardening for the stale-series guard. validateSeriesFreshness trusted the documented oldest-to-newest ordering by reading the last element as the newest point. A future L2 CE adapter returning unsorted or duplicate-date pages could make a stale tail pass (fresh point last) or a fresh series falsely error. Add validateSeriesChronology: reject any non-monotonic or duplicate calendar date (compared after UTC-day truncation) with an explicit error naming the offending index and dates. Runs before the age check, so the last element is provably the latest. It also catches duplicate-date rows the count-based coverage check cannot see (N points spanning < N days). Boundary tests lock both off-by-ones: TestGetUsageBaseline_AgeExactlyAtBoundary_NoError -- age == max passes TestGetUsageBaseline_CoverageExactlyAtBoundary_NoError -- 27 points pass TestGetUsageBaseline_NonMonotonicDates_ReturnsError -- swap + dup date
Address CodeRabbit review on the stale-series guard (PR #1365): 1. Coverage check counted TOTAL points, so a series with enough points smeared across more than lookbackDays days (e.g. 27 points over 53 days) passed despite sparse in-window coverage. New validateInWindowCoverage counts only points whose date falls within [today-lookbackDays, today] and requires that count >= lookbackDays - maxMissingDays; fail-loud otherwise. 2. Freshness derived "days old" from elapsed hours (floor(hours/24)), making the result time-of-day dependent and contradicting the calendar-day contract. Age is now the whole-UTC-calendar-day difference between today and the latest point, both normalized via the new utcDay helper. today is computed once in GetUsageBaseline and shared by both the coverage window and the freshness check for consistent date math. 3. Test hardening: the freshness/stale/recent date helpers built dates via hour arithmetic before truncation, which selected the wrong calendar day between 00:00-00:59 UTC and intermittently failed. All date-based test fixtures now build UTC calendar days directly. New regression test: TestGetUsageBaseline_InsufficientInWindowCoverage_ReturnsError -- 27 total points, only 20 in-window -> error (fails on the old total-count check, passes now).
PR #1362 merged to main with factory.go's noopOnDemandSeriesSource returning []float64, while this branch changed the onDemandSeriesSource interface to return []DailyPoint (Date-carrying points for the stale-series freshness check). Merging without this alignment would break main despite no textual conflict (semantic conflict). Update the noop's return type to []DailyPoint. Behavior is unchanged: it still fails loud with the same not-yet-wired error, so plan runs against the unwired factory continue to be counted Errored.
cristim
force-pushed
the
feat/ladder-stale-series
branch
from
July 16, 2026 13:01
d9d265e to
4aff31f
Compare
Member
Author
|
Rebased onto main (7adacd7, post-#1362) and aligned the newly-merged factory.go noop GetOnDemandSeries to the []DailyPoint signature this PR introduces (fail-loud behavior unchanged; audit confirmed it was the only implementer needing alignment). Zero textual conflicts; full gates green on the rebased branch (build/vet 0, providers/aws ladder 122, internal/server 401, gocyclo clean). New head 4aff31f. This resolves the semantic conflict that would have broken main had #1365 merged un-rebased. |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
Merged to main after: Fable review (SHIP + hardening round), 3 CodeRabbit findings fixed (in-window coverage counting, UTC calendar-day freshness, hour-independent boundary tests), rebase onto post-#1362 main with the factory.go noop aligned to []DailyPoint (resolving the semantic conflict pre-merge), CR-clean re-review on the rebased head, and full functional CI green. The baseline now fails loud on stale, sparse, non-monotonic, or insufficient in-window Cost Explorer data. Next in the ladder chain: L2 (real CE read adapters into the factory) is in implementation against this []DailyPoint interface. |
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.
What
Work item L4 of the auto-laddering full-automation plan (closes gap G6a). Adds fail-loud stale/insufficient-series detection to the AWS ladder usage baseline so untrustworthy Cost Explorer data cannot silently drive a commitment purchase.
Today
GetUsageBaselinecomputes a low-water baseline from a CE time series with no freshness or completeness guard: a stale or sparse series silently yields a baseline that the utilization clamp then trusts. This adds three guards, all returning an explicit error (never a degraded/nil baseline), so the config is countedErroredand surfaced rather than silently held or over-committed.Guards (named constants, no magic numbers)
maxSeriesAgeDays = 3): errors if the latest point is more than 3 UTC calendar days old.maxMissingDays = 3): errors if the series has fewer thanlookbackDays - 3points.validateSeriesChronologyrequires strictly-increasing calendar dates, failing loud on non-monotonic or duplicate-date input, so the last element is provably the latest and duplicate rows the count-based check can't see are rejected.minBaselineSeriesDays = 7absolute floor still fires first for very short series.Scope
Only the freshness/completeness/chronology guard is added; the daily-average percentile math is unchanged (the hourly-granularity
baseline_granularityenum is a separate item, L7). TheonDemandSeriesSource.GetOnDemandSeriesinterface return type changes[]float64->[]DailyPoint{Date, USDPerHour}so freshness can be checked against real dates.Verification (exit 0)
go build ./...,go vet ./...clean (root + providers/aws).go test ./ladder/...121 pass,gocyclo -over 10clean.LowWaterUSDPerHouris a benign Hold inpkg/ladder, while an error fromGetUsageBaselinepropagates asErrored— the guards return errors, so untrustworthy data is never silently held.Coordination note
The
GetOnDemandSeriessignature change ([]float64 -> []DailyPoint) touchesonDemandSeriesSource. PR #1362 (feat/ladder-run-scheduler) addsproviders/aws/ladder/factory.gowith a noop implementation of this interface; whichever of the two merges second will need a trivial update tofactory.go's noop to return[]DailyPoint. Item L2 wires the real Cost-Explorer-backed adapter against this signature.Part of #1336. Tracker #1333.
Summary by CodeRabbit