add a time roller #139

SSebo · 2020-03-08T07:21:20Z

There is a need when you have some thing to log which should be keep for some days, but you can not estimate the size it will be (for example user online duration). If you use the size roller, the size may be small and log will be removed. A time roller would be nice to handle this thing.

estk · 2020-03-09T17:09:25Z

Hi @SSebo ,
Thanks so much for opening this pull request!

I have some general comments:

If you would open an issue describing the problem that you are solving that would be great.
Adding a description for the PR would be helpful as well as I use those to write the changelog
Feature name should be changed from time_based_roller to time_roller to match our naming conventions so far.
Time format should be fixed to local time, and should have a fixed format so you can drop the fmt arg to time trigger
Would be great to add some tests, especially around the time_trigger as I'm fairly certain it does not do the right thing at the moment.
Everything test related should be compiled out of the final bin, so the mock_time field should be as well, and in fact its probably better as a separate fn, check out: (this blog post)[https://blog.iany.me/2019/03/how-to-mock-time-in-rust-tests-and-cargo-gotchas-we-met/]
the scale arg needs some refinement I think, maybe it should be day hour or minute?

Sorry for all the up front comments, but hopefully that helps, and thanks for the contribution!

SSebo · 2020-03-10T01:49:41Z

Thanks for your comment! I'll try to fix it.

How about rename fmt to scale in time_trigger, and valid values are day, hour, minute?
It will be helpful if you give more info about the bug in time_trigger.
I don't like the scale too, but when roller need to rm outdated log, it needs to filter valid logs, then sort. NaiveDateTime will complains if the fmt is a date (like "%Y%m%d").

SSebo · 2020-03-10T02:06:32Z

estk · 2020-03-10T02:59:31Z

One other comment would be that you should be able to do what you want using the fixed window roller. You'll just need to add an option that allows you to pass a file pattern containing a {t} "log-{t}.log"
You could actually change it around so that you could use:
{c}and {} for counts as we do currently
{d} for datetime
Check out https://docs.rs/log4rs/0.10.0/log4rs/encode/pattern/index.html for more info.
You could then pass a date formatter like so: {d(%Y-%m-%d %H:%M:%S)}

If you need more info lmk, this would substantially reduce the amount of copy pasta.

SSebo · 2020-03-10T08:50:43Z

You mean we use two trigger, but the same fixed_window roller ?
I'll have a try.

SSebo · 2020-03-11T13:58:27Z

Time roller support {d(%Y%m%d)} right now and no need to configure scale, fmt.
When user configure path in windows like C:\\SomeDir\\{d(%Y%m%d)}\\foo.log, the back slash processing in parser need a little work.
I suggest time roller separate from fixed window roller. There is a TimeBasedRollingPolicy in log4j as well.
Sorry for my copy pasta, I did some work this time to avoid lots of common code.

SSebo · 2020-03-14T22:02:26Z

@estk PTAL

SSebo · 2020-04-01T16:34:11Z

@estk Could you give me some advice ? If you think this PR is not acceptable, I can close it. That is ok. log4rs is a really good crate.

estk · 2020-04-04T06:38:11Z

Hi @SSebo ,
I really apologize for not responding earlier, life has been a bit hectic. I'll have a look in the next few days. Thanks for your patience!

estk · 2020-04-04T17:29:08Z

The first task I'd like to see here is removing the time based roller and using the fixedwindow roller in its place.

estk

There are a few things I'd like to see addressed before merge.

Time based roller: It isn't necessary, we need to use fixed_window_roller
We cant make a public interface change to trait Trigger
There is a lot of noise in this PR due to code being moved around. The reorganization needs to be split out into a separate PR. In particular: pattern/log_content and pattern/log_path should be moved back to pattern/mod.rs. To be clear, I think there is room for re-organization, but it should be in a separate cleanup PR.

estk · 2020-04-04T17:35:30Z

src/encode/pattern/parser.rs

@@ -188,6 +192,29 @@ impl<'a> Parser<'a> {
        }
        Piece::Text(&self.pattern[start..])
    }
+
+    #[cfg(target_os = "windows")]
+    fn text(&mut self, start: usize) -> Piece<'a> {


This is a public interface change, and I'd like it in a separate PR

estk · 2020-04-04T17:38:12Z

src/encode/pattern/parser.rs

+    #[cfg(target_os = "windows")]
+    fn process_double_back_slash() -> Option<Piece<'a>> {
+        Some(Piece::Text("\\"))
+    }


Why was this change made?

I found on Windows, when use set a path like C:\xxx\yyyy\log{}, parser processed is C:\\xxx\\yyy\\log{}. Currently parser not process \\ properly.

estk · 2020-04-04T17:57:44Z

src/append/rolling_file/policy/compound/trigger/time.rs

+#[serde(deny_unknown_fields)]
+pub struct TimeTriggerConfig {
+    #[serde(deserialize_with = "deserialize_unit")]
+    unit: String,


This should deserialize into the type you want not a string. I think in this case its a TimeTrigger?

Valid values of this field are 'day', 'hour' which specify how often the time trigger should be fired. Maybe unit is confusing, if so could you give me some suggestion?

src/append/rolling_file/policy/compound/mod.rs

estk · 2020-04-04T18:04:17Z

src/append/rolling_file/policy/compound/mod.rs

+use std::cell::RefCell;
+
+#[cfg(test)]
+thread_local! {


Please add a comment on why this needs to be thread_local. A assume its some Send or Sync issue, why not use an Arc?

When set_mock_time invoked, RefCell can safely mutate on thread local variable.
We can also use unsafe to make static mut. I found the test case is not stable when use unsafe.

#[cfg(test)] static mut MOCK_TIME_STR: Option<String> = None; #[cfg(test)] fn now_string(_fmt: &str) -> String { unsafe { if let Some(time_str) = &MOCK_TIME_STR { time_str.to_string() } else { panic!("set mock now_string before use") } } } #[cfg(not(test))] fn now_string(fmt: &str) -> String { Local::now().format(fmt).to_string() } #[cfg(test)] fn set_mock_time(time: &str) { // MOCK_TIME_STR.with(|cell| *cell.borrow_mut() = Some(time.to_owned())); unsafe { MOCK_TIME_STR = Some(time.to_owned()); } }

src/append/rolling_file/policy/compound/trigger/.#time.rs

src/append/rolling_file/policy/compound/trigger/mod.rs

src/append/rolling_file/policy/compound/trigger/time.rs

estk · 2020-04-04T18:15:27Z

src/append/rolling_file/policy/compound/trigger/time.rs

+    fn trigger() {
+        set_mock_time("2020-03-07");
+        let trigger = TimeTrigger::new("%Y%m%d");
+        assert_eq!(false, trigger.trigger(Option::None).unwrap());


You can use the tempfile crate to mock log files, I'd also like to see at least one test that is not using the mock time stuff.

The time function is replaced under #[cfg(test)]. If we use #[cfg(all(test, mock_time))] on test mod, we need to add config settings when we run cargo test. Any suggestions?

estk · 2020-04-04T18:22:19Z

Additionally merging in master would be great, I've improved some CI things. Thanks @SSebo !

estk · 2021-04-14T15:51:12Z

@SSebo seems like there are still conflicts here, I'm going to close this for now. If its something you'd like to wrap up feel free to reopen.

SSebo changed the title ~~add a time base appender~~ add a time based appender Mar 8, 2020

SSebo force-pushed the master branch 6 times, most recently from 301218d to 3fa1f45 Compare March 9, 2020 09:09

estk added the WIP Work In Progress label Mar 9, 2020

SSebo changed the title ~~add a time based appender~~ add a time roller Mar 10, 2020

Add a time roller

d4f239d

SSebo force-pushed the master branch from 3fa1f45 to d4f239d Compare March 10, 2020 01:52

SSebo added 4 commits March 10, 2020 12:08

Refine time mock

d8e03ff

Add test for time trigger

2be08e2

Remove time roller scale field

605ac9e

Merge branch 'master' of github.com:estk/log4rs

59236aa

SSebo force-pushed the master branch from 13c8d94 to 9f6b9be Compare March 11, 2020 13:47

Time roller support {d(%Y%m%d)}

2b29b09

SSebo force-pushed the master branch from 9f6b9be to 2b29b09 Compare March 11, 2020 13:48

Fix windows path parsing error

44434d2

SSebo force-pushed the master branch from 4b8a3c1 to 44434d2 Compare March 11, 2020 16:00

estk reviewed Apr 4, 2020

View reviewed changes

SSebo force-pushed the master branch 4 times, most recently from 0ffb83a to 6c49c48 Compare April 5, 2020 00:43

revert public api change, tidy code

53eec71

SSebo force-pushed the master branch from 6c49c48 to 53eec71 Compare April 5, 2020 00:43

estk closed this Apr 14, 2021

estk mentioned this pull request Apr 19, 2022

can add trigger kind "time" in rolling_file #235

Closed

Dirreke mentioned this pull request Dec 14, 2022

add trigger kind "time" in rolling_file #296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a time roller #139

add a time roller #139

SSebo commented Mar 8, 2020 •

edited

estk commented Mar 9, 2020

SSebo commented Mar 10, 2020 •

edited

SSebo commented Mar 10, 2020 •

edited

estk commented Mar 10, 2020

SSebo commented Mar 10, 2020

SSebo commented Mar 11, 2020 •

edited

SSebo commented Mar 14, 2020

SSebo commented Apr 1, 2020

estk commented Apr 4, 2020

estk commented Apr 4, 2020

estk left a comment

estk Apr 4, 2020

estk Apr 4, 2020

SSebo Apr 4, 2020 •

edited

estk Apr 4, 2020

SSebo Apr 4, 2020

estk Apr 4, 2020

SSebo Apr 4, 2020 •

edited

estk Apr 4, 2020

SSebo Apr 5, 2020 •

edited

estk commented Apr 4, 2020

estk commented Apr 14, 2021

add a time roller #139

add a time roller #139

Conversation

SSebo commented Mar 8, 2020 • edited

estk commented Mar 9, 2020

SSebo commented Mar 10, 2020 • edited

SSebo commented Mar 10, 2020 • edited

estk commented Mar 10, 2020

SSebo commented Mar 10, 2020

SSebo commented Mar 11, 2020 • edited

SSebo commented Mar 14, 2020

SSebo commented Apr 1, 2020

estk commented Apr 4, 2020

estk commented Apr 4, 2020

estk left a comment

Choose a reason for hiding this comment

estk Apr 4, 2020

Choose a reason for hiding this comment

estk Apr 4, 2020

Choose a reason for hiding this comment

SSebo Apr 4, 2020 • edited

Choose a reason for hiding this comment

estk Apr 4, 2020

Choose a reason for hiding this comment

SSebo Apr 4, 2020

Choose a reason for hiding this comment

estk Apr 4, 2020

Choose a reason for hiding this comment

SSebo Apr 4, 2020 • edited

Choose a reason for hiding this comment

estk Apr 4, 2020

Choose a reason for hiding this comment

SSebo Apr 5, 2020 • edited

Choose a reason for hiding this comment

estk commented Apr 4, 2020

estk commented Apr 14, 2021

SSebo commented Mar 8, 2020 •

edited

SSebo commented Mar 10, 2020 •

edited

SSebo commented Mar 10, 2020 •

edited

SSebo commented Mar 11, 2020 •

edited

SSebo Apr 4, 2020 •

edited

SSebo Apr 4, 2020 •

edited

SSebo Apr 5, 2020 •

edited