proxy: Parse units with duration configurations #909
Conversation
It's easy to misconfigure default durations, since they're recorded as integers and converted to Durations separately. Now, all default constants that represent durations use const `Duration` instances (enabled by a recent Rust release). This fixes #905 which was caused by using the wrong time unit for the metrics retain time.
Configuration values that take durations are currently specified as time values with no units. So `600` may mean 600ms in some contexts and 10 minutes in others. In order to avoid this problem, this change now requires that configurations provide explicit units for time values such as '600ms' or 10 minutes'.
proxy/src/config.rs
Outdated
| lazy_static! { | ||
| static ref RE: Regex = Regex::new(r"(?x)^ | ||
| \s* (\d+) \s* | ||
| ( ms | msecs? | millis? | milliseconds? |
There was a problem hiding this comment.
I think we should only support the short form of each duration unit that we support: "ms", "s", "m", "h", "d", to make life easier and to make sure our testing is comprehensive.
proxy/src/config.rs
Outdated
| Some('h') => 60 * 60, | ||
| Some('d') => 60 * 60 * 24, | ||
| _ => return Err(ParseError::NotADuration), | ||
| }; |
There was a problem hiding this comment.
In particular, notice that this doesn't validate anything after the first character, which I'm sure will create problems later, if we change anything here later.
Instead, we should match unit as full strings. Then we can also merge the processing for "ms" into the match and use from_millis for everything.
proxy/src/config.rs
Outdated
| use super::*; | ||
|
|
||
| fn test_units<F: Fn(u64) -> Duration>(units: &[&str], to_duration: F) { | ||
| for v in &[1, 23, 456_789] { |
There was a problem hiding this comment.
We should include 0 here too. We should consider whether we support the value 0 at all for any of these values, and, if so, whether we require a unit for 0.
proxy/src/config.rs
Outdated
| #[test] | ||
| fn parse_invalid_unit() { | ||
| assert_eq!(parse_duration("12 moons"), Err(ParseError::NotADuration)); | ||
| } |
There was a problem hiding this comment.
We should also have a test for the input string that would cause a overflow, as well as for the maximum value that won't cause an overflow. Also negative numbers.
|
Note that this fixes #27. |
|
I imagine |
|
@briansmith these values are not currently configurable in the CLI, so no change is necessary |
Add tests for 0 and overflow values.
|
@briansmith i've removed support for all but the shortest names and i've added tests for "0" and an overflow number |
proxy/src/config.rs
Outdated
| @@ -340,6 +339,32 @@ fn parse_number<T>(s: &str) -> Result<T, ParseError> where T: FromStr { | |||
| s.parse().map_err(|_| ParseError::NotANumber) | |||
| } | |||
|
|
|||
| fn parse_duration(s: &str) -> Result<Duration, ParseError> { | |||
| use regex::Regex; | |||
| lazy_static! { | |||
There was a problem hiding this comment.
TIOLI: this use of lazy_static is overkill.
proxy/src/config.rs
Outdated
| use regex::Regex; | ||
| lazy_static! { | ||
| static ref RE: Regex = Regex::new(r"(?x)^ | ||
| \s* (\d+) \s* ( ms | s | m | h | d ) \s* |
There was a problem hiding this comment.
TIOLI: I didn't notice this the first time. IMO it is better to NOT allow whitespace between the digits and the unit.
proxy/tests/discovery.rs
Outdated
| @@ -111,7 +111,7 @@ macro_rules! generate_tests { | |||
| let mut env = config::TestEnv::new(); | |||
|
|
|||
| // set the bind timeout to 100 ms. | |||
| env.put(config::ENV_BIND_TIMEOUT, "100".to_owned()); | |||
| env.put(config::ENV_BIND_TIMEOUT, "100 ms".to_owned()); | |||
There was a problem hiding this comment.
Regardless of whether we allow spaces, let's be consistent and avoid using spaces ourselves.
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Signed-off-by: Sacha Froment <sfroment42@gmail.com>
Configuration values that take durations are currently specified as time values with no units. So `600` may mean 600ms in some contexts and 10 minutes in others. In order to avoid this problem, this change now requires that configurations provide explicit units for time values such as '600ms' or 10 minutes'. Fixes linkerd#27.
Configuration values that take durations are currently specified as
time values with no units. So
600may mean 600ms in some contexts and10 minutes in others.
In order to avoid this problem, this change now requires that
configurations provide explicit units for time values such as '600ms' or
'10 minutes'.
After this, users may set environment vars like
CONDUIT_PROXY_METRICS_RETAIN_IDLE="1h".