appender: refactor limit-by-size logic to remove the log index tracking #1
Conversation
adrianbenavides
force-pushed
the
adrian/appender/limit-by-size
branch
from
6c25c9a
to
a0ec6bd
Compare
adrianbenavides
force-pushed
the
adrian/appender/limit-by-size
branch
3 times, most recently
from
b8aa293
to
7a3c14a
Compare
|
@CBenoit I pushed some changes adding your proposals 👍 |
adrianbenavides
force-pushed
the
adrian/appender/limit-by-size
branch
2 times, most recently
from
60cda8e
to
7856ebc
Compare
|
Ready! Thanks a lot @CBenoit for the review 🙏 |
tracing-appender/src/rolling.rs
Outdated
| let should_rollover = |now: OffsetDateTime| -> bool { | ||
| if let Some(current_time) = self.state.should_rollover_by_date(now) { | ||
| let did_cas = self.state.advance_date(now, current_time); | ||
| debug_assert!(did_cas, "if we have &mut access to the appender, no other thread can have advanced the timestamp..."); | ||
| return true; | ||
| } | ||
| if self.state.should_rollover_by_size() { | ||
| self.state.advance_same_date_filename_index(); | ||
| return true; | ||
| } | ||
| false | ||
| }; | ||
|
|
||
| let now = self.now(); | ||
| let should_rollover = should_rollover(now); |
There was a problem hiding this comment.
style: It looks like we don’t need to use a closure at this place. This is making the code less readable in my opinion. We also don’t actually need to short-circuit the code using return because if-blocks are also expressions:
| let should_rollover = |now: OffsetDateTime| -> bool { | |
| if let Some(current_time) = self.state.should_rollover_by_date(now) { | |
| let did_cas = self.state.advance_date(now, current_time); | |
| debug_assert!(did_cas, "if we have &mut access to the appender, no other thread can have advanced the timestamp..."); | |
| return true; | |
| } | |
| if self.state.should_rollover_by_size() { | |
| self.state.advance_same_date_filename_index(); | |
| return true; | |
| } | |
| false | |
| }; | |
| let now = self.now(); | |
| let should_rollover = should_rollover(now); | |
| let now = self.now(); | |
| let should_rollover = if let Some(current_time) = self.state.should_rollover_by_date(now) { | |
| let did_cas = self.state.advance_date(now, current_time); | |
| debug_assert!(did_cas, "if we have &mut access to the appender, no other thread can have advanced the timestamp..."); | |
| true | |
| } else if self.state.should_rollover_by_size() { | |
| self.state.advance_same_date_filename_index(); | |
| true | |
| } else { | |
| false | |
| }; |
There was a problem hiding this comment.
True, this is a leftover from a previous version where I needed the writer inside the should_rollover logic, and I added the closure to handle the writer lock.
tracing-appender/src/rolling.rs
Outdated
| let should_rollover = |now: OffsetDateTime| -> bool { | ||
| if let Some(current_time) = self.state.should_rollover_by_date(now) { | ||
| // Did we get the right to lock the file? If not, another thread | ||
| // did it and we can just make a writer. | ||
| return self.state.advance_date(now, current_time); | ||
| } | ||
| if self.state.should_rollover_by_size() { | ||
| self.state.advance_same_date_filename_index(); | ||
| return true; | ||
| } | ||
| false | ||
| }; | ||
|
|
||
| let now = self.now(); | ||
| if should_rollover(now) { |
tracing-appender/src/rolling.rs
Outdated
| /// This will result in a log file located at `/some/path/rolling.log.yyyy-MM-dd`. | ||
| /// This will result in a log file located at `/some/path/rolling.log.yyyy-MM-dd-HH`. | ||
| pub fn daily( |
There was a problem hiding this comment.
issue: Wait, I don’t think daily is supposed to produce files with hours in the name… Did we accidentally changed the behavior?
There was a problem hiding this comment.
My bad, not sure why I added that change there.
In any case, I've just double-checked and it all works as expected, nothing changed in this regard.
tracing-appender/src/rolling.rs
Outdated
| #[derive(Clone, Copy, Eq, PartialEq, Debug)] | ||
| #[derive(Clone, Eq, PartialEq, Debug)] |
There was a problem hiding this comment.
nitpick: Not a big deal, but this kind of enum should implement Copy
tracing-appender/src/rolling.rs
Outdated
| let created = metadata.created().ok()?; | ||
| Some((entry, created)) |
There was a problem hiding this comment.
issue: It’s better to use the date parsed from the filename, because the created date in the metadata of the file could be different and cause strange behaviors. From instance, when copying files around for, e.g.: backups. Some backup tools may preserve the created date, but definitely not all tools are doing so. We would start deleting old files in a pseudo-random order (the order in which they were transferred and re-created). My suggestion is to keep parsing the filename for the date instead, and sort accordingly. This is the least surprising behavior I think.
There was a problem hiding this comment.
Got it, now I see why you made that change in this function. I'll add your code back.
There was a problem hiding this comment.
Done, I think I got it right. In the current implementation we shouldn't need the metadata.created() for anything right? Or would you use it as a fallback in case we can't parse the filename as a date? I don't think this case will ever happen, as we use the same date format that was used to create the log file in the first place.
There was a problem hiding this comment.
Yes, at least my code was completely ignoring files not following the format, considering them as not managed by the logger. I think that’s the appropriate behavior.
adrianbenavides
force-pushed
the
adrian/appender/limit-by-size
branch
from
7856ebc
to
9ccf7c8
Compare
|
Thank you! Since there are many changes, I’ll try this patch locally before merging, but things are looking good🙂 |
Refactor the limit-by-size logic as described in this comment.
This function summarizes the new logic
Additionally, I've tried to remove unnecessary code to focus the PR on the new filter logic and make the reviewer's job easier.