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;
@@ -413,13 +412,21 @@ impl TrippyConfig {
         let interface = cfg_layer_opt(args.interface, cfg_file_strategy.interface);
         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(),
+            cfg_file_strategy
+                .min_round_duration
+                .as_deref()
+                .map(humantime::parse_duration)
+                .transpose()?,
+            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(),
+            cfg_file_strategy
+                .max_round_duration
+                .as_deref()
+                .map(humantime::parse_duration)
+                .transpose()?,
+            defaults::DEFAULT_STRATEGY_MAX_ROUND_DURATION,
         );
         let initial_sequence = cfg_layer(
             args.initial_sequence,
@@ -433,8 +440,12 @@ impl TrippyConfig {
         );
         let grace_duration = cfg_layer(
             args.grace_duration,
-            cfg_file_strategy.grace_duration,
-            format_duration(defaults::DEFAULT_STRATEGY_GRACE_DURATION).to_string(),
+            cfg_file_strategy
+                .grace_duration
+                .as_deref()
+                .map(humantime::parse_duration)
+                .transpose()?,
+            defaults::DEFAULT_STRATEGY_GRACE_DURATION,
         );
         let max_inflight = cfg_layer(
             args.max_inflight,
@@ -478,8 +489,12 @@ impl TrippyConfig {
         };
         let read_timeout = cfg_layer(
             args.read_timeout,
-            cfg_file_strategy.read_timeout,
-            format_duration(defaults::DEFAULT_STRATEGY_READ_TIMEOUT).to_string(),
+            cfg_file_strategy
+                .read_timeout
+                .as_deref()
+                .map(humantime::parse_duration)
+                .transpose()?,
+            defaults::DEFAULT_STRATEGY_READ_TIMEOUT,
         );
         let tui_max_samples = cfg_layer(
             args.tui_max_samples,
@@ -498,8 +513,12 @@ 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),
+            cfg_file_tui
+                .tui_refresh_rate
+                .as_deref()
+                .map(humantime::parse_duration)
+                .transpose()?,
+            duration(constants::DEFAULT_TUI_REFRESH_RATE),
         );
         let tui_privacy_max_ttl = cfg_layer(
             args.tui_privacy_max_ttl,
@@ -545,8 +564,12 @@ impl TrippyConfig {
         );
         let dns_timeout = cfg_layer(
             args.dns_timeout,
-            cfg_file_dns.dns_timeout,
-            String::from(constants::DEFAULT_DNS_TIMEOUT),
+            cfg_file_dns
+                .dns_timeout
+                .as_deref()
+                .map(humantime::parse_duration)
+                .transpose()?,
+            duration(constants::DEFAULT_DNS_TIMEOUT),
         );
         let report_cycles = cfg_layer(
             args.report_cycles,
@@ -559,10 +582,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 +641,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
@@ -1280,11 +1297,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 +1311,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 +1354,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 +1401,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 +1457,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)")
+}