tracing-appender: Introduce weekly rotation #3218
Conversation
| /// } | ||
| /// ``` | ||
| /// | ||
| /// This will result in a log file located at `/some/path/rolling.log.yyyy-MM-dd-HH`. |
There was a problem hiding this comment.
There shouldn't be an hour should there?
| /// This will result in a log file located at `/some/path/rolling.log.yyyy-MM-dd-HH`. | |
| /// This will result in a log file located at `/some/path/rolling.log.yyyy-MM-dd`. |
I'm guessing this was copied from daily above (which is also wrong), so if you'd like to fix that too...
| date.replace_date( | ||
| Date::from_ordinal_date(date.year(), rounded_ordinal).expect( | ||
| "ordinal should never be out of range; this is a bug in tracing-appender", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
I feel that this approach may be surprising to some users. It means that this year, the files will rotate on a Wednesday morning (UTC), but last year they would have rotated on a Monday.
Instead, I would suggest that we rotate on a fixed day of the week always. My preference would be on Sunday, because that's the day that cron uses for 0. This may not be what everyone wants either, but without providing a configurable option, we just have to pick a day.
We should also call out explicitly in the docs for weekly() when the rotation will occur, since I don't think that any option we choose will be obvious to everyone.
There was a problem hiding this comment.
Thanks for the review, Hayden!
I've given this some thought and I agree with you. For my particular use case, I didn't care as much about the consistency, but I agree that this is probably going to be unexpected for users.
I really like the idea of matching cron behavior here. I'll work on those changes soon and get back to you.
Thanks again!
Motivation
While configuring tracing-appender, I wanted to specify a weekly log rotation interval. I was unable to do so, as the largest rotation interval was daily.
Solution
Before my introduction of weekly log rotation, rounding the current
OffsetDateTimewas straightforward: we could simply keep the current date and truncate part or all of the time component. However, we cannot simply truncate the time with weekly rotation; the date must now be modified.To round the date, I take its ordinal (how many days have passed since the start of the year) and round it backwards to the nearest 7 day period. This gives us consistent date-times that only change weekly.