Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timezone for timestamps #12

Closed
simonwiles opened this issue Aug 30, 2023 · 11 comments · Fixed by #16
Closed

Timezone for timestamps #12

simonwiles opened this issue Aug 30, 2023 · 11 comments · Fixed by #16

Comments

@simonwiles
Copy link
Contributor

simonwiles commented Aug 30, 2023

Thank you for this tool -- it's so useful!

Bug report/feature request:

Unless I've missed something, the timestamps produced by spacer are in UTC regardless of the timezone of the system (or, indeed, anything else)? It would be great to have them follow the system, or, even better have them be configurable (I regularly work on servers in other timezones -- the logs and output I might be interested in following all use timestamps that match the timezone of the server, as they should, but it would be awesome to be able to pipe output to spacer and see spacer's timestamps in my own local timezone).

% while true; do date; sleep 2; done | spacer
Wed Aug 30 01:54:40 PM PDT 2023
2023-08-30 20:54:41 1.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Wed Aug 30 01:54:42 PM PDT 2023
2023-08-30 20:54:43 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Wed Aug 30 01:54:44 PM PDT 2023
2023-08-30 20:54:45 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Wed Aug 30 01:54:46 PM PDT 2023
2023-08-30 20:54:47 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
@samwho
Copy link
Owner

samwho commented Aug 31, 2023

You're welcome!

We do try to get the local timezone, then fall back to UTC if we can't: https://github.com/samwho/spacer/blob/main/src/main.rs#L94.

I can add the timezone to the date output and that'll tell us for sure what the timezone is. I'm not actually sure what situations would cause time-rs to not know the local timezone 🤔

@simonwiles
Copy link
Contributor Author

simonwiles commented Aug 31, 2023

Thanks :) After posting the issue I took a look at the source code and then at the rust docs and realized it was falling back to UTC, but I couldn't work out why the local timezone wasn't available to rust on any of the systems I tested on.

Then I started looking at other ways to get the local timezone (which might also support passing an arbitrary timezone as a command-line argument), but it looks like it'd have to be platform dependent. I reckon it would be possible to get something working straightforwardly enough on my Linux systems by parsing /usr/share/zoneinfo (though I'm not much of a rustacean). Presumably something similar would work for darwin, but I haven't the first idea how to approach that for Windows :)

edit: I did find this for timezone conversion: https://docs.rs/crate/tzdata/0.4.1. Doesn't address getting the local timezone, though (and yeah, it's parsing /usr/share/zoneinfo which I suppose means it doesn't work on Windows?).

@samwho
Copy link
Owner

samwho commented Aug 31, 2023

I just pushed a commit to the main branch with a potential fix. Would you be able to test it? You'll need to cargo install --git https://github.com/samwho/spacer to get the latest code.

@simonwiles
Copy link
Contributor Author

Thanks so much for the speedy update.

% while true; do date; sleep 2; done | spacer
Thu Aug 31 09:59:16 AM PDT 2023
2023-08-31 08:59:17 -08:00 1.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Thu Aug 31 09:59:18 AM PDT 2023
2023-08-31 08:59:19 -08:00 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Thu Aug 31 09:59:20 AM PDT 2023
2023-08-31 08:59:21 -08:00 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Thu Aug 31 09:59:22 AM PDT 2023
$ while true; do date; sleep 2; done | spacer
Thu 31 Aug 19:09:14 CEST 2023
2023-08-31 18:09:15 +01:00 1.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Thu 31 Aug 19:09:16 CEST 2023
2023-08-31 18:09:17 +01:00 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Thu 31 Aug 19:09:18 CEST 2023
2023-08-31 18:09:19 +01:00 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

It works, and it's an improvement I think, but it hasn't picked up the daylight savings on my Arch workstation or on my Debian and Centos servers. I'm not immediately sure what to make of that (apart from the observation that daylight savings time is an obstructive anachronism at this point -- but that's not really very helpful :)).

@samwho
Copy link
Owner

samwho commented Aug 31, 2023

Why is time so hard.

So what I think was causing it to fall back to UTC all the time was this: https://github.com/time-rs/time/blob/18cabd12bd4258622028ffa2d9a7cb935f549b8e/time/src/sys/local_offset_at/unix.rs#L144. Seems if you have multiple threads running, it will always fall back to UTC. The comment about it being conservative wasn't lying. To get around it, I fetch the local timezone prior to spawning the background thread spacer relies on.

I have no good theories about DST, though.

@samwho
Copy link
Owner

samwho commented Aug 31, 2023

I've put out https://github.com/samwho/spacer/releases/tag/v0.2 so you don't have to run from head anymore.

@simonwiles
Copy link
Contributor Author

Thanks! Tbh I'm not sure if being an hour out is better or worse than just doing UTC regardless. It's confusing either way.

I have limited experience with rust, but would something like this be suitable? https://docs.rs/chrono/latest/chrono/

What do you feel about the possibility of adding the ability to specify a timezone? Would be a killer feature (for me, at least).

@simonwiles
Copy link
Contributor Author

simonwiles commented Sep 13, 2023

Just as a quick update to this -- what I really want for myself is something like this:

$ while true; do date; sleep 2; done | spacer --timezone America/Los_Angeles
Wed 13 Sep 21:02:40 CEST 2023
2023-09-13 12:02:41 PDT 1.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Wed 13 Sep 21:02:42 CEST 2023
2023-09-13 12:02:43 PDT 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Wed 13 Sep 21:02:44 CEST 2023
2023-09-13 12:02:45 PDT 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Wed 13 Sep 21:02:46 CEST 2023
2023-09-13 12:02:47 PDT 2.0s ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
...

I have a branch that does this and works on {Arch,Debian,Centos} Linux and Mac. I've not tried it on Windows (it does build, though, so I presume it's good), and it doesn't have proper tests (the accuracy of timestamps isn't currently tested, but test could be added to do some snapshot-style testings, perhaps -- e.g. that passing --timezone America/Los_Angeles outputs a timestamp that contains either PDT or PST etc?).

I haven't PR'd it until I know how you feel about #13 -- if you'd rather not have anything to do with this that's cool (I'll use my own fork!) -- but if this is a feature you'd be interested in having and this approach looks reasonable to you, I'd be pleased to help with anything else that needs doing to make it mergeable (code could very likely be improved -- I'm not much of a rustacean!).

@samwho
Copy link
Owner

samwho commented Sep 14, 2023

@simonwiles #13 has been merged, happy to review a PR for the --timezone flag 🙂

For testing, you could add an enum variant to enum Out called SpacerWithTimezone or some such, then in the match out within fn test_output you could have an arm that asserts a timezone is present.

@samwho
Copy link
Owner

samwho commented Sep 19, 2023

@simonwiles is it everything you hoped for? 🙂

@simonwiles
Copy link
Contributor Author

Yes! I am an even happier spacerer :) 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants