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

add trigger kind "time" in rolling_file #296

Merged
merged 11 commits into from Feb 2, 2024
Merged

Conversation

Dirreke
Copy link
Contributor

@Dirreke Dirreke commented Dec 14, 2022

I add the trigger kind "time" in rolling_file. It can be set with limit like 5second, 10minute, 1day, 3weeks...It may be able to solve the Issue #235. I referred to #139 and thank for @SSebo

@Dirreke
Copy link
Contributor Author

Dirreke commented Dec 17, 2022

I modified the trigger logic again. Now, assuming the trigger time is 2 days, a new log file will be generated at 0:00 every other day, even though the first trigger may be less than 2 days. The same applies to others.
it can achieve cover the feature of #295

@Sandvoxel
Copy link

With time based trigger you might want to add the pattern for the window to allow the time and date the file was created.

@Dirreke Dirreke force-pushed the time-triger branch 4 times, most recently from fbb654b to 8965543 Compare November 30, 2023 11:10
examples/sample_config.yml Show resolved Hide resolved
examples/sample_config.yml Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
src/append/rolling_file/mod.rs Outdated Show resolved Hide resolved
src/append/rolling_file/mod.rs Outdated Show resolved Hide resolved
@estk
Copy link
Owner

estk commented Dec 2, 2023

@Dirreke mention me here when ready for a new review @bconn98 big shoutout for the assist and driving this forward.

@bconn98 bconn98 mentioned this pull request Dec 2, 2023
@Dirreke Dirreke mentioned this pull request Dec 4, 2023
@estk
Copy link
Owner

estk commented Dec 4, 2023

I suggest that whether the log get written pre or post roll should be configurable. Lets make the logic dependent on the roller.

@Dirreke Dirreke requested a review from gadunga as a code owner December 4, 2023 12:14
@Dirreke
Copy link
Contributor Author

Dirreke commented Dec 4, 2023

Doc wasn't competed yet.

@Dirreke
Copy link
Contributor Author

Dirreke commented Dec 4, 2023

I added a config item in roller called is_pre_process(I haven't figured out the name yet) to detemine if the file should roll pre or post getting written. What do you think about these codes?

@Dirreke Dirreke force-pushed the time-triger branch 3 times, most recently from 9cb39f7 to 859bf93 Compare December 6, 2023 14:12
@Dirreke
Copy link
Contributor Author

Dirreke commented Dec 6, 2023

@estk @bconn98

@Dirreke Dirreke requested a review from estk December 6, 2023 14:16
@Dirreke Dirreke force-pushed the time-triger branch 2 times, most recently from 6878bf6 to dddbcf3 Compare January 24, 2024 14:59
@Dirreke
Copy link
Contributor Author

Dirreke commented Jan 24, 2024

The changes are as follows

  • Change limit to interval`.
  • Add modulate and max_random_delay parameters and implement them.
  • change the trigger logic to be same witj log4j.

I don't use the humantime because it sets year to 365.25 days and sets month to 30.43 days and I don't like it. I set they both to the actual number of days but not average.

@Dirreke Dirreke requested a review from bconn98 January 24, 2024 15:25
docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved
src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved
src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved
src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved
src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved
src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved
Dirreke and others added 5 commits January 27, 2024 14:07
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
@Dirreke Dirreke force-pushed the time-triger branch 2 times, most recently from 6c11f4d to 58b779d Compare January 27, 2024 07:36
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
@Dirreke
Copy link
Contributor Author

Dirreke commented Jan 27, 2024

There are two differences between log4rs and log4j.

  • In log4j, the first day of the week is Sunday. But in log4rs, the first day of the week will be Monday , which is the same as chrono crate and the ISO 8601 standard.
  • The unit used by log4rs is the pattern string in the file name. But in log4rs, I have to keep using the unit of the interval because we don't have a date/time pattern yet.

Any thoughts?

@bconn98
Copy link
Collaborator

bconn98 commented Jan 27, 2024

I'm good with both of those trade offs

@bconn98
Copy link
Collaborator

bconn98 commented Jan 29, 2024

I'll re-review tomorrow

docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved
Co-authored-by: Bryan Conn <30739012+bconn98@users.noreply.github.com>
@bconn98
Copy link
Collaborator

bconn98 commented Feb 2, 2024

@Dirreke, hey the MSRV needs to get bumped to support the toml crate. Here is a commit making the same change.

@estk estk merged commit 6c6ace0 into estk:main Feb 2, 2024
12 checks passed
@Dirreke
Copy link
Contributor Author

Dirreke commented Feb 10, 2024

We can close #289 #235 #309 now.

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.

None yet

6 participants