Skip to content

fix(timeShift): honor alignDST in the render timezone - #587

Merged
timtofan merged 1 commit into
masterfrom
atimofieiev/timeshift-align-dst
Oct 28, 2026
Merged

timtofan merged 1 commit into
masterfrom
atimofieiev/timeshift-align-dst

Conversation

@timtofan

@timtofan timtofan commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

What issue is this change attempting to solve?

Why

timeShift advertises alignDST, but ignores it. Weekly traffic comparisons therefore compare different local hours after a daylight-saving transition and can trigger false alerts. Passing true as the fourth argument currently changes nothing.

The render timezone must govern this comparison: a server can run in UTC while carbonapi is configured for a regional timezone, and a request can supply its own tz.

How does this change solve the problem? Why is this the best approach?

Architecture

The HTTP render handler carries the request timezone in the evaluation context, falling back to configured tz, then the local timezone. One shared TimeShiftOffset helper calculates the correction for both backend fetch planning and expression evaluation. This keeps the fetched historical window and the timestamps used to read it from the metric map identical.

Fetch planning now receives absolute request bounds and visits outer time transforms before their arguments. Nested shifts, moving windows, and timeStack therefore calculate each historical comparison against the dates at that level. Existing Metrics() callers retain relative requests without timezone correction.

The evaluator/backend boundary and storage protocol stay in place. This implements the advertised graphite-web compatibility option within the existing function, without adding another backend or a separate query path. Offset differences are measured in seconds, including regions with half-hour DST changes.

Before / after

Before After
timeShift(source,'7d',true,true) always fetches 168 hours earlier. With Europe/Amsterdam, it fetches 169 hours earlier across autumn DST and 167 across spring DST.
Each weekly comparison uses a fixed duration regardless of the configured/request timezone. Each lag is corrected independently using the render timezone.
Function metadata omits the advertised optional argument. Metadata exposes alignDST; positional arguments and both alignDST/alignDst named spellings work.
Ordinary shifts use a fixed number of seconds. Omitted/false alignDST and UTC/fixed-offset zones preserve fixed-duration shifts.

Usage

Existing fourth-argument consumers activate the implementation immediately. For a weekly alert baseline, send tz=Europe/Amsterdam with:

averageSeries(
  timeShift(source,'7d',true,true),
  timeShift(source,'14d',true,true),
  timeShift(source,'21d',true,true)
)

The production validation exercised an existing placement-traffic dashboard expression that already uses this argument. The README includes request and three-week-baseline examples.

As in graphite-web, one correction is applied only when neither range spans a timezone transition. This does not resample missing/repeated local hours point by point. The existing resetEnd limitation remains.

How can we be sure this works as expected?

  • Passed make test TAGS='-tags cairo,nativegzip': complete race suite.
  • Passed golangci-lint 2.14.0: zero issues.
  • Regression tests cover spring/autumn, signed shifts, UTC, half-hour DST, both named spellings, transition-spanning ranges, nested fetch/evaluation bounds, input immutability, and HTTP timezone precedence/error handling.
  • An independent comparison against the base revision matched the metric-request sets for 875 fixed-duration expressions, including nested moving windows, Holt-Winters, and timeStack.
  • Built the candidate on a production frontend and passed its parser, evaluator, and HTTP-handler race tests there.
  • Passed 15 HTTP checks through a temporary loopback-only production instance with real production backends. The existing placement-traffic comparison matched the explicit 169h/337h/505h workaround at all 60 timestamps, with 60 non-null values. All 60 changed from the incorrect fixed-week comparison; ordinary fixed-shift output matched the running service.
  • Production HTTP checks also verified spring/autumn, UTC override, positive shifts, half-hour DST, transition-spanning ranges, and the separate 7/14/21-day offsets after the next weekly boundary.

Rollout / rollback

The temporary instance was stopped and its build artifacts removed after validation. The running production service retained its original PID and executable and returned HTTP 200 before and after validation. This PR does not deploy the fleet or edit alerts. Consumers opt in with alignDST=true; disabling that option restores fixed-duration behavior.

@timtofan timtofan changed the title fix(timeShift): honor alignDST in the render timezone fix(timeShift): honor alignDST in the render timezone [OBSBACKB-8294] Oct 28, 2026
@timtofan timtofan changed the title fix(timeShift): honor alignDST in the render timezone [OBSBACKB-8294] fix(timeShift): honor alignDST in the render timezone Oct 28, 2026
@timtofan
timtofan merged commit c3363f6 into master Oct 28, 2026
6 checks passed
@timtofan
timtofan deleted the atimofieiev/timeshift-align-dst branch October 28, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants