Reworked parsing of the command for disabled metrics

bin/fuel-core/Cargo.toml

@@ -54,6 +54,7 @@ itertools = { workspace = true }
 pretty_assertions = { workspace = true }
 rand = { workspace = true }
 serde = { workspace = true }
+strum = { workspace = true }
 tempfile = { workspace = true }
 test-case = { workspace = true }
 

bin/fuel-core/src/cli/run.rs

@@ -51,7 +51,10 @@ use fuel_core_chain_config::{
     SnapshotMetadata,
     SnapshotReader,
 };
-use fuel_core_metrics::config::Module;
+use fuel_core_metrics::config::{
+    DisableConfig,
+    Module,
+};
 use fuel_core_poa::signer::SignMode;
 use fuel_core_types::blockchain::header::StateTransitionBytecodeVersion;
 use pyroscope::{
@@ -237,8 +240,8 @@ pub struct Command {
     #[cfg(feature = "p2p")]
     pub sync_args: p2p::SyncArgs,
 
-    #[arg(long = "disable-metrics", value_delimiter = ',', help = fuel_core_metrics::config::help_string(), default_value = "", env)]
-    pub metrics: fuel_core_metrics::config::Config,
+    #[arg(long = "disable-metrics", value_delimiter = ',', help = fuel_core_metrics::config::help_string(), env)]
+    pub disabled_metrics: Vec<Module>,
 
     #[clap(long = "verify-max-da-lag", default_value = "10", env)]
     pub max_da_lag: u64,
@@ -295,7 +298,7 @@ impl Command {
             p2p_args,
             #[cfg(feature = "p2p")]
             sync_args,
-            metrics,
+            disabled_metrics: metrics,
             max_da_lag,
             max_wait_time,
             tx_pool,
@@ -306,7 +309,7 @@ impl Command {
             profiling: _,
         } = self;
 
-        info!("Metrics config: {}", metrics);
+        info!("Metrics config: {:?}", metrics);
 
         let addr = net::SocketAddr::new(graphql.ip, graphql.port);
 
@@ -665,3 +668,85 @@ fn start_pyroscope_agent(
         })
         .transpose()
 }
+
+#[cfg(test)]
+#[allow(non_snake_case)]
+#[allow(clippy::bool_assert_comparison)]
+mod tests {
+    use super::*;
+    use strum::IntoEnumIterator;
+
+    fn parse_command(args: &[&str]) -> anyhow::Result<Command> {
+        Ok(Command::try_parse_from([""].iter().chain(args))?)
+    }
+
+    #[test]
+    fn parse_disabled_metrics__no_value_enables_everything() {
+        // Given
+        let args = [];
+
+        // When
+        let command = parse_command(&args).unwrap();
+
+        // Then
+        let config = command.disabled_metrics;
+        Module::iter().for_each(|module| {
+            assert_eq!(config.is_enabled(module), true);
+        });
+    }
+
+    #[test]
+    fn parse_disabled_metrics__all() {
+        // Given
+        let args = ["--disable-metrics", "all"];
+
+        // When
+        let command = parse_command(&args).unwrap();
+
+        // Then
+        let config = command.disabled_metrics;
+        Module::iter().for_each(|module| {
+            assert_eq!(config.is_enabled(module), false);
+        });
+    }
+
+    #[test]
+    fn parse_disabled_metrics__mixed_args() {
+        // Given
+        let args = [
+            "--disable-metrics",
+            "txpool,importer",
+            "--disable-metrics",
+            "graphql",
+        ];
+
+        // When
+        let command = parse_command(&args).unwrap();
+
+        // Then
+        let config = command.disabled_metrics;
+        assert_eq!(config.is_enabled(Module::TxPool), false);
+        assert_eq!(config.is_enabled(Module::Importer), false);
+        assert_eq!(config.is_enabled(Module::GraphQL), false);
+        assert_eq!(config.is_enabled(Module::P2P), true);
+        assert_eq!(config.is_enabled(Module::Producer), true);
+    }
+
+    #[test]
+    fn parse_disabled_metrics__bad_values() {
+        // Given
+        let args = ["--disable-metrics", "txpool,alpha,bravo"];
+
+        // When
+        let command = parse_command(&args);
+
+        // Then
+        let err = command.expect_err("should fail to parse");
+        assert_eq!(
+            err.to_string(),
+            "error: invalid value 'alpha' for \
+            '--disable-metrics <DISABLED_METRICS>': Matching variant not found\
+            \n\nFor more information, try '--help'.\n"
+        );
+    }
+}

crates/metrics/Cargo.toml

@@ -11,7 +11,6 @@ repository = { workspace = true }
 description = "Fuel metrics"
 
 [dependencies]
-derive_more = { workspace = true }
 once_cell = { workspace = true }
 parking_lot = { workspace = true }
 pin-project-lite = { workspace = true }

crates/metrics/src/config.rs

@@ -6,58 +6,26 @@ use strum_macros::{
     EnumString,
 };
 
-#[derive(Debug, derive_more::Display)]
-pub enum ConfigCreationError {
-    #[display(fmt = "No such module: {}", _0)]
-    UnknownModule(String),
-}
-
 #[derive(Debug, Display, Clone, Copy, PartialEq, EnumString, EnumIter)]
 #[strum(serialize_all = "lowercase")]
 pub enum Module {
+    All,
     Importer,
     P2P,
     Producer,
     TxPool, /* TODO[RC]: Not used. Add support in https://github.com/FuelLabs/fuel-core/pull/2321 */
     GraphQL, // TODO[RC]: Not used... yet.
 }
 
-#[derive(Debug, Clone, Default)]
-pub struct Config(Vec<Module>);
-impl Config {
-    pub fn is_enabled(&self, module: Module) -> bool {
-        !self.0.contains(&module)
-    }
-}
-
-impl std::fmt::Display for Config {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let disabled_modules = self
-            .0
-            .iter()
-            .map(|module| module.to_string())
-            .collect::<Vec<_>>();
-        write!(f, "Disable modules: {}", disabled_modules.join(", "))
-    }
+/// Configuration for disabling metrics.
+pub trait DisableConfig {
+    /// Returns `true` if the given module is enabled.
+    fn is_enabled(&self, module: Module) -> bool;
 }
 
-impl std::str::FromStr for Config {
-    // TODO: Figure out how to make `clap` work directly with `ConfigCreationError`
-    type Err = String;
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        if s == "all" || s.is_empty() {
-            return Ok(Self(Module::iter().collect()))
-        }
-        let mut modules = vec![];
-
-        for token in s.split(',') {
-            let parsed_module = token.parse::<Module>().map_err(|_| {
-                ConfigCreationError::UnknownModule(token.to_string()).to_string()
-            })?;
-            modules.push(parsed_module);
-        }
-
-        Ok(Self(modules))
+impl DisableConfig for Vec<Module> {
+    fn is_enabled(&self, module: Module) -> bool {
+        !self.contains(&module) && !self.contains(&Module::All)
     }
 }
 
@@ -72,67 +40,3 @@ static HELP_STRING: Lazy<String> = Lazy::new(|| {
 pub fn help_string() -> &'static str {
     &HELP_STRING
 }
-
-#[cfg(test)]
-mod tests {
-    use std::str::FromStr;
-
-    use strum::IntoEnumIterator;
-
-    use crate::config::{
-        Config,
-        Module,
-    };
-
-    fn assert_config(config: &Config, expected_disabled: Vec<Module>) {
-        expected_disabled
-            .iter()
-            .for_each(|module| assert!(!config.is_enabled(*module)));
-
-        let expected_enabled: Vec<_> = Module::iter()
-            .filter(|module| !expected_disabled.contains(module))
-            .collect();
-        expected_enabled
-            .iter()
-            .for_each(|module| assert!(config.is_enabled(*module)));
-    }
-
-    #[test]
-    fn metrics_config() {
-        const EXCLUDED_METRICS: &str = "importer,txpool";
-
-        let expected_disabled = [Module::Importer, Module::TxPool].to_vec();
-
-        let config = Config::from_str(EXCLUDED_METRICS).expect("should create config");
-        assert_config(&config, expected_disabled);
-    }
-
-    #[test]
-    fn metrics_config_with_incorrect_values() {
-        const EXCLUDED_METRICS: &str = "txpool,alpha,bravo";
-
-        let config = Config::from_str(EXCLUDED_METRICS);
-        assert!(matches!(config, Err(err) if err == "No such module: alpha"));
-    }
-
-    #[test]
-    fn metrics_config_with_empty_value() {
-        // This case is still possible if someone calls `--disable-metrics ""`
-        const EXCLUDED_METRICS: &str = "";
-
-        let expected_disabled = Module::iter().collect::<Vec<_>>();
-
-        let config = Config::from_str(EXCLUDED_METRICS).expect("should create config");
-        assert_config(&config, expected_disabled);
-    }
-
-    #[test]
-    fn metrics_config_supports_all() {
-        const EXCLUDED_METRICS: &str = "all";
-
-        let expected_disabled = Module::iter().collect::<Vec<_>>();
-
-        let config = Config::from_str(EXCLUDED_METRICS).expect("should create config");
-        assert_config(&config, expected_disabled);
-    }
-}