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.

Sign up for GitHub

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

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
218d4df
fix: replace deprecated API in chrono 0.4.35 for converting to time
bconn98 Apr 4, 2024
46b1779
chore: resolve review comments + clippy error
bconn98 Jul 10, 2024
88e123e
refactor: swap to unwrap_or on get_next_time call
bconn98 Jul 10, 2024
303d009
refactor: swap to unwrap_or to utilize duration
bconn98 Jul 10, 2024
5faead7
refactor: interval error message
bconn98 Jul 10, 2024
83fd3ed
doc: reason that i64::MAX is appropriate
bconn98 Jul 11, 2024
f6bc1da
chore: cleaner error handling with Result
bconn98 Jul 13, 2024
cf7f162
ci: bump codecov to v4 and set the CODECOV token from the secrets
bconn98 Jul 13, 2024
ef197e5
Merge branch 'codecov_update' into 366_chrono_bump_fix_deprecations
bconn98 Jul 13, 2024
9acd373
chore: unwrap safe try_days call
bconn98 Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ harness = false

[dependencies]
arc-swap = "1.6"
chrono = { version = "0.4.23", optional = true, features = ["clock"], default-features = false }
chrono = { version = "0.4.35", optional = true, features = ["clock"], default-features = false }
flate2 = { version = "1.0", optional = true }
fnv = "1.0"
humantime = { version = "2.1", optional = true }
Expand Down
63 changes: 48 additions & 15 deletions src/append/rolling_file/policy/compound/trigger/time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
//!
//! Requires the `time_trigger` feature.

#[cfg(test)]
use chrono::NaiveDateTime;
use chrono::{DateTime, Datelike, Duration, Local, TimeZone, Timelike};
#[cfg(test)]
use mock_instant::{SystemTime, UNIX_EPOCH};
Expand All @@ -13,6 +11,7 @@ use serde::de;
#[cfg(feature = "config_parsing")]
use std::fmt;
use std::sync::RwLock;
use thiserror::Error;

use crate::append::rolling_file::{policy::compound::trigger::Trigger, LogFile};
#[cfg(feature = "config_parsing")]
Expand Down Expand Up @@ -73,6 +72,20 @@ impl Default for TimeTriggerInterval {
}
}

#[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),
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@estk estk Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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)
}
  1. Ensure that in the caller that TimeTriggerIntervalError is handled and the name of the appender containing it is provided to the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#[cfg(feature = "config_parsing")]
impl<'de> serde::Deserialize<'de> for TimeTriggerInterval {
fn deserialize<D>(d: D) -> Result<Self, D::Error>
Expand Down Expand Up @@ -181,18 +194,26 @@ impl TimeTrigger {
let now: std::time::Duration = SystemTime::now()
.duration_since(UNIX_EPOCH)
.expect("system time before Unix epoch");
NaiveDateTime::from_timestamp_opt(now.as_secs() as i64, now.subsec_nanos())
DateTime::from_timestamp(now.as_secs() as i64, now.subsec_nanos())
.unwrap()
.naive_local()
.and_local_timezone(Local)
.unwrap()
};

#[cfg(not(test))]
let current = Local::now();
let next_time = TimeTrigger::get_next_time(current, config.interval, config.modulate);
// In the case where bad user input results in an invalid next time, provide a valid time.
let next_time = TimeTrigger::get_next_time(current, config.interval, config.modulate)
.unwrap_or(current + Duration::try_seconds(1_i64).unwrap());
let next_roll_time = if config.max_random_delay > 0 {
let random_delay = rand::thread_rng().gen_range(0..config.max_random_delay);
next_time + Duration::seconds(random_delay as i64)
// This is a valid unwrap because chrono::Duration::try_milliseconds accepts an i64
// and we're providing a known valid value. We can trust the Option will always return
// us a valid number.
next_time
+ Duration::try_seconds(random_delay as i64)
.unwrap_or(Duration::try_milliseconds(i64::MAX).unwrap())
} else {
next_time
};
Expand All @@ -207,13 +228,13 @@ impl TimeTrigger {
current: DateTime<Local>,
interval: TimeTriggerInterval,
modulate: bool,
) -> DateTime<Local> {
) -> Result<DateTime<Local>, TimeTrigerIntervalError> {
let year = current.year();
if let TimeTriggerInterval::Year(n) = interval {
let n = n as i32;
let increment = if modulate { n - year % n } else { n };
let year_new = year + increment;
return Local.with_ymd_and_hms(year_new, 1, 1, 0, 0, 0).unwrap();
return Ok(Local.with_ymd_and_hms(year_new, 1, 1, 0, 0, 0).unwrap());
}

if let TimeTriggerInterval::Month(n) = interval {
Expand All @@ -224,9 +245,9 @@ impl TimeTrigger {
let num_months_new = num_months + increment;
let year_new = (num_months_new / 12) as i32;
let month_new = (num_months_new) % 12 + 1;
return Local
return Ok(Local
.with_ymd_and_hms(year_new, month_new, 1, 0, 0, 0)
.unwrap();
.unwrap());
}

let month = current.month();
Expand All @@ -236,14 +257,19 @@ impl TimeTrigger {
let weekday = current.weekday().num_days_from_monday() as i64; // Monday is the first day of the week
let time = Local.with_ymd_and_hms(year, month, day, 0, 0, 0).unwrap();
let increment = if modulate { n - week0 % n } else { n };
return time + Duration::weeks(increment) - Duration::days(weekday);
let dur = Duration::try_weeks(increment)
.ok_or(TimeTrigerIntervalError::Week(interval))?
- Duration::try_days(weekday).ok_or(TimeTrigerIntervalError::Day(interval))?;
return Ok(time + dur);
}

if let TimeTriggerInterval::Day(n) = interval {
let ordinal0 = current.ordinal0() as i64;
let time = Local.with_ymd_and_hms(year, month, day, 0, 0, 0).unwrap();
let increment = if modulate { n - ordinal0 % n } else { n };
return time + Duration::days(increment);
let dur =
Duration::try_days(increment).ok_or(TimeTrigerIntervalError::Day(interval))?;
return Ok(time + dur);
}

let hour = current.hour();
Expand All @@ -252,7 +278,9 @@ impl TimeTrigger {
.with_ymd_and_hms(year, month, day, hour, 0, 0)
.unwrap();
let increment = if modulate { n - (hour as i64) % n } else { n };
return time + Duration::hours(increment);
let dur =
Duration::try_hours(increment).ok_or(TimeTrigerIntervalError::Hour(interval))?;
return Ok(time + dur);
}

let min = current.minute();
Expand All @@ -261,7 +289,9 @@ impl TimeTrigger {
.with_ymd_and_hms(year, month, day, hour, min, 0)
.unwrap();
let increment = if modulate { n - (min as i64) % n } else { n };
return time + Duration::minutes(increment);
let dur = Duration::try_minutes(increment)
.ok_or(TimeTrigerIntervalError::Minute(interval))?;
return Ok(time + dur);
}

let sec = current.second();
Expand All @@ -270,7 +300,9 @@ impl TimeTrigger {
.with_ymd_and_hms(year, month, day, hour, min, sec)
.unwrap();
let increment = if modulate { n - (sec as i64) % n } else { n };
return time + Duration::seconds(increment);
let dur = Duration::try_seconds(increment)
.ok_or(TimeTrigerIntervalError::Second(interval))?;
return Ok(time + dur);
}
panic!("Should not reach here!");
}
Expand All @@ -283,8 +315,9 @@ impl Trigger for TimeTrigger {
let now = SystemTime::now()
.duration_since(UNIX_EPOCH)
.expect("system time before Unix epoch");
NaiveDateTime::from_timestamp_opt(now.as_secs() as i64, now.subsec_nanos())
DateTime::from_timestamp(now.as_secs() as i64, now.subsec_nanos())
.unwrap()
.naive_local()
.and_local_timezone(Local)
.unwrap()
};
Expand Down
Loading