fix: replace deprecated API in chrono 0.4.35 for converting to time #367

bconn98 · 2024-04-04T01:17:49Z

Replace deprecated time conversion API from chrono crate.

Deprecation occured in v0.4.35.
Work performed by @Dirreke
Separated out by @bconn98
Resolves #366

codecov-commenter · 2024-04-04T01:20:03Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.88%. Comparing base (8ab1b34) to head (9acd373).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...ppend/rolling_file/policy/compound/trigger/time.rs	88.88%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #367      +/-   ##
==========================================
+ Coverage   63.39%   63.88%   +0.49%     
==========================================
  Files          24       25       +1     
  Lines        1557     1581      +24     
==========================================
+ Hits          987     1010      +23     
- Misses        570      571       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Dirreke · 2024-04-04T06:07:25Z

and we can add this test

    #[test]
    fn trigger_large_interval() {
        let interval = TimeTriggerInterval::Second(i64::MAX);
        let file = tempfile::tempdir().unwrap();
        let logfile = LogFile {
            writer: &mut None,
            path: file.path(),
            len: 0,
        };
        let config = TimeTriggerConfig {
            interval,
            ..Default::default()
        };

        let trigger = TimeTrigger::new(config);
        let error = trigger.trigger(&logfile).unwrap_err();
        let box_dyn = Box::<dyn StdError>::from(error);
        assert_eq!(
            format!("The integer value {:?} for the specified time trigger interval is too large, it must be less than 9,223,372,036,854,775,807 seconds.", interval),
            box_dyn.to_string()
        );
    }

bconn98 · 2024-04-04T22:24:17Z

and we can add this test

@Dirreke Doesn't look like it's doable without some of your refactor. Based on needing to unwrap the error in the new method vs unwrapping in the trigger. If you see another option let me know. I messed around with catch_unwind but without a concrete error type that wasn't working for me.

estk

Looking good, added a few comments.

Basically .unwrap() is ok if there is some explanation as to why it will never happen. (I usually add a comment on the proceeding line.)

For example unwrap is fine if you are passing literals in that you know to be fine. But as soon as you're taking user input we probably need to start handling errors.

Also, just want to note that they really arent your unwraps, just, while were in here we should do the "correct" thing.

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

estk · 2024-07-09T01:51:49Z

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

                .and_local_timezone(Local)
                .unwrap()
        };

        #[cfg(not(test))]
        let current = Local::now();
-        let next_time = TimeTrigger::get_next_time(current, config.interval, config.modulate);
+        let next_time =
+            TimeTrigger::get_next_time(current, config.interval, config.modulate).unwrap();


need to remove this unwrap and handle the error

Added a match which panics on error. This can be handled more gracefully with changes from #347 which changes where get_next_time is called to a method that returns a result.

Swapped to unwrap_or with a 1 second increment

Updated new to return a Result and passed the result up

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

bconn98 · 2024-07-10T01:14:54Z

@estk it looks like Codecov wiped out your access token. Can you try and fix that?

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

Dirreke · 2024-07-10T02:17:54Z

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

+#[derive(Debug, Error)]
+enum TimeTrigerIntervalError {
+    #[error("The 'Seconds' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Second(TimeTriggerInterval),
+    #[error("The 'Minutes' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Minute(TimeTriggerInterval),
+    #[error("The 'Hours' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Hour(TimeTriggerInterval),
+    #[error("The 'Days' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Day(TimeTriggerInterval),
+    #[error("The 'Weeks' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Week(TimeTriggerInterval),
+}
+


I dont think it's necessary either. For users, all these parameters are from the item interval, so just "the interval value....." is enough imo.

I added a comment that was something to the effect of "we should add context to errors". I obviously didnt add enough context, sorry. My point was this: if a user has a huge config with many intervals we need to set up the error handling such that when surfaced to the user, they know which interval was out of bounds.

So we need to:

Simply add a position discriminant to this error, roughly:

#[derive(Debug, Error)] enum TimeTriggerIntervalError { #[error("The '{}' value specified as a time trigger is out of bounds, ensure it fits within an i64: : '{0:?}'")] OutOfBounds(Position, TimeTriggerInterval) }

Ensure that in the caller that TimeTriggerIntervalError is handled and the name of the appender containing it is provided to the user.

I don't think we can access the name anywhere close to the triggers. They're owned in the config module but are not available anywhere within the append module that I can find

Alright I've been staring at this and tried a slew of different things, none of which I think get close to what you're asking for here. I think I need some assistance.

Like I mentioned we don't have access to the appender name, and I'm not really sure what you mean by a Position discriminant. The Error already reports the type of the interval (Seconds, Minutes, Hours, etc.) and I'm not sure what else we can add of value to the user.

I dont think it's necessary to include the appender name. The incorrect interval numbers is enougn to assist users to locate the configuration. Even if they have duplicate incorrect number, they need fix them all because all these numbers will be incorrect for interval

Dirreke · 2024-07-10T02:21:59Z

Thanks for the code! ♥️♥️

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

estk · 2024-07-10T03:21:33Z

Just wanted to add some more context here. It makes sense to use failable versions of for example Duration::weeks(), the only thing is it needs to be accompanied by an error handling strategy that gracefully presents that error and provides the context for the user to fix that error.

Previously by using the panicking version of these methods the error handling is easy, it just unwinds and gives the user (optionally) a stack trace to go find what happened. Aspirationally we would be able to use the failable methods and direct the user to the data that caused the issue instead of the code that caught the issue. (This is what would be possible in your current approach.)

However, if we're going to go thru the trouble of preventing a panic, we need to ensure the usability and debuggability is at least as good as it was when we panicked. As to how we do this... idk, but I'm certain we should alert the user that their input was incorrect instead of simply selecting some arbitrary default. IMO, without holistically rethinking our approach to presenting errors to users it will be difficult to present errors well without panicking.

Dirreke · 2024-07-10T03:27:33Z

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

+#[derive(Debug, Error)]
+enum TimeTrigerIntervalError {
+    #[error("The 'Seconds' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Second(TimeTriggerInterval),
+    #[error("The 'Minutes' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Minute(TimeTriggerInterval),
+    #[error("The 'Hours' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Hour(TimeTriggerInterval),
+    #[error("The 'Days' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Day(TimeTriggerInterval),
+    #[error("The 'Weeks' time trigger interval value is out of bounds, ensure it fits within an i64: : '{0:?}'")]
+    Week(TimeTriggerInterval),
+}
+


Besides, I remember now why I put a specific number here before. The max value of these is not i64::MAX. They should be within i64::MAX after they convert to milliseconds.

Action worthy?

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

bconn98 · 2024-07-10T10:34:18Z

FWIW the get_next_time error would be one that we generate, so I think we can/do/will when I fix the IntervalError provide some data to the user.

Bigger picture, we can setup a milestone + branch to start working towards a 2.0 version that includes the API change for multiple rollers and an informative but not panicking solution.

bconn98 · 2024-07-13T02:34:09Z

Closes #367

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

SL-RU · 2024-08-11T10:41:38Z

@estk can you review please?

Daniel599 · 2024-09-20T11:53:19Z

Hi, any update regarding this PR?
Looking forward for the support of date in log format.
Thanks

bconn98 requested review from estk and gadunga as code owners April 4, 2024 01:17

bconn98 mentioned this pull request Apr 18, 2024

Refactor time.rs to make the code logic the same as others. #347

Open

estk reviewed Jul 9, 2024

View reviewed changes

This was referenced Jul 9, 2024

Fix #370, make TimeTriggerConfig pub and add examples for time_trigger #372

Open

Integration tests #358

Open

Add support for Key-Value pairs #362

Open

bconn98 and others added 4 commits July 9, 2024 21:20

fix: replace deprecated API in chrono 0.4.35 for converting to time

218d4df

chore: resolve review comments + clippy error

46b1779

refactor: swap to unwrap_or on get_next_time call

88e123e

refactor: swap to unwrap_or to utilize duration

303d009

bconn98 force-pushed the 366_chrono_bump_fix_deprecations branch from 4263205 to 303d009 Compare July 10, 2024 01:20

Dirreke reviewed Jul 10, 2024

View reviewed changes

refactor: interval error message

5faead7

estk reviewed Jul 10, 2024

View reviewed changes

src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved

Dirreke reviewed Jul 10, 2024

View reviewed changes

doc: reason that i64::MAX is appropriate

83fd3ed

bconn98 mentioned this pull request Jul 12, 2024

Support date/time in log file name #374

Open

bconn98 added 3 commits July 12, 2024 21:42

chore: cleaner error handling with Result

f6bc1da

ci: bump codecov to v4 and set the CODECOV token from the secrets

cf7f162

Merge branch 'codecov_update' into 366_chrono_bump_fix_deprecations

ef197e5

bconn98 requested review from estk and Dirreke July 13, 2024 01:56

bconn98 mentioned this pull request Jul 13, 2024

Codecov action out of date #378

Open

Dirreke reviewed Jul 13, 2024

View reviewed changes

src/append/rolling_file/policy/compound/trigger/time.rs Outdated Show resolved Hide resolved

chore: unwrap safe try_days call

9acd373

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replace deprecated API in chrono 0.4.35 for converting to time #367

fix: replace deprecated API in chrono 0.4.35 for converting to time #367

bconn98 commented Apr 4, 2024

codecov-commenter commented Apr 4, 2024 •

edited

Loading

Dirreke commented Apr 4, 2024

bconn98 commented Apr 4, 2024

estk left a comment

estk Jul 9, 2024

bconn98 Jul 10, 2024

bconn98 Jul 10, 2024 •

edited

Loading

bconn98 Jul 13, 2024

bconn98 commented Jul 10, 2024

Dirreke Jul 10, 2024

estk Jul 10, 2024 •

edited

Loading

bconn98 Jul 10, 2024

bconn98 Jul 11, 2024

Dirreke Jul 11, 2024

Dirreke commented Jul 10, 2024

estk commented Jul 10, 2024

Dirreke Jul 10, 2024

bconn98 Jul 10, 2024

bconn98 commented Jul 10, 2024

bconn98 commented Jul 13, 2024

SL-RU commented Aug 11, 2024

Daniel599 commented Sep 20, 2024

fix: replace deprecated API in chrono 0.4.35 for converting to time #367

Are you sure you want to change the base?

fix: replace deprecated API in chrono 0.4.35 for converting to time #367

Conversation

bconn98 commented Apr 4, 2024

codecov-commenter commented Apr 4, 2024 • edited Loading

Codecov Report

Dirreke commented Apr 4, 2024

bconn98 commented Apr 4, 2024

estk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bconn98 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bconn98 commented Jul 10, 2024

Choose a reason for hiding this comment

estk Jul 10, 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

Dirreke commented Jul 10, 2024

estk commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bconn98 commented Jul 10, 2024

bconn98 commented Jul 13, 2024

SL-RU commented Aug 11, 2024

Daniel599 commented Sep 20, 2024

codecov-commenter commented Apr 4, 2024 •

edited

Loading

bconn98 Jul 10, 2024 •

edited

Loading

estk Jul 10, 2024 •

edited

Loading