Disabling the Instrument Name Validation under a new feature flag

opentelemetry-sdk/Cargo.toml

@@ -58,6 +58,7 @@ experimental_metrics_periodicreader_with_async_runtime = ["metrics"]
 spec_unstable_metrics_views = ["metrics"]
 experimental_logs_batch_log_processor_with_async_runtime = ["logs"]
 experimental_trace_batch_span_processor_with_async_runtime = ["trace"]
+experimental_metrics_disable_name_validation = ["metrics"]
 
 
 [[bench]]

opentelemetry-sdk/src/metrics/meter.rs

@@ -23,13 +23,16 @@ use super::noop::NoopSyncInstrument;
 const INSTRUMENT_NAME_MAX_LENGTH: usize = 255;
 // maximum length of instrument unit name
 const INSTRUMENT_UNIT_NAME_MAX_LENGTH: usize = 63;
+#[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
 const INSTRUMENT_NAME_ALLOWED_NON_ALPHANUMERIC_CHARS: [char; 4] = ['_', '.', '-', '/'];
 
 // instrument validation error strings
 const INSTRUMENT_NAME_EMPTY: &str = "instrument name must be non-empty";
 const INSTRUMENT_NAME_LENGTH: &str = "instrument name must be less than 256 characters";
+#[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
 const INSTRUMENT_NAME_INVALID_CHAR: &str =
     "characters in instrument name must be ASCII and belong to the alphanumeric characters, '_', '.', '-' and '/'";
+#[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
 const INSTRUMENT_NAME_FIRST_ALPHABETIC: &str =
     "instrument name must start with an alphabetic character";
 const INSTRUMENT_UNIT_LENGTH: &str = "instrument unit must be less than 64 characters";
@@ -572,6 +575,33 @@ fn validate_bucket_boundaries(boundaries: &[f64]) -> MetricResult<()> {
     Ok(())
 }
 
+// Regex pattern for Windows Performance Counter names
+// #[cfg(feature = "experimental_metrics_disable_name_validation")]
+// fn is_valid_windows_perf_counter_name(name: &str) -> bool {
+//     let pattern = r"^\\[A-Za-z0-9_ ]+\\[A-Za-z0-9_ /%-]+$";
+//     let re = Regex::new(pattern).unwrap();
+//     re.is_match(name)
+// }
+
+#[cfg(feature = "experimental_metrics_disable_name_validation")]
+fn validate_instrument_name(name: &str) -> MetricResult<()> {
+    if name.is_empty() {
+        return Err(MetricError::InvalidInstrumentConfiguration(
+            INSTRUMENT_NAME_EMPTY,
+        ));
+    }
+    if name.len() > INSTRUMENT_NAME_MAX_LENGTH {
+        return Err(MetricError::InvalidInstrumentConfiguration(
+            INSTRUMENT_NAME_LENGTH,
+        ));
+    }
+
+    // No name restrictions when name validation is disabled,
+    // except for empty names and length
+    Ok(())
+}
+
+#[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
 fn validate_instrument_name(name: &str) -> MetricResult<()> {
     if name.is_empty() {
         return Err(MetricError::InvalidInstrumentConfiguration(
@@ -583,6 +613,7 @@ fn validate_instrument_name(name: &str) -> MetricResult<()> {
             INSTRUMENT_NAME_LENGTH,
         ));
     }
+
     if name.starts_with(|c: char| !c.is_ascii_alphabetic()) {
         return Err(MetricError::InvalidInstrumentConfiguration(
             INSTRUMENT_NAME_FIRST_ALPHABETIC,
@@ -676,12 +707,15 @@ mod tests {
     use crate::metrics::MetricError;
 
     use super::{
-        validate_instrument_name, validate_instrument_unit, INSTRUMENT_NAME_FIRST_ALPHABETIC,
-        INSTRUMENT_NAME_INVALID_CHAR, INSTRUMENT_NAME_LENGTH, INSTRUMENT_UNIT_INVALID_CHAR,
-        INSTRUMENT_UNIT_LENGTH,
+        validate_instrument_name, validate_instrument_unit, INSTRUMENT_NAME_EMPTY,
+        INSTRUMENT_NAME_LENGTH, INSTRUMENT_UNIT_INVALID_CHAR, INSTRUMENT_UNIT_LENGTH,
     };
 
+    #[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
+    use super::{INSTRUMENT_NAME_FIRST_ALPHABETIC, INSTRUMENT_NAME_INVALID_CHAR};
+
     #[test]
+    #[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
     fn instrument_name_validation() {
         // (name, expected error)
         let instrument_name_test_cases = vec![
@@ -694,6 +728,52 @@ mod tests {
             ("allow/slash", ""),
             ("allow_under_score", ""),
             ("allow.dots.ok", ""),
+            ("", INSTRUMENT_NAME_EMPTY),
+            ("\\allow\\slash /sec", INSTRUMENT_NAME_FIRST_ALPHABETIC),
+            ("\\allow\\$$slash /sec", INSTRUMENT_NAME_FIRST_ALPHABETIC),
+            ("Total $ Count", INSTRUMENT_NAME_INVALID_CHAR),
+            (
+                "\\test\\UsagePercent(Total) > 80%",
+                INSTRUMENT_NAME_FIRST_ALPHABETIC,
+            ),
+            ("/not / allowed", INSTRUMENT_NAME_FIRST_ALPHABETIC),
+        ];
+        for (name, expected_error) in instrument_name_test_cases {
+            let assert = |result: Result<_, MetricError>| {
+                if expected_error.is_empty() {
+                    assert!(result.is_ok());
+                } else {
+                    assert!(matches!(
+                        result.unwrap_err(),
+                        MetricError::InvalidInstrumentConfiguration(msg) if msg == expected_error
+                    ));
+                }
+            };
+
+            assert(validate_instrument_name(name).map(|_| ()));
+        }
+    }
+
+    #[test]
+    #[cfg(feature = "experimental_metrics_disable_name_validation")]
+    fn instrument_name_validation() {
+        // (name, expected error)
+        let instrument_name_test_cases = vec![
+            ("validateName", ""),
+            ("_startWithNoneAlphabet", ""),
+            ("utf8char锈", ""),
+            ("a".repeat(255).leak(), ""),
+            ("a".repeat(256).leak(), INSTRUMENT_NAME_LENGTH),
+            ("invalid name", ""),
+            ("allow/slash", ""),
+            ("allow_under_score", ""),
+            ("allow.dots.ok", ""),
+            ("", INSTRUMENT_NAME_EMPTY),
+            ("\\allow\\slash /sec", ""),
+            ("\\allow\\$$slash /sec", ""),
+            ("Total $ Count", ""),
+            ("\\test\\UsagePercent(Total) > 80%", ""),
+            ("/not / allowed", ""),
         ];
         for (name, expected_error) in instrument_name_test_cases {
             let assert = |result: Result<_, MetricError>| {

opentelemetry-sdk/src/metrics/mod.rs

@@ -132,15 +132,16 @@ mod tests {
     use std::time::Duration;
 
     // Run all tests in this mod
-    // cargo test metrics::tests --features=testing
+    // cargo test metrics::tests --features=testing,spec_unstable_metrics_views
     // Note for all tests from this point onwards in this mod:
     // "multi_thread" tokio flavor must be used else flush won't
     // be able to make progress!
 
     #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+    #[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
     async fn invalid_instrument_config_noops() {
         // Run this test with stdout enabled to see output.
-        // cargo test invalid_instrument_config_noops --features=testing -- --nocapture
+        // cargo test invalid_instrument_config_noops --features=testing,spec_unstable_metrics_views -- --nocapture
         let invalid_instrument_names = vec![
             "_startWithNoneAlphabet",
             "utf8char锈",
@@ -215,6 +216,66 @@ mod tests {
         }
     }
 
+    #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+    #[cfg(feature = "experimental_metrics_disable_name_validation")]
+    async fn valid_instrument_config_noops_with_feature_experimental_metrics_disable_name_validation(
+    ) {
+        // Run this test with stdout enabled to see output.
+        // cargo test valid_instrument_config_noops_with_feature_experimental_metrics_disable_name_validation --features "testing,spec_unstable_metrics_views,experimental_metrics_disable_name_validation" -- --nocapture
+        let invalid_instrument_names = vec![
+            "_startWithNoneAlphabet",
+            "utf8char锈",
+            "\\allow\\slash /sec",
+            "\\allow\\$$slash /sec",
+            "Total $ Count",
+            "\\test\\UsagePercent(Total) > 80%",
+            "invalid name",
+        ];
+        for name in invalid_instrument_names {
+            let test_context = TestContext::new(Temporality::Cumulative);
+            let counter = test_context.meter().u64_counter(name).build();
+            counter.add(1, &[]);
+
+            let up_down_counter = test_context.meter().i64_up_down_counter(name).build();
+            up_down_counter.add(1, &[]);
+
+            let gauge = test_context.meter().f64_gauge(name).build();
+            gauge.record(1.9, &[]);
+
+            let histogram = test_context.meter().f64_histogram(name).build();
+            histogram.record(1.0, &[]);
+
+            let _observable_counter = test_context
+                .meter()
+                .u64_observable_counter(name)
+                .with_callback(move |observer| {
+                    observer.observe(1, &[]);
+                })
+                .build();
+
+            let _observable_gauge = test_context
+                .meter()
+                .f64_observable_gauge(name)
+                .with_callback(move |observer| {
+                    observer.observe(1.0, &[]);
+                })
+                .build();
+
+            let _observable_up_down_counter = test_context
+                .meter()
+                .i64_observable_up_down_counter(name)
+                .with_callback(move |observer| {
+                    observer.observe(1, &[]);
+                })
+                .build();
+
+            test_context.flush_metrics();
+
+            // As instrument name is invalid, no metrics should be exported
+            test_context.check_no_metrics();
+        }
+    }
+
     #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
     async fn counter_aggregation_delta() {
         // Run this test with stdout enabled to see output.
@@ -2495,6 +2556,10 @@ mod tests {
                 .get_finished_metrics()
                 .expect("metrics expected to be exported"); // TODO: Need to fix InMemoryMetricExporter to return None.
 
+            #[cfg(feature = "experimental_metrics_disable_name_validation")]
+            assert!(!resource_metrics.is_empty(), "metrics should be exported"); // TODO: Update this when enhanced feature flag is added.
+
+            #[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
             assert!(resource_metrics.is_empty(), "no metrics should be exported");
         }