Skip to content

Add UseUTC option to exclude timezone from UTC timestamps#55

Closed
hanzei wants to merge 11 commits intomasterfrom
MM-63462_utc-logs
Closed

Add UseUTC option to exclude timezone from UTC timestamps#55
hanzei wants to merge 11 commits intomasterfrom
MM-63462_utc-logs

Conversation

@hanzei
Copy link
Contributor

@hanzei hanzei commented Sep 4, 2025

Per discussion in https://mattermost.atlassian.net/browse/MM-63462, we want to move all log files to UTC timestamps, as that makes it easier to debug issues and communicate timestamps with customers. Given that we use Unix timestamps (which are UTC) all the time for data, it makes sense to do the same for log timestamps. Please note that the lumberjack.Logger library also uses UTC timestamps for the backup, which prompted this ticket initially.

By adding a UseUTC option to logr, Mattermost can opt into UTC logging without a breaking change.

Summary

  • Add UseUTC option to JSON and Plain formatters to convert timestamps to UTC
  • When UseUTC is true, timezone information is excluded from timestamp format for cleaner output
  • Add comprehensive tests to verify UTC timestamp functionality

Changes

  • formatter.go: Add DefTimestampUTCFormat constant for UTC timestamps without timezone suffix
  • formatters/json.go: Add UseUTC field and logic to use UTC format when enabled
  • formatters/plain.go: Add UseUTC field and logic to use UTC format when enabled
  • Tests: Add test cases to verify UTC timestamp conversion and timezone exclusion

Before/After

Before (with timezone):

{"timestamp": "2025-09-04 15:04:05.000 +00:00", "level": "error", "msg": "test"}

After (without timezone):

{"timestamp": "2025-09-04 15:04:05.000", "level": "error", "msg": "test"}

Ticket Link

https://mattermost.atlassian.net/browse/MM-63462

🤖 Generated with Claude Code

hanzei and others added 2 commits September 4, 2025 11:01
- Add UseUTC bool field to both JSON and Plain formatters
- When UseUTC is true, timestamps are converted to UTC before formatting
- Default behavior remains unchanged (local time)
- Add comprehensive tests for UTC timestamp functionality
- Maintain backward compatibility with existing configurations

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Add DefTimestampUTCFormat constant for UTC timestamps without timezone
- Modify JSON and Plain formatters to use UTC format when UseUTC is true
- Update tests to verify timezone exclusion from UTC timestamps
- UTC timestamps now format as "2006-01-02 15:04:05.000" instead of "2006-01-02 15:04:05.000 +00:00"
- Maintain backward compatibility with existing timestamp formats

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Sep 4, 2025

Unit Test Results

112 tests  +6   112 ✔️ +6   28s ⏱️ ±0s
  10 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 43aa788. ± Comparison against base commit 11788d8.

♻️ This comment has been updated with latest results.

hanzei and others added 2 commits September 4, 2025 11:17
Use time.ParseInLocation with time.Local instead of time.Parse to properly
parse timestamps in local timezone for test validation. This fixes CI test
failures where time.Parse was incorrectly returning UTC times.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Sep 4, 2025
@wiggin77
Copy link
Member

This is fine, but I am curious what prompted the addition. Did you run across a case where production server wasn't set to UTC at the OS level? That would be strange.

@wiggin77 wiggin77 requested a review from Copilot September 16, 2025 20:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a UseUTC option to both JSON and Plain formatters to standardize logging timestamps to UTC format without timezone information, making it easier to debug issues and communicate timestamps with customers.

Key changes:

  • Add UseUTC boolean field to both JSON and Plain formatters
  • When enabled, converts timestamps to UTC and uses format without timezone suffix
  • Add comprehensive test coverage for both formatters to verify UTC conversion functionality

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

File Description
formatters/json.go Add UseUTC field and UTC conversion logic to JSON formatter
formatters/plain.go Add UseUTC field and UTC conversion logic to Plain formatter
formatters/json_test.go Add test cases for JSON formatter UTC timestamp functionality
formatters/plain_test.go Add test cases for Plain formatter UTC timestamp functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

timestampStr := matches[1]

// Parse the timestamp from the log output using UTC format (no timezone)
loggedTime, err := time.Parse(logr.DefTimestampFormat, timestampStr)
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment mentions parsing with 'UTC format (no timezone)' but the code uses logr.DefTimestampFormat which includes timezone information. When UseUTC is true, the formatter should use a different timestamp format without timezone suffix, but the test is parsing with the wrong format.

Suggested change
loggedTime, err := time.Parse(logr.DefTimestampFormat, timestampStr)
const utcTimestampFormat = "2006-01-02T15:04:05.000"
loggedTime, err := time.Parse(utcTimestampFormat, timestampStr)

Copilot uses AI. Check for mistakes.
timestampStr := matches[1]

// Parse the timestamp from the log output using UTC format (no timezone)
loggedTime, err := time.Parse(logr.DefTimestampFormat, timestampStr)
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment mentions parsing with 'UTC format (no timezone)' but the code uses logr.DefTimestampFormat which includes timezone information. When UseUTC is true, the formatter should use a different timestamp format without timezone suffix, but the test is parsing with the wrong format.

Suggested change
loggedTime, err := time.Parse(logr.DefTimestampFormat, timestampStr)
loggedTime, err := time.Parse("2006-01-02 15:04:05.000", timestampStr)

Copilot uses AI. Check for mistakes.
Comment on lines +133 to +135
if jlr.UseUTC {
time = time.UTC()
}
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation only converts the time to UTC but doesn't change the timestamp format. According to the PR description, when UseUTC is true, the format should exclude timezone information, but the code still uses the same timestampFmt which includes timezone suffix.

Copilot uses AI. Check for mistakes.
Comment on lines +97 to +99
if p.UseUTC {
t = t.UTC()
}
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation only converts the time to UTC but doesn't change the timestamp format. According to the PR description, when UseUTC is true, the format should exclude timezone information, but the code still uses the same timestampFmt which includes timezone suffix.

Copilot uses AI. Check for mistakes.
@hanzei
Copy link
Contributor Author

hanzei commented Sep 17, 2025

This is fine, but I am curious what prompted the addition. Did you run across a case where production server wasn't set to UTC at the OS level? That would be strange.

From my experience at CRE, almost no customer uses UTC for their servers. All use the local timezone. Hence, there is often confusion with the time stamps, as we use Unix time stamps internally, and hence, a lot of log events are also in UTC.

By adding a UseUTC option, the server can opt into UTC logging for all log events. See mattermost/mattermost#33837

Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTGM 👍 Copilot pointed out some inconsistencies in the comments.

@hanzei
Copy link
Contributor Author

hanzei commented Oct 6, 2025

@hanzei hanzei closed this Oct 6, 2025
@hanzei hanzei added Invalid This doesn't seem right and removed 2: Dev Review Requires review by a core committer labels Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants