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

refactor: use jiff for timestamp #40

Merged
merged 8 commits into from
Aug 12, 2024
Merged

refactor: use jiff for timestamp #40

merged 8 commits into from
Aug 12, 2024

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Aug 11, 2024

This closes #35.

@1996fanrui if you'd like to implement something more flexible like Logback's and Log4j's PatternLayout, you can try to follow #7.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun marked this pull request as draft August 11, 2024 11:59
@tisonkun
Copy link
Contributor Author

@BurntSushi FYI this is what I'm trying to replace time and humantime with jiff. The background of BurntSushi/jiff#90.

Copy link

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Jiff uses API design to try to prevent you from making mistakes. For a Timestamp, the meaning of "day" is ambiguous. It might be reasonable to assume it's always 24 hours, but it might not. So Jiff forces you to make an explicit choice. You could switch to Zoned in the UTC time zone. Or you could just add 24.hours() to a Timestamp for each day if you want to make that assumption explicitly.

src/append/rolling_file/rotation.rs Outdated Show resolved Hide resolved
Signed-off-by: tison <wander4096@gmail.com>
src/layout/json.rs Outdated Show resolved Hide resolved
@tisonkun
Copy link
Contributor Author

@BurntSushi Thanks for all your help! Final question:

Currently, I use let time = Zoned::now().strftime("%Y-%m-%dT%H:%M:%S.%6f%:z"); for both fixing the precision alignment issue and follow RFC3339 pattern.

The question is, I don't know if jiff's default pattern, i.e., 2024-08-11T22:38:58.835466+08:00[Asia/Shanghai] follows some RFC and can be parsed with other libraries (especially libraries existing outside the Rust world). Otherwise, it's a downside or even blocker I use it as a logging default timestamp format because downstream would find it challenging to parse this tribal format if it is.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun marked this pull request as ready for review August 11, 2024 14:47
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@BurntSushi
Copy link

Currently, I use let time = Zoned::now().strftime("%Y-%m-%dT%H:%M:%S.%6f%:z"); for both fixing the precision alignment issue and follow RFC3339 pattern.

This is fine for fixing the precision for now until jiff::fmt::temporal::DateTimePrinter grows a precision option. But for RFC 3339, you'd want Timestamp::now().to_string().

The question is, I don't know if jiff's default pattern, i.e., 2024-08-11T22:38:58.835466+08:00[Asia/Shanghai] follows some RFC and can be parsed with other libraries (especially libraries existing outside the Rust world). Otherwise, it's a downside or even blocker I use it as a logging default timestamp format because downstream would find it challenging to parse this tribal format if it is.

Yes, as documented, it follows RFC 9557. This is documented in lots of places:

RFC 9557 doesn't yet have broad support, but it's supported by java.time and will be supported by Temporal in ECMAScript once that lands. It will take time to propagate to other datetime libraries. But the only other way to correctly serialize a Zoned in a way that roundtrips losslessly is to encode the time zone as some out-of-band information. So if you do want the RFC 3339 format, you can either do what you're doing, or just serialize a Timestamp. The downside of the latter approach is that it always uses Z and not an offset. The former will let you encode the offset.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 12, 2024

@BurntSushi Thanks for your detailed explaination! I'll keep the current strftime impl for now.

The downside of the latter approach is that it always uses Z and not an offset. The former will let you encode the offset.

Yes. Although humantime only accepts Z without offset, I'd prefer to deliver the offset and it's also defined part of RFC3339 - I confirm that chrono and other language's lib understand that.

Once RFC 9557 becomes more famous, I would reconsider this format. And it's always possible we add it an option in #7.

Looking forward to jiff 1.0 and I'll sooner release a logforth 0.9 for the first jiff integrated version :D

@tisonkun tisonkun merged commit 8f7d8c7 into main Aug 12, 2024
5 checks passed
@tisonkun tisonkun deleted the datetime branch August 12, 2024 01:44
@BurntSushi
Copy link

Sweet thank you!

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 this pull request may close these issues.

Support TimeZone
2 participants