Add Timestamp type for flexible log timestamp parsing#10
Conversation
Timestamp wraps time.Time and provides JSON unmarshalling from a wide range of common log timestamp formats (RFC3339, RFC1123, Apache/NGINX CLF, syslog stamp, compact ISO 8601, Go log default, space/slash datetimes with optional fractional seconds) plus JSON numbers as Unix epoch seconds. Marshalling uses DefaultTimeFormat and emits null for the zero value. Implements IsNull/SetNull, Set, database/sql.Scanner and driver.Valuer. Renames the context helper Timestamp(ctx) to TimestampFromContextOrNow to free the Timestamp identifier for the new type. ContextWithTimestamp is now generic over time.Time | Timestamp.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new golog.Timestamp wrapper type to make log timestamp handling more flexible across JSON and SQL boundaries, and renames the existing context-timestamp helper to avoid identifier collisions.
Changes:
- Added
Timestamptype with JSON marshal/unmarshal (multiple layouts + unix epoch seconds), nullability helpers, and SQLScanner/Valuersupport. - Added comprehensive table-driven tests for supported timestamp formats and SQL/JSON behaviors.
- Renamed
golog.Timestamp(ctx)toTimestampFromContextOrNow, and updated logger/context helpers accordingly (including makingContextWithTimestampaccepttime.TimeorTimestamp).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| timestamp.go | Implements the new Timestamp type (JSON/SQL/nullability) and exported TimestampFormats. |
| timestamp_test.go | Adds tests covering parsing formats, roundtrips, null handling, and SQL interfaces. |
| logger.go | Switches message creation to use TimestampFromContextOrNow after helper rename. |
| context.go | Updates context timestamp keying, renames helper, and makes ContextWithTimestamp accept `time.Time |
| context_test.go | Updates tests to use TimestampFromContextOrNow after the breaking rename. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (t *Timestamp) Set(time time.Time) { | ||
| t.Time = time |
There was a problem hiding this comment.
The Set method parameter is named time, which shadows the imported time package name within the method scope. This makes the code harder to read and can lead to mistakes during future edits (e.g., if you later need time.Now inside this method). Rename the parameter to something like v, tt, or value.
| func (t *Timestamp) Set(time time.Time) { | |
| t.Time = time | |
| func (t *Timestamp) Set(value time.Time) { | |
| t.Time = value |
There was a problem hiding this comment.
Fixed in 21c35e2. Renamed to value — the parameter no longer shadows the time package.
Align doc.go JSON example and message-timestamp section with NewDefaultFormat. Add godoc for Format, NewDefaultFormat, and DefaultTimeFormat. Extend timestamp Scan coverage (nil, time.Time, int64 Unix, string/[]byte via ParseTimestamp, errors, unsupported types, nil receiver). Update Scan implementation integration. Made-with: Cursor
- README.md: add Timestamp + Format to API Reference Core Types, add "Parsing Log Timestamps" subsection under Advanced Features (with upgrade note for the TimestampFromContextOrNow rename), and fix the stale JSON output example to match NewDefaultFormat (time key, "2006-01-02 15:04:05.000" layout) — same fix that already landed in doc.go. - timestamp.go: unify error message prefix in (*Timestamp).Scan default branch to match the other four "golog.Timestamp: ..." errors in the file. - context_test.go: rename TestTimestamp to TestTimestampFromContextOrNow (name collided with the new Timestamp type) and add a subtest pinning the stored-zero-time fallback so a future refactor cannot silently regress the new semantics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- timestamp.go: fix misleading "local / no timezone" comments on zoneless layouts in TimestampFormats. time.Parse returns UTC for layouts without a zone indicator (use time.ParseInLocation for Local). Updated both "2006-01-02 15:04:05" and "20060102T150405" comments, and noted the Parse semantics inline. - timestamp.go: rename (*Timestamp).Set parameter from "time" to "value" so it no longer shadows the imported time package. - context_test.go: add two subtests exercising the generic ContextWithTimestamp[T] Timestamp branch (non-zero Timestamp round-trips via TimestampFromContext, zero Timestamp falls back to time.Now via TimestampFromContextOrNow). Prevents regressions in the type-assertion path. Copilot comment about UnmarshalJSON "manually stripping quotes" is a false positive: the string branch already calls json.Unmarshal into a string var, which handles JSON escapes correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Timestamptype that embedstime.Timeand unmarshals JSON from a wide set of common log timestamp formats (RFC3339 with/without fractional seconds, RFC1123/Z, Apache/NGINX CLF, syslogStamp, compact ISO 8601, Go log default, space/slash datetimes) and from JSON numbers as Unix epoch seconds. Accepted layouts live in the exportedTimestampFormatsslice so callers can extend or replace them.MarshalJSONformats usingDefaultTimeFormat(currentlytime.RFC3339Nano) and emitsnullwhen the value is null.IsNull/SetNull/Set, aNewTimestampconstructor, anddatabase/sql.Scanner+driver.Valuerso the type plugs into SQL drivers with SQL NULL mapped to the zero value.golog.Timestamp(ctx)helper toTimestampFromContextOrNowto free theTimestampidentifier for the new type.ContextWithTimestampis now generic overtime.Time | Timestamp, and its doc comments were refreshed along with the other context timestamp helpers.Breaking change
golog.Timestamp(ctx context.Context) time.Timeis gone — callers should usegolog.TimestampFromContextOrNow(ctx)(same behavior) orgolog.TimestampFromContext(ctx)if they want the zero value instead oftime.Now()as the fallback.Test plan
go test ./.passes, including new table-driven tests covering every accepted formatIsNull,SetNull,Set,NewTimestamp,Scan(time.Time / nil / bad type),Value(null / set),MarshalJSONnull caseTimestampFromContextOrNowzero-time fallback so the new semantics cannot silently regressDocumentation
Pre-merge documentation pass (via
/review+/document-release):TimestampandFormatto the API Reference → Core Types list; added a new "Parsing Log Timestamps" subsection under Advanced Features with a JSON/sql.Scannerexample, aTimestampFormatsnote, and a callout for thegolog.Timestamp(ctx)→golog.TimestampFromContextOrNow(ctx)rename; fixed the stale{"timestamp":"2024-01-15T10:30:45Z",...}example in the JSON Output section to matchNewDefaultFormat("time"key,"2006-01-02 15:04:05.000"layout) — same fix that already landed indoc.go.(*Timestamp).Scan'sdefaultbranch from"failed to scan %T as golog.Timestamp"to"golog.Timestamp: failed to scan %T"so all five error paths in the file share one prefix.TestTimestamp→TestTimestampFromContextOrNow(the old name collided with the newTimestamptype) and added a third subtest asserting thatContextWithTimestamp(ctx, time.Time{})+TimestampFromContextOrNow(ctx)falls back totime.Now()instead of returning the stored zero.benchmarks/,goslog/,logfile/,logsentry/): audited, no changes required — none of them referenced the renamed helper or the old JSON example.