Skip to content

fix(clickhouse): use Unix nanosecond parameters to avoid timezone shifts - #3714

Closed
Piwriw wants to merge 1 commit into
e2b-dev:mainfrom
Piwriw:fix/timezone
Closed

Piwriw wants to merge 1 commit into
e2b-dev:mainfrom
Piwriw:fix/timezone

Conversation

@Piwriw

@Piwriw Piwriw commented Oct 21, 2026 •

Copy link
Copy Markdown

Summary

Follow-up to #3304, which has not been merged. This PR fixes timezone-dependent time filters in ClickHouse metrics queries while preserving the existing sandbox metrics end-time rounding behavior.

Problem

Metrics queries bind time boundaries using clickhouse.DateNamed with timezone-unspecified DateTime64 parameters. The driver serializes these values as date-time strings without an explicit timezone offset.

When the timezone used to format a boundary differs from the timezone ClickHouse uses to interpret it, the query window shifts. This can return missing or unexpected metrics.

Converting the Go values to UTC alone does not resolve this ambiguity, since the serialized strings still carry no timezone information.

Changes

  • Bind sandbox and team metrics time boundaries as Int64 Unix timestamps and convert them with fromUnixTimestamp64Nano, avoiding timezone-dependent string parsing.
  • Use nanoseconds to preserve the precision of the existing DateTime64(9) columns.
  • Preserve sandbox metrics end-time rounding: partial seconds round up, while exact-second boundaries remain unchanged.
  • Add a shared timestamp conversion helper that clamps values outside the int64 nanosecond range to prevent overflow.
  • Reuse the helper in sandbox and build log queries.
  • Add unit tests for the timestamp helper, covering timezone-equivalent inputs, subsecond precision, and timestamp range limits.

No schema migration or API changes are required.

Validation

  • go -C packages/clickhouse test -race -count=1 ./...
  • git diff --check

Both checks pass. The added tests cover the timestamp conversion helper only. This change has not been validated against a live ClickHouse instance with a non-UTC timezone.

@ryan-sana

Copy link
Copy Markdown
Contributor

cannot you just use UTC as the base so that the timezone issue removes, instead of nanosecond? nanosecond is quite unnecessary.

Can you clarify?

@cla-bot cla-bot Bot added the cla-signed label Oct 21, 2026
@Piwriw

Piwriw commented Oct 21, 2026

Copy link
Copy Markdown
Author

cannot you just use UTC as the base so that the timezone issue removes, instead of nanosecond? nanosecond is quite unnecessary.

Can you clarify?

Thanks for raising this. You're right that nanosecond precision is not required to fix the timezone issue.

The problem is that clickhouse.DateNamed serializes the value as a date-time string without a timezone offset. Calling .UTC() before binding only changes the timezone used to format that string; it does not tell ClickHouse to interpret it as UTC. With the current timezone-unspecified DateTime64 parameters, a non-UTC server/session can still interpret the boundary as local time and shift the query window.

Explicitly specifying UTC in the SQL parameter type, or enforcing a UTC session timezone, would also be valid solutions. Using Unix timestamps here avoids relying on either setting.

I chose nanoseconds to preserve the precision of the existing DateTime64(9) metrics columns and reuse the timestamp conversion already used by log queries—not because nanosecond precision itself fixes the timezone issue. The timezone fix comes from passing an unambiguous epoch value. Unix seconds would address that too, but would discard subsecond boundary precision.

@ryan-sana

Copy link
Copy Markdown
Contributor

One small thing before we merge: the description mentions regression tests for sub-second precision, the sandbox end-time rounding and query construction, but the PR only adds tests for the timestamp helper. Could you either add those tests or trim the description to match? Either is fine with me.

Once that's done I'll get CI running and take it from there. Thanks again for sticking with this one!

@Piwriw

Piwriw commented Oct 21, 2026

Copy link
Copy Markdown
Author

Good catch, thanks! @ryan-sana I've updated the PR description to match the actual test coverage. The added tests cover the timestamp helper only; I've removed the claims about query-construction and sandbox end-time rounding tests.

@ryan-sana

Copy link
Copy Markdown
Contributor

We have not observed any cross-timezone queries unfortunately under our prod. While I acknowledge that this is a good reliability bug fix but I still close this PR and will get back to it once we observe real issues.

@ryan-sana ryan-sana closed this Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants