fix(logs): parse date-time log timestamps#33
Conversation
732053a to
0c05f52
Compare
marckong
left a comment
There was a problem hiding this comment.
Clean, well-scoped migration of log timestamps from epoch-millis to RFC3339 time.Time. Verified no production callers are broken: the local logSearchRequest never carried start/end times, LogActivityBucket times have no consumers, and printLogEvent is updated consistently. Fixtures and assertions match the new contract.
Non-blocking: common.gen.go is marked DO NOT EDIT (oapi-codegen) — worth confirming the source OpenAPI spec is updated so these edits survive regeneration.
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI’s log handling to align with an updated API contract where log timestamp fields are returned/accepted as RFC3339 date-time strings rather than Unix milliseconds. It updates the generated API models accordingly, adjusts log rendering to format native time.Time values, and refreshes test fixtures to match the new payload shape.
Changes:
- Update generated log-related API models to use
time.Time/*time.Timefor timestamp fields. - Render searched and streamed log events using
FormatTimestamp(time.Time)instead of converting from Unix milliseconds. - Update log-related test fixtures (search + SSE stream) to emit RFC3339 timestamp strings.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/output/functions.go | Switch log event printing to accept time.Time timestamps and format via FormatTimestamp. |
| internal/logfollow/follow_test.go | Update SSE test payload fixtures to emit RFC3339 timestamp strings. |
| internal/cmd/functions/logs_test.go | Update function logs test fixtures to match timestamp-as-string contract (including fractional seconds). |
| internal/cmd/frontends/logs_test.go | Update frontend logs test fixtures to match timestamp-as-string contract (including fractional seconds). |
| internal/apiclient/common/common.gen.go | Regenerate OpenAPI client models to represent log timestamps and time filters as time.Time. |
| internal/apiclient/client_test.go | Update client request-body assertions for start_time/end_time to compare strings. |
| internal/api/log_stream_test.go | Update streamed log event fixtures to RFC3339 strings and assert parsed time.Time values. |
Files not reviewed (1)
- internal/apiclient/common/common.gen.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Timestamp Unix timestamp in milliseconds. | ||
| Timestamp int64 `json:"timestamp"` | ||
| // Timestamp Event timestamp. | ||
| Timestamp time.Time `json:"timestamp"` |
There was a problem hiding this comment.
[P2] Rollout coupling — no fallback decoder for the old contract.
Switching these timestamp fields from int64 to time.Time means the JSON decoder now requires a quoted RFC3339 string. A bare epoch-millis number (e.g. 1760000000000) will fail time.Time.UnmarshalJSON at runtime. The updated fixtures make the tests pass, but they can't prove the deployed API actually emits date-time strings — the fixtures changed in lockstep with the model.
Please confirm the server-side contract change is already deployed (or that CLI and API ship together). If there's any version-skew window where the API still returns integer timestamps, volcano logs and log streaming will hard-fail with an unmarshal error rather than degrade. This applies to every log timestamp field in this file (LogEvent, LogSearchEvent, LogActivityBucket) plus the *Request start/end times.
Summary
Tests