Add option to display timestamps in the local system timezone #3530

jparismorgan · 2023-09-28T22:58:56Z

What

Adds the option to display timestamps in the local system timezone, addressing #1714.

rerun_local_time.mov

Checklist

I have read and agree to Contributor Guide and the Code of Conduct
I've included a screenshot or gif (if applicable)
I have tested demo.rerun.io (if applicable)

jparismorgan · 2023-09-28T23:00:00Z

Hi there! I've quite liked watching Rerun be developed over the past year, so I wanted to contribute a bit. This is also my first Rust I've written, it's quite interesting how similar yet different it is to C++.

Anyways, I'm opening up this PR as an early draft for feedback - does it line up with what you were expecting? A few specific questions / thoughts:

I went with the pattern of passing show_timestamps_in_local_timezone into format(), which seems easiest to keep the function stateless. But curious if there's another pattern to do this I missed in the codebase?
I hard-coded show_timestamps_in_local_timezone = false in several locations in arrow_store/. I believe this is correct because this is the code to serialize data, but perhaps I missed something there?
Cargo.lock had a new dependency added, I believe because I added local-offset here: time = { workspace = true, features = ["formatting", "macros", "local-offset"] }. Will test this before opening this up for proper review.
I have a failing unit test locally which looks like it's in crates/re_sdk/src/recording_stream.rs. Still debugging if it's related:

(rerun-jparismorgan-scripts) ~/repo/rerun-jparismorgan cargo test --all-targets --all-features -p re_sdk --lib                             local-time-option 
   Compiling re_sdk v0.9.0-alpha.4 (/Users/parismorgan/repo/rerun-jparismorgan/crates/re_sdk)
    Finished test [optimized + debuginfo] target(s) in 1.30s
     Running unittests src/lib.rs (target/debug/deps/re_sdk-2385fcf4b104c75d)

running 6 tests
test recording_stream::tests::impl_send_sync ... ok
test recording_stream::tests::disabled ... ok
thread panicked while processing panic. aborting.
test recording_stream::tests::always_flush ... ok
test recording_stream::tests::flush_hierarchy ... ok
test recording_stream::tests::never_flush ... ok
error: test failed, to rerun pass `-p re_sdk --lib`

Caused by:
  process didn't exit successfully: `/Users/parismorgan/repo/rerun-jparismorgan/target/debug/deps/re_sdk-2385fcf4b104c75d` (signal: 5, SIGTRAP: trace/breakpoint trap)

Finally, there are quite a few TODO's left to do - would be curious if you know the answer to any. I was mainly working quickly and will come back to them once the general structure looks good.

Thank you!

PS. Here's a screenshot of work so far - you can see we still have a UTC date next to rerun_example_detect_and_track_objects, but the Recording started date is correct:

And here's the option menu:

emilk · 2023-09-29T10:03:56Z

Hi, and thank you so much for this PR!

I strongly suggest switching out the boolean for an enum:

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
enum TimeZone {
    Local,
    Utc,
}

This will make the code more readable (basically self-documenting), but also allow us to easily add custom timezones in the future, if we so desire.

The store mostly uses time formatting for trace logging and error messages, and for that UTC is fine. You can create a helper format_utc() so you don't need to pass in TimeZone::Utc in all these places.

crates/re_log_types/src/time.rs

crates/re_time_panel/src/paint_ticks.rs

crates/re_viewer/src/ui/rerun_menu.rs

Wumpf

First of all thank you so much for contributing and tackling this feature! And welcome to Rust 😄 .

I went with the pattern of passing show_timestamps_in_local_timezone into format(), which seems easiest to keep the function stateless. But curious if there's another pattern to do this I missed in the codebase?

I think that's the correct way to got about it here, but instead of making it a an enum (something like enum TimeFormat?) would make things more readable and extensible :)

I hard-coded show_timestamps_in_local_timezone = false in several locations in arrow_store/. I believe this is correct because this is the code to serialize data, but perhaps I missed something there?

Didn't go through all the callsites yet, but it looks like this affects mostly debug printing, tests and examples internal to arrow_store. So yes, for those the UTC behavior seems best 👍

Cargo.lock had a new dependency added, I believe because I added local-offset here: time = { workspace = true, features = ["formatting", "macros", "local-offset"] }.

Digged after this a bit and can confirm. The local-offset feature drags this dependency in on linux https://github.com/time-rs/time/blob/main/time/Cargo.toml#L62. But it seems to be super small, so that's alright.

I have a failing unit test locally which looks like it's in crates/re_sdk/src/recording_stream.rs.

I actually have the same failure on main running locally Oo. Let me check what's up there...

Finally, there are quite a few TODO's left to do

I left some comments 👍

General direction works I think. Passing the setting through every corner is annoying on one hand but more flexible in the long run. Relatedly, I'm not sure yet where we want to store the setting in the long run and how granular this kind of thing should be configurable but for the moment it's definitely the right place.

As you already know, if you need quick feedback or answers to question where the Github format doesn't do you can always post on Discord as well :)

crates/re_arrow_store/src/store_format.rs

crates/re_build_tools/src/lib.rs

crates/re_log_types/src/data_row.rs

crates/re_time_panel/src/paint_ticks.rs

Wumpf · 2023-09-29T10:17:09Z

looks like @emilk wrote much of the same while I was busy 😅

jparismorgan · 2023-09-29T14:17:19Z

Thanks @Wumpf and @emilk! I've changed to an enum and cleaned up the code a bit. Still have a few TODO's in there if you'd like to take another pass, they are mainly around Rust & Rerun patterns.

crates/re_arrow_store/src/store_read.rs

Wumpf

(brief scroll over new TODO notes / ones I might have missed :))

crates/re_log_types/src/lib.rs

crates/re_log_types/src/time.rs

jparismorgan · 2023-10-02T10:26:56Z

Okay so removed the last of my TODO's and ready for another round of review!

https://demo.rerun.io/pr/3530 is not yet live - I believe because some of the workflows require approval from a maintainer to start?

Wumpf · 2023-10-02T13:24:30Z

https://demo.rerun.io/pr/3530 is not yet live

web demo upload is sadly one of the things we can't do yet for PRs from fork since it queries a bunch of tokens that we don't want to leak to a github action that we don't own 🤔
you can ignore that part of the pr template

crates/re_arrow_store/src/store_format.rs

Wumpf

aaaalmost good to go. Looking really good now :)

crates/re_log_types/src/time.rs

Wumpf · 2023-10-02T14:15:07Z

Tested it locally on the web, works fine on Firefox & Chrome on my Mac :)

Wumpf

Thank you again!

All good from my side. I merged in main because we have some ci issues here that I thought we fixed there but turns out they still weren't configured correctly. @jprochazk is taking care of it right now :)
Will merge once we achieved green ci!

Wumpf · 2023-10-02T16:14:24Z

oh, looks like your new test actually fails on CI right now:

'time::tests::test_formatting_local_time_zone_no_z' panicked at 'assertion failed: !&datetime.format(TimeZone::Local).contains(\'Z\')', crates/re_log_types/src/time.rs:388:9

https://github.com/rerun-io/rerun/actions/runs/6382347518/job/17320741545?pr=3530

I suspect it failed to determine the time offset on CI?

jparismorgan · 2023-10-02T21:46:46Z

oh, looks like your new test actually fails on CI right now:
'time::tests::test_formatting_local_time_zone_no_z' panicked at 'assertion failed: !&datetime.format(TimeZone::Local).contains(\'Z\')', crates/re_log_types/src/time.rs:388:9
https://github.com/rerun-io/rerun/actions/runs/6382347518/job/17320741545?pr=3530

I suspect it failed to determine the time offset on CI?

Thanks, yep looks like it. I pushed a commit which added some logging, but I'm not seeing it printed out (https://github.com/rerun-io/rerun/actions/runs/6384854632/job/17329151635). So seems I need to update something about the CI also to get it to print. Looking more into that...

jprochazk · 2023-10-02T22:47:19Z

So seems I need to update something about the CI also to get it to print. Looking more into that...

The test needs re_log::setup_native_logging at the start. That might not be enough, I don't remember what the default log levels are. I think you may have more success replacing all the re_log calls with plain println!

Wumpf · 2023-10-03T07:34:34Z

If we can't get this to work on CI I'm very much open to disabling the test since clearly it worked on a variety of systems. But we should probably make the "local time" option greyed out in the ui if we know it's not available, we can do so in a follow-up
Would be ofc nice to find out what about the CI config causes it to fail to get the UTC offset, but this might get tricky

jparismorgan · 2023-10-03T13:40:58Z

Makes sense, thanks. I removed the test (that's what you meant by disable, right?) and in a follow-up will disable the radio boxes if we fail to get the local timezone.

Wumpf · 2023-10-03T15:05:33Z

commented out or something would have been fine, but this will do. Waiting for final final ci run and then we can ship this as part of 0.9 later this week 😄

add option to display timestamps in local timezone

db4d18f

jparismorgan marked this pull request as draft September 28, 2023 23:01

jparismorgan mentioned this pull request Sep 28, 2023

Make timezone used to display time configurable #1714

Closed

Wumpf self-requested a review September 29, 2023 09:37

emilk reviewed Sep 29, 2023

View reviewed changes

crates/re_log_types/src/time.rs Outdated Show resolved Hide resolved

crates/re_log_types/src/time.rs Outdated Show resolved Hide resolved

crates/re_time_panel/src/paint_ticks.rs Outdated Show resolved Hide resolved

crates/re_viewer/src/ui/rerun_menu.rs Outdated Show resolved Hide resolved

Wumpf reviewed Sep 29, 2023

View reviewed changes

jparismorgan added 3 commits September 29, 2023 16:07

update to use timezone enum, add tests

b59ac20

(minor) remove commented out code

326fda7

(minor) clarify comment

0fe5207

Wumpf mentioned this pull request Sep 29, 2023

cargo test -p re_sdk --lib fallbacks crashes on aarch64 Mac #3558

Closed

emilk reviewed Sep 30, 2023

View reviewed changes

crates/re_arrow_store/src/store_read.rs Outdated Show resolved Hide resolved

jparismorgan added 2 commits October 1, 2023 19:34

Add format_utc and format_range_utc, remove debug ui

b01ad23

merge with upstream/main

d317d35

Wumpf reviewed Oct 2, 2023

View reviewed changes

remove todo's and update time_string() to a match

22cd24d

jparismorgan marked this pull request as ready for review October 2, 2023 10:23

jparismorgan requested review from emilk and Wumpf October 2, 2023 10:27

emilk reviewed Oct 2, 2023

View reviewed changes

crates/re_arrow_store/src/store_format.rs Outdated Show resolved Hide resolved

Wumpf requested changes Oct 2, 2023

View reviewed changes

crates/re_log_types/src/time.rs Outdated Show resolved Hide resolved

crates/re_log_types/src/time.rs Outdated Show resolved Hide resolved

add format_time_range_utc(), improve time.rs logic

133cc13

Wumpf added enhancement New feature or request 📺 re_viewer affects re_viewer itself labels Oct 2, 2023

Wumpf added the include in changelog label Oct 2, 2023

Wumpf self-requested a review October 2, 2023 15:19

Merge branch 'main' into local-time-option

14e787c

Wumpf approved these changes Oct 2, 2023

View reviewed changes

Wumpf and others added 2 commits October 2, 2023 17:30

Merge branch 'main' into local-time-option

63b3c26

fix typo that for some reason shows up on ci only now

f3664e6

Wumpf linked an issue Oct 2, 2023 that may be closed by this pull request

Make timezone used to display time configurable #1714

Closed

add logs to investgate failing test on CI

a65ac86

remove debug logs and test_formatting_local_time_zone_no_z test

62a3a9b

Wumpf merged commit 6585d4a into rerun-io:main Oct 3, 2023
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to display timestamps in the local system timezone #3530

Add option to display timestamps in the local system timezone #3530

jparismorgan commented Sep 28, 2023 •

edited by github-actions bot

Loading

jparismorgan commented Sep 28, 2023 •

edited

Loading

emilk commented Sep 29, 2023 •

edited

Loading

Wumpf left a comment

Wumpf commented Sep 29, 2023

jparismorgan commented Sep 29, 2023

Wumpf left a comment

jparismorgan commented Oct 2, 2023

Wumpf commented Oct 2, 2023

Wumpf left a comment

Wumpf commented Oct 2, 2023

Wumpf left a comment

Wumpf commented Oct 2, 2023 •

edited

Loading

jparismorgan commented Oct 2, 2023

jprochazk commented Oct 2, 2023

Wumpf commented Oct 3, 2023

jparismorgan commented Oct 3, 2023

Wumpf commented Oct 3, 2023

Add option to display timestamps in the local system timezone #3530

Add option to display timestamps in the local system timezone #3530

Conversation

jparismorgan commented Sep 28, 2023 • edited by github-actions bot Loading

What

Checklist

jparismorgan commented Sep 28, 2023 • edited Loading

emilk commented Sep 29, 2023 • edited Loading

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf commented Sep 29, 2023

jparismorgan commented Sep 29, 2023

Wumpf left a comment

Choose a reason for hiding this comment

jparismorgan commented Oct 2, 2023

Wumpf commented Oct 2, 2023

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf commented Oct 2, 2023

Wumpf left a comment

Choose a reason for hiding this comment

Wumpf commented Oct 2, 2023 • edited Loading

jparismorgan commented Oct 2, 2023

jprochazk commented Oct 2, 2023

Wumpf commented Oct 3, 2023

jparismorgan commented Oct 3, 2023

Wumpf commented Oct 3, 2023

jparismorgan commented Sep 28, 2023 •

edited by github-actions bot

Loading

jparismorgan commented Sep 28, 2023 •

edited

Loading

emilk commented Sep 29, 2023 •

edited

Loading

Wumpf commented Oct 2, 2023 •

edited

Loading