feat: improve command line error reporting for durations (#1150)

crates/trippy/src/config.rs

@@ -2,7 +2,6 @@ use anyhow::anyhow;
 use clap::ValueEnum;
 use clap_complete::Shell;
 use file::ConfigFile;
-use humantime::format_duration;
 use itertools::Itertools;
 use serde::Deserialize;
 use std::collections::HashMap;
@@ -414,12 +413,12 @@ impl TrippyConfig {
         let min_round_duration = cfg_layer(
             args.min_round_duration,
             cfg_file_strategy.min_round_duration,
-            format_duration(defaults::DEFAULT_STRATEGY_MIN_ROUND_DURATION).to_string(),
+            defaults::DEFAULT_STRATEGY_MIN_ROUND_DURATION,
         );
         let max_round_duration = cfg_layer(
             args.max_round_duration,
             cfg_file_strategy.max_round_duration,
-            format_duration(defaults::DEFAULT_STRATEGY_MAX_ROUND_DURATION).to_string(),
+            defaults::DEFAULT_STRATEGY_MAX_ROUND_DURATION,
         );
         let initial_sequence = cfg_layer(
             args.initial_sequence,
@@ -434,7 +433,7 @@ impl TrippyConfig {
         let grace_duration = cfg_layer(
             args.grace_duration,
             cfg_file_strategy.grace_duration,
-            format_duration(defaults::DEFAULT_STRATEGY_GRACE_DURATION).to_string(),
+            defaults::DEFAULT_STRATEGY_GRACE_DURATION,
         );
         let max_inflight = cfg_layer(
             args.max_inflight,
@@ -479,7 +478,7 @@ impl TrippyConfig {
         let read_timeout = cfg_layer(
             args.read_timeout,
             cfg_file_strategy.read_timeout,
-            format_duration(defaults::DEFAULT_STRATEGY_READ_TIMEOUT).to_string(),
+            defaults::DEFAULT_STRATEGY_READ_TIMEOUT,
         );
         let tui_max_samples = cfg_layer(
             args.tui_max_samples,
@@ -499,7 +498,7 @@ impl TrippyConfig {
         let tui_refresh_rate = cfg_layer(
             args.tui_refresh_rate,
             cfg_file_tui.tui_refresh_rate,
-            String::from(constants::DEFAULT_TUI_REFRESH_RATE),
+            constants::DEFAULT_TUI_REFRESH_RATE,
         );
         let tui_privacy_max_ttl = cfg_layer(
             args.tui_privacy_max_ttl,
@@ -546,7 +545,7 @@ impl TrippyConfig {
         let dns_timeout = cfg_layer(
             args.dns_timeout,
             cfg_file_dns.dns_timeout,
-            String::from(constants::DEFAULT_DNS_TIMEOUT),
+            constants::DEFAULT_DNS_TIMEOUT,
         );
         let report_cycles = cfg_layer(
             args.report_cycles,
@@ -559,10 +558,6 @@ impl TrippyConfig {
             (false, false, false, ProtocolConfig::Tcp) | (_, true, _, _) => Protocol::Tcp,
             (false, false, false, ProtocolConfig::Icmp) | (_, _, true, _) => Protocol::Icmp,
         };
-        let read_timeout = humantime::parse_duration(&read_timeout)?;
-        let min_round_duration = humantime::parse_duration(&min_round_duration)?;
-        let max_round_duration = humantime::parse_duration(&max_round_duration)?;
-        let grace_duration = humantime::parse_duration(&grace_duration)?;
         let source_addr = source_address
             .as_ref()
             .map(|addr| {
@@ -622,14 +617,12 @@ impl TrippyConfig {
                 ));
             }
         };
-        let tui_refresh_rate = humantime::parse_duration(&tui_refresh_rate)?;
         let dns_resolve_method = match dns_resolve_method_config {
             DnsResolveMethodConfig::System => ResolveMethod::System,
             DnsResolveMethodConfig::Resolv => ResolveMethod::Resolv,
             DnsResolveMethodConfig::Google => ResolveMethod::Google,
             DnsResolveMethodConfig::Cloudflare => ResolveMethod::Cloudflare,
         };
-        let dns_timeout = humantime::parse_duration(&dns_timeout)?;
         let max_rounds = match mode {
             Mode::Stream | Mode::Tui => None,
             Mode::Pretty
@@ -744,13 +737,13 @@ impl Default for TrippyConfig {
             interface: None,
             multipath_strategy: defaults::DEFAULT_STRATEGY_MULTIPATH,
             port_direction: PortDirection::None,
-            dns_timeout: duration(constants::DEFAULT_DNS_TIMEOUT),
+            dns_timeout: constants::DEFAULT_DNS_TIMEOUT,
             dns_resolve_method: dns_resolve_method(constants::DEFAULT_DNS_RESOLVE_METHOD),
             dns_lookup_as_info: constants::DEFAULT_DNS_LOOKUP_AS_INFO,
             tui_max_samples: constants::DEFAULT_TUI_MAX_SAMPLES,
             tui_max_flows: constants::DEFAULT_TUI_MAX_FLOWS,
             tui_preserve_screen: constants::DEFAULT_TUI_PRESERVE_SCREEN,
-            tui_refresh_rate: duration(constants::DEFAULT_TUI_REFRESH_RATE),
+            tui_refresh_rate: constants::DEFAULT_TUI_REFRESH_RATE,
             tui_privacy_max_ttl: constants::DEFAULT_TUI_PRIVACY_MAX_TTL,
             tui_address_mode: constants::DEFAULT_TUI_ADDRESS_MODE,
             tui_as_mode: constants::DEFAULT_TUI_AS_MODE,
@@ -774,10 +767,6 @@ impl Default for TrippyConfig {
     }
 }
 
-fn duration(duration: &str) -> Duration {
-    humantime::parse_duration(duration).expect("valid duration")
-}
-
 fn dns_resolve_method(dns_resolve_method: DnsResolveMethodConfig) -> ResolveMethod {
     match dns_resolve_method {
         DnsResolveMethodConfig::System => ResolveMethod::System,
@@ -1280,11 +1269,11 @@ mod tests {
     #[test_case("trip example.com", Ok(cfg().min_round_duration(Duration::from_millis(1000)).build()); "default min round duration")]
     #[test_case("trip example.com --min-round-duration 250ms", Ok(cfg().min_round_duration(Duration::from_millis(250)).build()); "custom min round duration")]
     #[test_case("trip example.com -i 250ms", Ok(cfg().min_round_duration(Duration::from_millis(250)).build()); "custom min round duration short")]
-    #[test_case("trip example.com --min-round-duration 0", Err(anyhow!("time unit needed, for example 0sec or 0ms")); "invalid format min round duration")]
+    #[test_case("trip example.com --min-round-duration 0", Err(anyhow!("error: invalid value '0' for '--min-round-duration <MIN_ROUND_DURATION>': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format min round duration")]
     #[test_case("trip example.com", Ok(cfg().min_round_duration(Duration::from_millis(1000)).build()); "default max round duration")]
     #[test_case("trip example.com --max-round-duration 1250ms", Ok(cfg().max_round_duration(Duration::from_millis(1250)).build()); "custom max round duration")]
     #[test_case("trip example.com -T 2s", Ok(cfg().max_round_duration(Duration::from_millis(2000)).build()); "custom max round duration short")]
-    #[test_case("trip example.com --min-round-duration 0", Err(anyhow!("time unit needed, for example 0sec or 0ms")); "invalid format max round duration")]
+    #[test_case("trip example.com --max-round-duration 0", Err(anyhow!("error: invalid value '0' for '--max-round-duration <MAX_ROUND_DURATION>': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format max round duration")]
     #[test_case("trip example.com -i 250ms -T 250ms", Ok(cfg().min_round_duration(Duration::from_millis(250)).max_round_duration(Duration::from_millis(250)).build()); "custom min and max round duration")]
     #[test_case("trip example.com -i 300ms -T 250ms", Err(anyhow!("max-round-duration (250ms) must not be less than min-round-duration (300ms)")); "min round duration greater than max")]
     fn test_round_duration(cmd: &str, expected: anyhow::Result<TrippyConfig>) {
@@ -1294,7 +1283,7 @@ mod tests {
     #[test_case("trip example.com", Ok(cfg().grace_duration(Duration::from_millis(100)).build()); "default grace duration")]
     #[test_case("trip example.com --grace-duration 10ms", Ok(cfg().grace_duration(Duration::from_millis(10)).build()); "custom grace duration")]
     #[test_case("trip example.com -g 50ms", Ok(cfg().grace_duration(Duration::from_millis(50)).build()); "custom grace duration short")]
-    #[test_case("trip example.com --grace-duration 0", Err(anyhow!("time unit needed, for example 0sec or 0ms")); "invalid format grace duration")]
+    #[test_case("trip example.com --grace-duration 0", Err(anyhow!("error: invalid value '0' for '--grace-duration <GRACE_DURATION>': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format grace duration")]
     #[test_case("trip example.com --grace-duration 9ms", Err(anyhow!("grace-duration (9ms) must be between 10ms and 1s inclusive")); "invalid low grace duration")]
     #[test_case("trip example.com --grace-duration 1001ms", Err(anyhow!("grace-duration (1.001s) must be between 10ms and 1s inclusive")); "invalid high grace duration")]
     fn test_grace_duration(cmd: &str, expected: anyhow::Result<TrippyConfig>) {
@@ -1337,7 +1326,7 @@ mod tests {
 
     #[test_case("trip example.com", Ok(cfg().read_timeout(Duration::from_millis(10)).build()); "default read timeout")]
     #[test_case("trip example.com --read-timeout 20ms", Ok(cfg().read_timeout(Duration::from_millis(20)).build()); "custom read timeout")]
-    #[test_case("trip example.com --read-timeout 20", Err(anyhow!("time unit needed, for example 20sec or 20ms")); "invalid custom read timeout")]
+    #[test_case("trip example.com --read-timeout 20", Err(anyhow!("error: invalid value '20' for '--read-timeout <READ_TIMEOUT>': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid custom read timeout")]
     #[test_case("trip example.com --read-timeout 9ms", Err(anyhow!("read-timeout (9ms) must be between 10ms and 100ms inclusive")); "invalid low custom read timeout")]
     #[test_case("trip example.com --read-timeout 101ms", Err(anyhow!("read-timeout (101ms) must be between 10ms and 100ms inclusive")); "invalid high custom read timeout")]
     fn test_read_timeout(cmd: &str, expected: anyhow::Result<TrippyConfig>) {
@@ -1384,7 +1373,7 @@ mod tests {
 
     #[test_case("trip example.com", Ok(cfg().dns_timeout(Duration::from_millis(5000)).build()); "default dns timeout")]
     #[test_case("trip example.com --dns-timeout 20ms", Ok(cfg().dns_timeout(Duration::from_millis(20)).build()); "custom dns timeout")]
-    #[test_case("trip example.com --dns-timeout 20", Err(anyhow!("time unit needed, for example 20sec or 20ms")); "invalid custom dns timeout")]
+    #[test_case("trip example.com --dns-timeout 20", Err(anyhow!("error: invalid value '20' for '--dns-timeout <DNS_TIMEOUT>': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid custom dns timeout")]
     fn test_dns_timeout(cmd: &str, expected: anyhow::Result<TrippyConfig>) {
         compare(parse_config(cmd), expected);
     }
@@ -1440,7 +1429,7 @@ mod tests {
     #[test_case("trip example.com --tui-refresh-rate 200ms", Ok(cfg().tui_refresh_rate(Duration::from_millis(200)).build()); "custom tui refresh rate")]
     #[test_case("trip example.com --tui-refresh-rate 49ms", Err(anyhow!("tui-refresh-rate (49ms) must be between 50ms and 1s inclusive")); "invalid low tui refresh rate")]
     #[test_case("trip example.com --tui-refresh-rate 1001ms", Err(anyhow!("tui-refresh-rate (1.001s) must be between 50ms and 1s inclusive")); "invalid high tui refresh rate")]
-    #[test_case("trip example.com --tui-refresh-rate foo", Err(anyhow!("expected number at 0")); "invalid format tui refresh rate")]
+    #[test_case("trip example.com --tui-refresh-rate foo", Err(anyhow!("error: invalid value 'foo' for '--tui-refresh-rate <TUI_REFRESH_RATE>': expected time unit (i.e. 100ms, 2s, 1000us) For more information, try '--help'.")); "invalid format tui refresh rate")]
     fn test_tui_refresh_rate(cmd: &str, expected: anyhow::Result<TrippyConfig>) {
         compare(parse_config(cmd), expected);
     }

crates/trippy/src/config/cmd.rs

@@ -5,10 +5,11 @@ use crate::config::{
     LogFormat, LogSpanEvents, Mode, MultipathStrategyConfig, ProtocolConfig, TuiColor,
     TuiKeyBinding,
 };
-use anyhow::anyhow;
+use anyhow::{anyhow, Context};
 use clap::builder::Styles;
 use clap::Parser;
 use clap_complete::Shell;
+use std::time::Duration;
 
 /// Trace a route to a host and record statistics
 #[allow(clippy::doc_markdown)]
@@ -101,17 +102,17 @@ pub struct Args {
     pub interface: Option<String>,
 
     /// The minimum duration of every round [default: 1s]
-    #[arg(short = 'i', long)]
-    pub min_round_duration: Option<String>,
+    #[arg(short = 'i', long, value_parser = parse_duration)]
+    pub min_round_duration: Option<Duration>,
 
     /// The maximum duration of every round [default: 1s]
-    #[arg(short = 'T', long)]
-    pub max_round_duration: Option<String>,
+    #[arg(short = 'T', long, value_parser = parse_duration)]
+    pub max_round_duration: Option<Duration>,
 
     /// The period of time to wait for additional ICMP responses after the target has responded
     /// [default: 100ms]
-    #[arg(short = 'g', long)]
-    pub grace_duration: Option<String>,
+    #[arg(short = 'g', long, value_parser = parse_duration)]
+    pub grace_duration: Option<Duration>,
 
     /// The initial sequence number [default: 33000]
     #[arg(long)]
@@ -150,8 +151,8 @@ pub struct Args {
     pub icmp_extensions: bool,
 
     /// The socket read timeout [default: 10ms]
-    #[arg(long)]
-    pub read_timeout: Option<String>,
+    #[arg(long, value_parser = parse_duration)]
+    pub read_timeout: Option<Duration>,
 
     /// How to perform DNS queries [default: system]
     #[arg(value_enum, short = 'r', long)]
@@ -162,8 +163,8 @@ pub struct Args {
     pub dns_resolve_all: bool,
 
     /// The maximum time to wait to perform DNS queries [default: 5s]
-    #[arg(long)]
-    pub dns_timeout: Option<String>,
+    #[arg(long, value_parser = parse_duration)]
+    pub dns_timeout: Option<Duration>,
 
     /// Lookup autonomous system (AS) information during DNS queries [default: false]
     #[arg(long, short = 'z')]
@@ -206,8 +207,8 @@ pub struct Args {
     pub tui_preserve_screen: bool,
 
     /// The Tui refresh rate [default: 100ms]
-    #[arg(long)]
-    pub tui_refresh_rate: Option<String>,
+    #[arg(long, value_parser = parse_duration)]
+    pub tui_refresh_rate: Option<Duration>,
 
     /// The maximum ttl of hops which will be masked for privacy [default: 0]
     #[arg(long)]
@@ -283,3 +284,7 @@ fn parse_tui_binding_value(value: &str) -> anyhow::Result<(TuiCommandItem, TuiKe
     let binding = TuiKeyBinding::try_from(&value[pos + 1..])?;
     Ok((item, binding))
 }
+
+fn parse_duration(value: &str) -> anyhow::Result<Duration> {
+    humantime::parse_duration(value).context("expected time unit (i.e. 100ms, 2s, 1000us)")
+}

crates/trippy/src/config/constants.rs

@@ -53,7 +53,7 @@ pub const DEFAULT_TUI_MAX_ADDRS: u8 = 0;
 pub const DEFAULT_TUI_ADDRESS_MODE: AddressMode = AddressMode::Host;
 
 /// The default value for `tui-refresh-rate`.
-pub const DEFAULT_TUI_REFRESH_RATE: &str = "100ms";
+pub const DEFAULT_TUI_REFRESH_RATE: Duration = Duration::from_millis(100);
 
 /// The default value for `tui-privacy-max-ttl`.
 pub const DEFAULT_TUI_PRIVACY_MAX_TTL: u8 = 0;
@@ -68,7 +68,7 @@ pub const DEFAULT_ADDR_FAMILY: AddressFamilyConfig = AddressFamilyConfig::Ipv4Th
 pub const DEFAULT_DNS_LOOKUP_AS_INFO: bool = false;
 
 /// The default value for `dns-timeout`.
-pub const DEFAULT_DNS_TIMEOUT: &str = "5s";
+pub const DEFAULT_DNS_TIMEOUT: Duration = Duration::from_millis(5000);
 
 /// The default value for `report-cycles`.
 pub const DEFAULT_REPORT_CYCLES: usize = 10;