convert ISO timestamp output from UTC to local time + offset in flux dmesg and eventlog commands
#6423
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: It would be useful to optionally append the TZ offset to ISO 8601 timestamps in Flux, but no function exists to do that. Add a new timestamp_tzoffset() function that takes a struct tm and returns the current timezone TZ offset of the form [+-]HH:MM. Note that this function is required in order to add the colon (:), which aids in readability, because strftime(3) "%z" does not include a colon. (The colon is optional in ISO 8601). timestamp_tzoffset() also returns "Z" instead of "+00:00" for UTC for backwards compatibility with existing timestamp formats.
Problem: There's no tests of timestamp_tzoffset() handling of invalid arguments. Add a couple tests to the libutil timestamp.c unit tests.
Problem: The eventlogger ISO formatter always outputs eventlog timestamps in UTC. This can make it difficult to compare these timestamps to system timestamps which are usually recorded in localtime. Convert the eventlog formatter ISO timestamp to use localtime plus the timezone offset (e.g. +08:00). If UTC is desired, then TZ=UTC can be used to get the old behavior.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
chu11
reviewed
Nov 8, 2024
t/t0009-dmesg.t
Outdated
| test_expect_success 'dmesg with TZ=America/Los_Angeles uses tz offset' ' | ||
| TZ=America/Los_Angeles flux dmesg \ | ||
| | grep -E \ | ||
| "^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}\.[0-9]+-[0-9]{2}:[0-9]{2}" |
There was a problem hiding this comment.
my memory isn't good on timezone stuff, but if we're fixing American/Los Angeles in the test, wouldn't the offset in the last chunk be known? -08:00?
There was a problem hiding this comment.
I think it could be different depending on current daylight savings? Anyway, this was probably good enough and I didn't want a randomly failing test.
The test is failing anyway in some builders - I wonder if they don't support this timezeone
|
@chu11 noticed I dropped the actual conversion of the dmesg timestamp to local time here. Setting WIP until I fix that. |
grondo
changed the title
flux dmesg and eventlog commandsflux dmesg and eventlog commands
Problem: flux-dmesg(1) logs ISO 8601 timestamps in UTC timezone only, which makes it difficult to compare to other system timestamps, which are usually displayed in localtime. Convert timestamps by default to localtime+offset form in flux-dmesg(1) output. If UTC is desired, the command may be run with TZ=UTC.
Problem: The timestamp print functions in the dmesg builtin command pass a `struct stdlog_header` by value, but CodeQL rightly warns this structure has a large size and would be better passed by address. Pass a pointer to the stdlog_header in these functions instead of the entire struct.
Problem: A missing colon breaks the display of :command:`flux-dmesg`. Add the missing colon.
grondo
force-pushed
the
issue#6421
branch
2 times, most recently
from
7d28d5d
to
628375a
Compare
Problem: None of the tests in t0009-demsg.t test that flux-dmesg(1) properly formats timestamps in the local time + timezone offset. Add a couple tests that ensure the timestamp output of flux-dmesg(1) is as expected.
Problem: No tests ensure `flux job eventlog` with `--time-format=iso` shows timestamps in local time with a timezone offset. Add a test to t2230-job-info-lookup.t.
grondo
changed the title
flux dmesg and eventlog commandsflux dmesg and eventlog commands
|
Removed WIP. Had to add a workaround in the tests for detecting missing tzdata on |
|
setting MWP. Thanks! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6423 +/- ##
==========================================
- Coverage 83.64% 83.63% -0.01%
==========================================
Files 524 524
Lines 87617 87637 +20
==========================================
+ Hits 73286 73294 +8
- Misses 14331 14343 +12
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the default timestamp display for
flux dmesgand the "iso" time format forflux job eventlogflux kvs eventlogandflux job wait-eventto use local time instead of UTC. This better matches the behavior of most other system utilities and logfiles, and the old behavior can easily be restored if desired by settingTZ=UTC.Fixes #6421