fix(clickhouse): avoid server-tz drift in sandbox/team metrics filters#3304
fix(clickhouse): avoid server-tz drift in sandbox/team metrics filters#3304Piwriw wants to merge 5 commits into
Conversation
Replace {start_time:DateTime64}/{end_time:DateTime64} placeholders with
fromUnixTimestamp64Nano({start:Int64})/{end:Int64} and pass Unix nano values
via clickhouse.Named, mirroring the pattern already used in
packages/clickhouse/pkg/sandboxlogs/sandboxlogs.go.
The previous DateNamed(..., clickhouse.Seconds) call serializes a UTC
time.Time to a wall-clock string without offset (e.g. '2025-02-17
00:00:00'). On a non-UTC server the column inherits that tz and the
filter window shifts by the offset (8h on CST), returning empty or stale
data for GET /sandboxes/{sandboxID}/metrics, GET /teams/{teamID}/metrics,
and GET /teams/{teamID}/metrics/max.
Unix nano is an absolute instant and sidesteps literal interpretation
entirely. Returned time.Time values still carry Location via the
DateTime64 column type, so downstream JSON serialization stays RFC3339.
Signed-off-by: joohwan <piwriw@163.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 482ce5f002
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Could someone familiar with the ClickHouse readers take a review pass? Happy to address any feedback. |
API handlers accept Unix-second query params without an upper bound and utils.GetSandboxStartEndTime passes them straight into time.Unix. Common "up to latest" sentinels like 9999999999 (= year 2286) make the previous start.UTC().UnixNano() wrap to a negative int64 before ClickHouse ever sees it, silently breaking the fromUnixTimestamp64Nano filter window built in the prior commit. Go's time.Time.UnixNano is undefined beyond ~2262-04-11 (int64 ns range). Add unixNanoForCH which clamps the Unix-second input to the largest value whose nanosecond representation still fits in int64 (9223372036s = 2262-04-11T23:47:16Z), then converts. Routing all conversions through this helper keeps the prior fix valid for unbounded end values. Verified: end=9999999999 raw .UnixNano() = -8446744074709551616 (wrapped), unixNanoForCH returns 9223372036000000000 (positive, in range). Signed-off-by: joohwan <piwriw@163.com>
112194d to
ff8d973
Compare
The unixNano helper in packages/clickhouse/pkg/sandboxlogs/sandboxlogs.go (introduced in e2b-dev#3236, 2026-07-08) had the same int64 overflow risk as the code paths fixed in the previous commit. end=9999999999 (= year 2286, a common "up to latest" sentinel) would wrap to a negative int64 and break the fromUnixTimestamp64Nano filter for GET /sandboxes/{sandboxID}/logs and GET /templates/{templateID}/builds/{buildID}/logs. Apply the same Unix-second clamp used in packages/clickhouse/pkg/sandbox.go. The constant is duplicated to avoid pulling the parent package into this subpackage; consolidate into a shared timeutil package if a third caller appears. Signed-off-by: joohwan <piwriw@163.com>
726e2b2 to
ddf14b7
Compare
Signed-off-by: joohwan <piwriw@163.com>
8e454de to
f4b3287
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4b3287f34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: joohwan <piwriw@163.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Summary
DateTime64query parameters withInt64Unix-nanosecond parameters converted throughfromUnixTimestamp64Nano.sandbox_metrics_gaugequeries inpackages/clickhouse/pkg/sandbox.goteam_metrics_sumandteam_metrics_gaugequeries inpackages/clickhouse/pkg/team.gopackages/clickhouse/pkg/timestamp, including safe clamping for dates outside theint64nanosecond range.Why
clickhouse.DateNamed(..., clickhouse.Seconds)serializes a UTCtime.Timeas a wall-clock string without an offset, such as'2025-02-17 00:00:00'.When a
DateTime64(9)column has no explicit timezone, ClickHouse interprets that value using the server timezone. On a non-UTC server such asAsia/Shanghai, the query window shifts by the timezone offset. This can produce empty results for short ranges and stale or incorrect results for longer ranges.The issue has existed since the original metrics migrations:
20250717135224_sandbox_metrics.sql20250801113224_team_metrics.sqlThe sandbox logs reader introduced in #3236 already used Unix nanoseconds. This PR applies the same timezone-independent approach to the older metrics readers and moves the shared conversion into a dedicated package.
Affected Endpoints
GET /sandboxes/{sandboxID}/metricsGET /teams/{teamID}/metricsGET /teams/{teamID}/metrics/maxImplementation
Unix nanoseconds represent an absolute instant. Passing them as
Int64values and converting them withfromUnixTimestamp64Nanoavoids timezone-dependent string parsing:The shared
timestamp.UnixNanohelper also clamps far-future and far-past values before conversion, preventingtime.Time.UnixNanooverflow.Returned timestamps and their JSON serialization are unchanged.
Testing
int64range clamping.No schema changes or migrations are required.