make accumulate be able to govern windows - #3758
Conversation
There was a problem hiding this comment.
Pull request overview
Enhances allocation “accumulate/accumulateBy” behavior so accumulation can govern step sizing and query window rounding (e.g., calendar-week/month boundaries), aiming to keep existing behavior working while enabling more intuitive time-bucketed results.
Changes:
- Add helpers to resolve accumulation option, derive an appropriate default step, and round the query window to accumulation boundaries.
- Update allocation query paths (handler + costmodel) to use the rounded “query window” for fetching data, then trim back to the originally requested window after accumulation.
- Add unit tests covering option/step/window resolution and trimming behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/costmodel/costmodel.go | Queries allocations over a resolved/rounded query window for accumulation modes and trims results back to the requested window. |
| pkg/costmodel/aggregation.go | Adds accumulate/step/window resolution helpers; updates HTTP handlers to use them and to trim after accumulation. |
| pkg/costmodel/aggregation_test.go | Adds unit tests for the new resolution + trimming logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Accumulate is an optional parameter, defaulting to false, which if true | ||
| // sums each Set in the Range, producing one Set. | ||
| accumulate := qp.GetBool("accumulate", false) | ||
| accumulate := opencost.ParseAccumulate(qp.Get("accumulate", "")) |
There was a problem hiding this comment.
opencost.ParseAccumulate does not preserve the previous qp.GetBool("accumulate", false) behavior: e.g. query values like "1", "t", "TRUE" previously parsed as true via strconv.ParseBool, but now resolve to AccumulateOptionNone and disable accumulation. To maintain backwards compatibility, consider falling back to qp.GetBool("accumulate", false) when ParseAccumulate returns None but the raw param is truthy, or extend parsing to accept strconv.ParseBool-style values.
| accumulate := opencost.ParseAccumulate(qp.Get("accumulate", "")) | |
| rawAccumulate := qp.Get("accumulate", "") | |
| accumulate := opencost.ParseAccumulate(rawAccumulate) | |
| if accumulate == opencost.AccumulateOptionNone && qp.GetBool("accumulate", false) { | |
| accumulate = opencost.AccumulateOptionAll | |
| } |
| // Accumulate is an optional parameter, defaulting to false, which if true | ||
| // sums each Set in the Range, producing one Set. | ||
| accumulate := qp.GetBool("accumulate", false) | ||
| accumulate := opencost.ParseAccumulate(qp.Get("accumulate", "")) | ||
|
|
||
| // Accumulate is an optional parameter that accumulates an AllocationSetRange | ||
| // by the resolution of the given time duration. | ||
| // Defaults to 0. If a value is not passed then the parameter is not used. | ||
| accumulateBy := opencost.AccumulateOption(qp.Get("accumulateBy", "")) | ||
|
|
||
| // if accumulateBy is not explicitly set, and accumulate is true, ensure result is accumulated | ||
| if accumulateBy == opencost.AccumulateOptionNone && accumulate { | ||
| accumulateBy = opencost.AccumulateOptionAll | ||
| accumulateBy := resolveAccumulateOption(accumulate, qp.Get("accumulateBy", "")) | ||
| step, err := resolveStepFromQuery(qp, window, accumulateBy) |
There was a problem hiding this comment.
Same backward-compatibility issue as in the summary handler: switching from qp.GetBool("accumulate", false) to opencost.ParseAccumulate drops support for truthy values like "1"/"t"/"TRUE" (accepted by strconv.ParseBool). This can silently change behavior for existing clients relying on those forms.
| func resolveAccumulateOption(accumulate opencost.AccumulateOption, accumulateBy string) opencost.AccumulateOption { | ||
| accumulateByOpt := opencost.AccumulateOption(accumulateBy) | ||
| if accumulateByOpt != opencost.AccumulateOptionNone { | ||
| return accumulateByOpt | ||
| } | ||
|
|
||
| return accumulate | ||
| } |
There was a problem hiding this comment.
resolveAccumulateOption treats any non-empty accumulateBy string as valid, including values like "none" or different casing (e.g. "Week"), which will later cause AllocationSetRange.Accumulate/Window.GetAccumulateWindows to error and can surface as a 500. Consider normalizing case and validating accumulateBy against the supported options (e.g. via opencost.ParseAccumulate plus an explicit invalid-value check) and returning a 400 for invalid inputs.
| func trimAllocationSetRangeToRequestWindow(asr *opencost.AllocationSetRange, requestWindow opencost.Window) *opencost.AllocationSetRange { | ||
| if asr == nil { | ||
| return nil | ||
| } | ||
|
|
||
| trimmed := opencost.NewAllocationSetRange() | ||
| for _, as := range asr.Slice() { | ||
| // Keep only sets that overlap the originally requested window. | ||
| if as.Start().Before(*requestWindow.End()) && as.End().After(*requestWindow.Start()) { | ||
| trimmed.Append(as) | ||
| } | ||
| } | ||
|
|
||
| return trimmed |
There was a problem hiding this comment.
trimAllocationSetRangeToRequestWindow iterates via asr.Slice(), which deep-clones every AllocationSet in the range (and all contained allocations) before filtering. For large allocation responses this is a significant and unnecessary CPU/memory cost just to drop non-overlapping sets. Consider iterating asr.Allocations directly (or filtering in-place) and preserve asr.FromStore on the returned range if that field is meaningful to callers.
There was a problem hiding this comment.
@ameijer Copilot got one right I think -- unless there is any reason to preserve the original asr *opencost.AllocationSetRange I think just iterating over asr.Allocations is better -- Slice() seems to be quite the footgun 😨
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Given the stated goal of backward compatibility, add test coverage for the legacy truthy forms of the accumulate query param (e.g. "1" or "t") to ensure they still enable accumulation after switching to ParseAccumulate (or whatever compatibility fix is applied).
| func TestParseAccumulate_LegacyTruthyValues(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| input string | |
| }{ | |
| { | |
| name: "true remains supported", | |
| input: "true", | |
| }, | |
| { | |
| name: "1 remains supported for backward compatibility", | |
| input: "1", | |
| }, | |
| { | |
| name: "t remains supported for backward compatibility", | |
| input: "t", | |
| }, | |
| } | |
| for _, tc := range tests { | |
| t.Run(tc.name, func(t *testing.T) { | |
| got, err := ParseAccumulate(tc.input) | |
| if err != nil { | |
| t.Fatalf("unexpected error parsing %q: %s", tc.input, err) | |
| } | |
| if got != opencost.AccumulateOptionAll { | |
| t.Fatalf("expected %q for %q, got %q", opencost.AccumulateOptionAll, tc.input, got) | |
| } | |
| }) | |
| } | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case opencost.AccumulateOptionWeek, opencost.AccumulateOptionMonth: | ||
| // week accumulation supports either daily or already-weekly sets | ||
| if accumulateBy == opencost.AccumulateOptionWeek && step == week { | ||
| return week | ||
| } | ||
| return day | ||
| default: | ||
| return step | ||
| } |
There was a problem hiding this comment.
resolveStepForAccumulate does not special-case AccumulateOptionQuarter. As a result, callers can request accumulateBy=quarter&step=week (or another non-daily duration) and this function will return the non-daily step, despite the comments below stating quarter accumulation operates on daily inputs. Add AccumulateOptionQuarter to the month/week branch (returning daily) so quarter accumulation always queries daily inputs unless/ until a dedicated quarterly step is supported.
| // quarter accumulation operates on daily inputs and calendar-rounded query windows | ||
| return resolveStepForAccumulate(24*time.Hour, accumulateBy), nil | ||
| default: | ||
| step := qp.GetDuration("step", window.Duration()) |
There was a problem hiding this comment.
resolveStepFromQuery is declared to return (time.Duration, error) but it never returns a non-nil error (it relies on qp.GetDuration, which falls back to the default on parse failure). Either (a) remove the error return and simplify the call sites, or (b) explicitly validate/parsing step and return a BadRequest error on invalid values so the new error handling paths are meaningful.
| step := qp.GetDuration("step", window.Duration()) | |
| step, err := time.ParseDuration(stepRaw) | |
| if err != nil { | |
| return 0, fmt.Errorf("invalid step %q", stepRaw) | |
| } |
| // Accumulate is an optional parameter, defaulting to false, which if true | ||
| // sums each Set in the Range, producing one Set. | ||
| accumulate := qp.GetBool("accumulate", false) | ||
| accumulate := resolveAccumulateFromQuery(qp) | ||
| accumulateBy, err := resolveAccumulateOption(accumulate, qp.Get("accumulateBy", "")) | ||
| if err != nil { |
There was a problem hiding this comment.
In ComputeAllocationHandlerSummary, the comment above accumulate says it “sums each Set in the Range, producing one Set”, but the new accumulateBy behavior can produce multiple calendar-window sets (e.g. weekly). Also accumulate is now an opencost.AccumulateOption (not a bool), which is confusing given the name and the existing comment. Update the comment and consider renaming accumulate to something like accumulateOpt to reflect the new type/semantics.
| // Accumulate is an optional parameter, defaulting to false, which if true | ||
| // sums each Set in the Range, producing one Set. | ||
| accumulate := qp.GetBool("accumulate", false) | ||
| accumulate := resolveAccumulateFromQuery(qp) | ||
|
|
||
| // Accumulate is an optional parameter that accumulates an AllocationSetRange | ||
| // by the resolution of the given time duration. | ||
| // Defaults to 0. If a value is not passed then the parameter is not used. | ||
| accumulateBy := opencost.AccumulateOption(qp.Get("accumulateBy", "")) | ||
|
|
||
| // if accumulateBy is not explicitly set, and accumulate is true, ensure result is accumulated | ||
| if accumulateBy == opencost.AccumulateOptionNone && accumulate { | ||
| accumulateBy = opencost.AccumulateOptionAll | ||
| accumulateBy, err := resolveAccumulateOption(accumulate, qp.Get("accumulateBy", "")) |
There was a problem hiding this comment.
In ComputeAllocationHandler, accumulate is now an opencost.AccumulateOption rather than a bool, but surrounding comments still describe it as a boolean flag and describe accumulateBy as a “time duration”. This is now inaccurate and makes the parameter semantics harder to understand/maintain. Update the comments (and consider renaming accumulate similarly to the summary handler) so they match the new option-based accumulation behavior.
| if accumulateByRaw == "none" { | ||
| return opencost.AccumulateOptionNone, nil | ||
| } | ||
|
|
||
| accumulateByOpt := opencost.ParseAccumulate(accumulateByRaw) | ||
| if accumulateByOpt == opencost.AccumulateOptionNone { | ||
| return opencost.AccumulateOptionNone, fmt.Errorf("invalid accumulateBy option: %s", accumulateBy) | ||
| } |
There was a problem hiding this comment.
resolveAccumulateOption rejects accumulateBy=all because opencost.ParseAccumulate("all") returns AccumulateOptionNone (it only maps "true" -> all). Previously the handler accepted accumulateBy=all (it was cast directly to opencost.AccumulateOption), so this breaks backward compatibility. Handle "all" explicitly here (similar to "none") or extend ParseAccumulate to support "all".
|
Looks good — covers most of the original review. Two things from Copilot's review that I'd want addressed before merge: Reader-mode ReadString (buffer.go:372) and NewFileStringTableReaderFrom (support.go.tmpl) still allocate based on attacker-controlled length with no hard cap. After the v0.2 length prefix change, ReadString can be asked to allocate ~4 GB. Add a MaxStringLength / MaxStringTableEntries constant checked unconditionally before make. H2 (sticky errors threaded through generated unmarshallers) is genuinely structural and worth its own PR, just want to make sure it doesn't get marked as done by association with Buffer.Err(). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| queryWindow, err := resolveQueryWindowForAccumulate(window, accumulateBy) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid accumulation configuration: %w", err) | ||
| } |
There was a problem hiding this comment.
resolveQueryWindowForAccumulate() expands the queried window to calendar boundaries, but the subsequent trimAllocationSetRangeToRequestWindow() only filters overlapping sets (it doesn’t prorate/contract boundary sets). This means responses can include costs from outside the originally requested window when the request isn’t aligned to the accumulation boundary (e.g. mid-week to mid-week with weekly accumulation). If the API contract is that window bounds the costs returned, consider contracting/prorating boundary sets to the requested window (or avoid expanding the query window and instead allow partial edge buckets).
| queryWindow, err := resolveQueryWindowForAccumulate(window, accumulateBy) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid accumulation configuration: %w", err) | |
| } | |
| // Validate the accumulation configuration, but keep the actual query | |
| // bounded to the caller-requested window. Expanding to calendar-aligned | |
| // boundaries here can cause ComputeAllocation/ComputeAssets to include | |
| // costs that fall outside the requested window when the request is not | |
| // aligned to the accumulation period. | |
| if _, err := resolveQueryWindowForAccumulate(window, accumulateBy); err != nil { | |
| return nil, fmt.Errorf("invalid accumulation configuration: %w", err) | |
| } | |
| queryWindow := window |
| queryWindow, err := resolveQueryWindowForAccumulate(window, accumulateBy) | ||
| if err != nil { | ||
| proto.WriteError(w, proto.BadRequest(fmt.Sprintf("Invalid accumulation configuration: %s", err))) | ||
| return | ||
| } |
There was a problem hiding this comment.
queryWindow expansion (calendar-rounded) plus the later trimAllocationSetRangeToRequestWindow() (overlap filter) means the handler can return full calendar buckets that include time/cost outside the originally requested window. If that’s intentional, please document it near the queryWindow/trim usage; if not, trimming likely needs to contract/prorate boundary buckets instead of only filtering overlaps.
@peatey Will you make a ticket for these and assign to me? From just a quick once over, these seem mostly accurate, and I'd like to set aside some time to get them addressed. Also worth noting that I think these were from my changeset, just merged into Alex's :) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Meijer <ameijer@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Meijer <ameijer@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| step, err := time.ParseDuration(stepRaw) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("invalid step %q: must be a Go duration or one of hour, day, week, month, quarter: %w", stepRaw, err) | ||
| } | ||
| return resolveStepForAccumulate(step, accumulateBy), nil |
There was a problem hiding this comment.
resolveStepFromQuery drops the underlying time.ParseDuration error details, making it hard for callers to understand what was wrong with the provided step. Consider wrapping the parse error (and/or mentioning the accepted keyword values) so the API returns an actionable message when a user passes an invalid duration.
| func resolveAccumulateFromQuery(qp httputil.QueryParams) opencost.AccumulateOption { | ||
| rawAccumulate := strings.TrimSpace(qp.Get("accumulate", "")) | ||
| accumulate := opencost.ParseAccumulate(rawAccumulate) | ||
| if accumulate == opencost.AccumulateOptionNone && qp.GetBool("accumulate", false) { | ||
| return opencost.AccumulateOptionAll | ||
| } | ||
|
|
||
| return accumulate |
There was a problem hiding this comment.
resolveAccumulateFromQuery uses opencost.ParseAccumulate, which does not recognize the string "all" (it only maps "true" to AccumulateOptionAll). That means requests like ?accumulate=all (which this PR’s docstring implies is valid) will silently behave as none. Consider explicitly handling "all" (and maybe "none") here before calling ParseAccumulate and add a test for accumulate=all to prevent regressions.
| func resolveQueryWindowForAccumulate(window opencost.Window, accumulateBy opencost.AccumulateOption) (opencost.Window, error) { | ||
| switch accumulateBy { | ||
| case opencost.AccumulateOptionHour, opencost.AccumulateOptionDay, opencost.AccumulateOptionWeek, opencost.AccumulateOptionMonth, opencost.AccumulateOptionQuarter: | ||
| windows, err := window.GetAccumulateWindows(accumulateBy) | ||
| if err != nil { | ||
| return opencost.Window{}, err | ||
| } | ||
| if len(windows) == 0 { | ||
| return opencost.Window{}, fmt.Errorf("no query windows for accumulate option %s", accumulateBy) | ||
| } | ||
|
|
||
| return opencost.NewClosedWindow(*windows[0].Start(), *windows[len(windows)-1].End()), nil | ||
| default: | ||
| return window, nil | ||
| } |
There was a problem hiding this comment.
This code path allows accumulateBy=quarter (via ParseAccumulate) and also rounds the query window for AccumulateOptionQuarter, but core/pkg/opencost/allocation.go’s (*AllocationSetRange).Accumulate does not implement AccumulateOptionQuarter and will return an error ("invalid accumulateByType: quarter"). Either add quarter support in AllocationSetRange.Accumulate, or reject/translate quarter in the costmodel layer so the API doesn’t advertise an option that cannot succeed.
…into atm/fix-accumulate-by
|
Description
enhances accumulate parameter to govern step sizes. maintains backward compatibility
Related Issues
User Impact
Testing