Feature/add duration rotation policy #74

lioriz · 2024-09-18T19:19:52Z

Adding RotationPolicy::Duration

Rotating after defined duration (hours, minutes, seconds)
Adding flexible RotationPolicy with the option to set the rotation time for any period of time
Adding unit tests for the new policy

Issue: #73

SpriteOvO

👍 Thanks for your work on this feature! The added tests are pretty good. I just commented some change suggestions, please have a look.

By the way, the CI is failing because of the other part of the code, I'll fix it in the main branch so you don't have to worry about it for now.

SpriteOvO · 2024-09-18T22:26:54Z

spdlog/src/sink/rotating_file_sink.rs

+    /// Rotating to a new log file after given duration (greater then {0, 0, 0}) is passed.
+    Duration {
+        /// Hours to the next rotation.. Range: [0, u32::MAX].
+        hours: u32,
+        /// Minutes to the next rotation. Range: [0, 59].
+        minutes: u32,
+        /// Seconds to the next rotation. Range: [0, 59].
+        seconds: u32,
+    },


Suggested change

/// Rotating to a new log file after given duration (greater then {0, 0, 0}) is passed.

Duration {

/// Hours to the next rotation.. Range: [0, u32::MAX].

hours: u32,

/// Minutes to the next rotation. Range: [0, 59].

minutes: u32,

/// Seconds to the next rotation. Range: [0, 59].

seconds: u32,

},

/// Rotating to a new log file after a given period has elapsed.

Period(

/// Period duration. Must be longer than 1 minute.

Duration,

),

That's a little different than what I had in mind. std::time::Duration should be more appropriate here as the parameter.

Considering a user wants to rotate every week, in the current way they have to write Duration { hours: 7 * 24, minutes: 0, seconds: 0 }, if we use Duration, just a Duration::from_weeks(1) will work. Something like 1.5 hours can also be expressed as Duration::from_hours(1) + Duration::from_mins(30).

And I prefer the name "Period" because we have already use the term in Logger::set_flush_period.

SpriteOvO · 2024-09-18T22:29:07Z

spdlog/examples/02_file.rs

+    info!("this log will be written to the file `rotating_duration.log`, and the file will be rotated every hour, 2 minutes, and 3 seconds");
+
+    Ok(())
+}


I like the more detailed examples. Just a nitpick - the file should ends with a newline.

SpriteOvO · 2024-09-18T22:35:09Z

spdlog/src/sink/rotating_file_sink.rs

+            Self::Duration {
+                hours,
+                minutes,
+                seconds,
+            } => {
+                if *hours == 0 && *minutes == 0 && *seconds == 0
+                    || *minutes > 59
+                    || *seconds > 59 {
+                    return Err(format!(
+                        "policy 'duration' expect `(hours, minutes, seconds)` to be ([0, u32::MAX], [0, 59], [0, 59]) and greater then (0, 0, 0) but got ({}, {}, {})",
+                        *hours, *minutes, *seconds
+                    ));
+                }
+            }


If we use Duration as the parameter, it has already ensured the value correctness, thanks to which we don't need to check it again in our code.

But I want to return an error here if duration < Duration::mins(1), as I commented #73 (reply in thread) before, a rotating policy of less than 1 minute doesn't make much sense.

SpriteOvO · 2024-09-18T22:51:50Z

spdlog/src/sink/rotating_file_sink.rs

        };

-        if rotation_time < now {
+        if rotation_time <= now {


Makes sense, nice catch! 👍

SpriteOvO · 2024-09-19T00:10:24Z

The CI failure is fixed, you can rebase your branch to main branch once to sync the code.

lioriz1 · 2024-09-20T12:12:07Z

👍 Thanks for your work on this feature! The added tests are pretty good. I just commented some change suggestions, please have a look.

Thanks @SpriteOvO
I updated the PR according to the review, it makes sense to use Duration instead [hour, minute, second], as it looks better and uses a standard time format.

SpriteOvO

Most looks good to me now! There are just 2 tiny code issues, and some formatting issues that should be fixed by running cargo fmt --all once.

SpriteOvO · 2024-09-20T12:05:14Z

spdlog/src/sink/rotating_file_sink.rs

+fn minutes(minutes: u64) ->Duration {
+    chrono::Duration::minutes(minutes as i64).to_std().expect(format!("Failed to create Duration::minutes({}", minutes).as_str())
+}
+
+fn hours(hours: u64) -> Duration {
+    chrono::Duration::hours(hours as i64).to_std().expect(format!("Failed to create Duration::hours({}", hours).as_str())
+}
+
+fn days(days: u64) -> Duration {
+    chrono::Duration::days(days as i64).to_std().expect(format!("Failed to create Duration::days({}", days).as_str())
+}
+
+git
+fn weeks(weeks: u64) -> Duration {
+    chrono::Duration::weeks(weeks as i64).to_std().expect(format!("Failed to create Duration::weeks({}", weeks).as_str())
 }


It doesn't look like chrono::Duration needs to be involved here, std::time::Duration should be enough.

There is also an accidentally added "git" line that should be removed.

std::time::Duration doesn't have minutes, hours, and days out of the box, so I used chrono for readability.
I see two options for change:

Use a multiplication of 60 in Duratuion::from_secs()

Add dependency of fastdata (it's already imported as a dependency for fast_log), and receive Duration::from_minute, Duration::from_hour, Duration::from_day.

Let me know what you prefer.

I think multiplication of 60 in Duratuion::from_secs() is accepted, as long as we wrap each unit as a constant or function for readability.

So you want to remove those wrapper functions and just use const val with Duration::from_secs or use those wrapper functions with the const val and Duration::from_secs?

I think const val is better, because there is no function and we don't need to worry about whether they can be inlined at all.

SpriteOvO · 2024-09-20T12:07:35Z

spdlog/src/sink/rotating_file_sink.rs

@@ -102,6 +127,7 @@ struct RotatorTimePoint {
 enum TimePoint {
    Daily { hour: u32, minute: u32 },
    Hourly,
+    Period { duration: Duration },


Suggested change

Period { duration: Duration },

Period(Duration),

Since it's just a single parameter I prefer to leave it unnamed.

SpriteOvO · 2024-09-21T04:20:54Z

spdlog/examples/02_file.rs

+                duration: (chrono::Duration::hours(1)
+                    + chrono::Duration::minutes(2)
+                    + chrono::Duration::seconds(3))
+                .to_std()
+                .unwrap(),


spdlog-rs doesn't expose chrono to users, all chrono stuff are hidden under the hood, therefore all API accepts std Duration instead of chrono Duration.

Thus, chrono Duration should not appear in the examples as well. Let's change it to just using std Duration.

SpriteOvO

Just nitpicks, use more reasonable values in example for users.

SpriteOvO · 2024-09-21T06:00:36Z

spdlog/examples/02_file.rs

+    let file_sink = Arc::new(
+        RotatingFileSink::builder()
+            .base_path(path)
+            .rotation_policy(RotationPolicy::Period(Duration::from_secs(
+                60 * 60 * 2 + 60 * 4 + 5,
+            )))
+            .build()?,
+    );
+    let new_logger = Arc::new(Logger::builder().sink(file_sink).build()?);
+    spdlog::set_default_logger(new_logger);
+
+    info!("this log will be written to the file `rotating_period.log`, and the file will be rotated every hour, 2 minutes, and 3 seconds");


Suggested change

let file_sink = Arc::new(

RotatingFileSink::builder()

.base_path(path)

.rotation_policy(RotationPolicy::Period(Duration::from_secs(

60 * 60 * 2 + 60 * 4 + 5,

)))

.build()?,

);

let new_logger = Arc::new(Logger::builder().sink(file_sink).build()?);

spdlog::set_default_logger(new_logger);

info!("this log will be written to the file `rotating_period.log`, and the file will be rotated every hour, 2 minutes, and 3 seconds");

let file_sink = Arc::new(

RotatingFileSink::builder()

.base_path(path)

.rotation_policy(RotationPolicy::Period(Duration::from_secs(

60 * 90, // 90 minutes

)))

.build()?,

);

let new_logger = Arc::new(Logger::builder().sink(file_sink).build()?);

spdlog::set_default_logger(new_logger);

info!("this log will be written to the file `rotating_period.log`, and the file will be rotated every 1.5 hours");

SpriteOvO · 2024-09-21T06:03:26Z

spdlog/src/sink/rotating_file_sink.rs

+/// // Rotating periodically
+/// RotationPolicy::Period(std::time::Duration::from_secs(3 * 60 * 60 + 6));


Suggested change

/// // Rotating periodically

/// RotationPolicy::Period(std::time::Duration::from_secs(3 * 60 * 60 + 6));

/// // Rotating every 6 hour.

/// # use std::time::Duration;

/// RotationPolicy::Period(Duration::from_secs(6 * 60 * 60));

… in example for users

SpriteOvO

LGTM now! Thank you for continually improving this PR.

@Lancern Would you like to take a look at this PR?

lioriz1 · 2024-09-22T05:17:44Z

LGTM now! Thank you for continually improving this PR.

@Lancern Would you like to take a look at this PR?

Thanks,
Waiting for this one in my project

lioriz · 2024-09-30T07:35:08Z

@SpriteOvO @Lancern
Hi,
Do you need more work on this from my side for approval?

Lancern

LGTM!

SpriteOvO · 2024-09-30T14:32:52Z

@lioriz Merged! Thanks, I will release the v0.4.0 ASAP.

lioriz · 2024-10-01T05:42:50Z

@lioriz Merged! Thanks, I will release the v0.4.0 ASAP.

@SpriteOvO
Thank you for accepting my proposal and merging the PR.
Waiting for this feature.

SpriteOvO · 2024-10-07T22:48:06Z

@lioriz Hi, v0.4.0 has been released. Let me know if it works for you :)

lioriz added 5 commits September 18, 2024 21:14

Add RotationPolicy::Duration

9e64a9c

Add RotationPolicy::Duration to README

795f373

Add RotationPolicy::Duration unit tests

c822e85

Document fixes, adding RotationPolicy::Duration ranges

aacd591

Add examples for rotating file sinks

b27d5c2

SpriteOvO requested changes Sep 18, 2024

View reviewed changes

lioriz added 9 commits September 19, 2024 11:36

Add RotationPolicy::Duration

75b97c5

Add RotationPolicy::Duration to README

2230e09

Add RotationPolicy::Duration unit tests

056ed43

Document fixes, adding RotationPolicy::Duration ranges

04966ae

Add examples for rotating file sinks

d6d63b6

Align after rebase with remote

a098090

Add new line at end of file

ad8d042

Rename RotationPolicy::Duration to RotationPolicy::Period

75ce579

Change Period::duration from [hour, minute, second] to Duration

6db9177

lioriz force-pushed the feature/add-duration-rotation-policy branch from f18a4fb to 6db9177 Compare September 20, 2024 12:06

SpriteOvO reviewed Sep 20, 2024

View reviewed changes

lioriz added 2 commits September 21, 2024 06:52

Fix format and typo

ac9b5dc

Fix all format

88c536a

SpriteOvO reviewed Sep 21, 2024

View reviewed changes

lioriz added 2 commits September 21, 2024 07:24

Use unnamed parameter for a single parameter

e8ab9c9

API accepts std Duration instead of chrono Duration

03c2d47

SpriteOvO reviewed Sep 21, 2024

View reviewed changes

Use std::time::Duration instead of chrono, use more reasonable values…

73e43a6

… in example for users

lioriz force-pushed the feature/add-duration-rotation-policy branch from c4c0416 to 73e43a6 Compare September 21, 2024 15:29

Fix format

c9c617f

SpriteOvO approved these changes Sep 21, 2024

View reviewed changes

Lancern self-requested a review September 22, 2024 06:33

Lancern approved these changes Sep 30, 2024

View reviewed changes

SpriteOvO merged commit 16e259e into SpriteOvO:main Sep 30, 2024
33 checks passed

lioriz deleted the feature/add-duration-rotation-policy branch October 1, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add duration rotation policy #74

Feature/add duration rotation policy #74

lioriz commented Sep 18, 2024

SpriteOvO left a comment

SpriteOvO Sep 18, 2024 •

edited

Loading

SpriteOvO Sep 18, 2024

SpriteOvO Sep 18, 2024

SpriteOvO Sep 18, 2024

SpriteOvO commented Sep 19, 2024

lioriz1 commented Sep 20, 2024

SpriteOvO left a comment •

edited

Loading

SpriteOvO Sep 20, 2024 •

edited

Loading

lioriz1 Sep 21, 2024

SpriteOvO Sep 21, 2024 •

edited

Loading

lioriz1 Sep 21, 2024

SpriteOvO Sep 21, 2024 •

edited

Loading

SpriteOvO Sep 20, 2024

SpriteOvO Sep 21, 2024

SpriteOvO left a comment

SpriteOvO Sep 21, 2024

SpriteOvO Sep 21, 2024

SpriteOvO left a comment

lioriz1 commented Sep 22, 2024 •

edited

Loading

lioriz commented Sep 30, 2024

Lancern left a comment

SpriteOvO commented Sep 30, 2024

lioriz commented Oct 1, 2024

SpriteOvO commented Oct 7, 2024

		/// // Rotating periodically
		/// RotationPolicy::Period(std::time::Duration::from_secs(3 * 60 * 60 + 6));

-/// // Rotating periodically
-/// RotationPolicy::Period(std::time::Duration::from_secs(3 * 60 * 60 + 6));
+/// // Rotating every 6 hour.
+/// # use std::time::Duration;
+/// RotationPolicy::Period(Duration::from_secs(6 * 60 * 60));

Feature/add duration rotation policy #74

Feature/add duration rotation policy #74

Conversation

lioriz commented Sep 18, 2024

SpriteOvO left a comment

Choose a reason for hiding this comment

SpriteOvO Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpriteOvO commented Sep 19, 2024

lioriz1 commented Sep 20, 2024

SpriteOvO left a comment • edited Loading

Choose a reason for hiding this comment

SpriteOvO Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpriteOvO Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpriteOvO Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpriteOvO left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpriteOvO left a comment

Choose a reason for hiding this comment

lioriz1 commented Sep 22, 2024 • edited Loading

lioriz commented Sep 30, 2024

Lancern left a comment

Choose a reason for hiding this comment

SpriteOvO commented Sep 30, 2024

lioriz commented Oct 1, 2024

SpriteOvO commented Oct 7, 2024

SpriteOvO Sep 18, 2024 •

edited

Loading

SpriteOvO left a comment •

edited

Loading

SpriteOvO Sep 20, 2024 •

edited

Loading

SpriteOvO Sep 21, 2024 •

edited

Loading

SpriteOvO Sep 21, 2024 •

edited

Loading

lioriz1 commented Sep 22, 2024 •

edited

Loading