Update time crate to 0.3.5 #5761
Conversation
There was a problem hiding this comment.
This also switches the timestamp format from local timezone to UTC since getting the local timezone is fallible. Rather than complicating the code with a fallback to UTC, just use UTC all the time.
You make it sound like this is somehow an improvement, but the code is significantly more complex and performs worse. Is the motivation solely to bump the version number of the dependency?
The main motivation is bumping the version, yes. 0.1 is in maintenance mode and rather than add more things that depend on it to Debian, I thought I would propose a change to update to the maintained time version.
I didn't think a different way of describing the format would be considered significantly more complex. I see that RFC3339 format is one of the well-known formats, so that line could be changed to let now = OffsetDateTime::now_utc().format(&well_known::Rfc3339).unwrap();
The parsing of the format string is done at compile time, and the actual formatting of the time object to the resulting output string is, from what I can tell, the same between 0.1 and 0.3. Is there a general performance difference between the two versions of the crate or was this something measured? If it's the latter, I would be glad to know how I should be verifying such issues. |
Fair enough, that's probably one of the best arguments one could have come up with. While version 0.1 of the time crate might be in "maintenance mode", which also is perfectly fine since Alacritty does not need any new features, afaik a lot of crates still depend on it. I think the significant majority still depends on 0.2 iirc. Do you have any insight on the distribution of crates relevant for Debian packaging that depend on the different versions? Is the majority actually on 0.3?
While neither of them are monumentally complicated implementations, one certainly is more complex than the other. Especially if we consider that the
Excuse my impreciseness here, my comment about performance was not actually about performance, but about the way it performs. You've mentioned it's switched from local time to UTC time which is a significant UX regression as far as I'm concerned.
Correct me if I'm wrong, but isn't that format "1985-04-12T23:20:50.52Z"? If so that's inferior to our current format both in readability and preciseness (which has been significant with some issues in the past). |
From a quick glance, it looks like we only have a few crates depending on time at all. 1 was the impetus for packaging 0.3, 4 still use 0.1, 1 has dropped the dependency upstream but we haven't pulled that in yet, and 1 has ported to 0.2.
Fair. I had never actually encountered the abbreviation formats you were using before, even though I'm familiar with the classic strftime formats.
:) For most things I'm involved in, times are expected to be in UTC so that biased me. It is possible to get the local timezone with time 0.3.5 ... with some caveats, which is why it's fallible. I figured for a logging system, being infallible was more important.
Yes, the Rather than using the DSL, there's a trait that can be implemented to format the time... On the balance, it seems like this isn't a change that fits. Feel free to close it, if so. The previous attempt I found to update to 0.2 solely mentioned the mass of dependencies as an issue, so when I saw that wasn't an issue with 0.3 I thought I'd give it a shot. If nothing else, I learned a bit more than I wanted about dealing with time in Rust and I have something to point to if there are questions about updating the crate in the future. |
I'd assume we're not talking about sporadic failure here? So it shouldn't be that big of a problem. And failure should of course be handled and mitigated. But these kinds of issues are exactly why I haven't migrated in the past, with the 0.1 crate our "ideal" solution was straight-forward to implement without ever having to read any documentation, getting the same exact behavior with 0.3 is just a headache. Of course one could go for all sorts of compromises, but to me that just seems rather unfortunate for something that should be so simple (and has been with 0.1 of the crate).
That surprises me, but I guess the RFC does suggest it's "at least 1" digit, not exactly two. Good to know though, thanks.
Certainly all of that is possible and one of these things can likely be used to migrate in this PR. I've just been holding off until now because I'm rather frustrated with time's API progression.
It's probably mostly an issue of just sooner or later accepting the disadvantages of the new API and moving on. I personally don't need any migration, I'm perfectly fine with keeping it as 0.1 forever, but as you've pointed out sooner or later distributions packaging Alacritty would likely prefer having more recent dependencies. So since that gives us a good reason to upgrade, it probably makes sense to do so. I'd say the biggest issue with this PR is the UTC time. I don't like to confuse users looking at logs and I don't particularly like to be confused myself. Since this is an application expected to run on the user's desktop, not some kind of server that might be in a different timezone, I see no good reason why we would have our logs in UTC format. So if you can get that figured out while keeping our current format, I think we can begrudgingly ignore the other issues. |
I've addressed this by caching, via lazy_static, the local TZ at startup and then using that offset to adjust the timestamp when logging. |
1e37ab3 to
d3ffa0f
Compare
alacritty/src/logging.rs
Outdated
| logfile, | ||
| stdout, | ||
| event_proxy: Mutex::new(event_proxy), | ||
| tz_offset: UtcOffset::current_local_offset().unwrap(), |
There was a problem hiding this comment.
I'd like an expect rather than unwrap here.
alacritty/src/logging.rs
Outdated
| let now = OffsetDateTime::now_utc() | ||
| .to_offset(tz_offset) | ||
| .format(format_description!( | ||
| "[year]-[month]-[day] [hour repr:24]:[minute]:[second].[subsecond digits:9]Z" |
There was a problem hiding this comment.
| "[year]-[month]-[day] [hour repr:24]:[minute]:[second].[subsecond digits:9]Z" | |
| "[year]-[month]-[day] [hour repr:24]:[minute]:[second].[subsecond digits:9]" |
Not UTC anymore.
alacritty/src/logging.rs
Outdated
| fn create_log_message(record: &log::Record<'_>, target: &str, tz_offset: UtcOffset) -> String { | ||
| let now = OffsetDateTime::now_utc() | ||
| .to_offset(tz_offset) | ||
| .format(format_description!( |
There was a problem hiding this comment.
If you move this format_description! out of the parameter, it'll clean stuff up:
let time_format = format_description!(
"[year]-[month]-[day] [hour repr:24]:[minute]:[second].[subsecond digits:9]"
);
let now = OffsetDateTime::now_utc().to_offset(tz_offset).format(time_format).unwrap();Due to unsoundness issues (c.f., time-rs/time#380 and time-rs/time#293), determining the local timezone can only happen while single-threaded. Determine the timezone early in startup and apply the offset to the UTC timestamp before formatting.
Due to unsoundness issues (c.f., time-rs/time#380 and time-rs/time#293), determining the local timezone can only happen while single-threaded.
Determine the timezone early in startup and apply the offset to the UTC timestamp before formatting.