Disabling the Instrument Name Validation under a new feature flag (#2543)

Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Lalit Kumar Bhasin <[email protected]>

Author: 3 people

opentelemetry-sdk/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## vNext
 
+- *Feature*: Introduced a new feature flag, `experimental_metrics_disable_name_validation`, under the `opentelemetry-sdk`, which allows disabling the Instrument Name Validation. This is useful in scenarios where you need to use *special characters*, *Windows Perf Counter Wildcard Path*, or similar cases. For more details, check [#2543](https://github.com/open-telemetry/opentelemetry-rust/pull/2543).
+  > **WARNING**: While this feature provides flexibility, **be cautious** when using it, as platforms like **Prometheus** impose restrictions on metric names and labels (e.g., no spaces, capital letters, or certain special characters). Using invalid characters may result in compatibility issues or incorrect behavior. Ensure that instrument names comply with the requirements of your target platform to avoid potential problems.
+
 - *Breaking(Affects custom metric exporter authors only)* `start_time` and `time` is moved from DataPoints to aggregations (Sum, Gauge, Histogram, ExpoHistogram) see [#2377](https://github.com/open-telemetry/opentelemetry-rust/pull/2377) and [#2411](https://github.com/open-telemetry/opentelemetry-rust/pull/2411), to reduce memory.
 
 - *Breaking* `start_time` is no longer optional for `Sum` aggregation, see [#2367](https://github.com/open-telemetry/opentelemetry-rust/pull/2367), but is still optional for `Gauge` aggregation see [#2389](https://github.com/open-telemetry/opentelemetry-rust/pull/2389).

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

@@ -1,3 +1,4 @@
+#![allow(dead_code)]
 use core::fmt;
 use std::{borrow::Cow, sync::Arc};
 
@@ -23,15 +24,18 @@ 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;
+// Characters allowed in instrument name
 const INSTRUMENT_NAME_ALLOWED_NON_ALPHANUMERIC_CHARS: [char; 4] = ['_', '.', '-', '/'];
 
-// instrument validation error strings
+// instrument name 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";
 const INSTRUMENT_NAME_INVALID_CHAR: &str =
     "characters in instrument name must be ASCII and belong to the alphanumeric characters, '_', '.', '-' and '/'";
 const INSTRUMENT_NAME_FIRST_ALPHABETIC: &str =
     "instrument name must start with an alphabetic character";
+
+// instrument unit validation error strings
 const INSTRUMENT_UNIT_LENGTH: &str = "instrument unit must be less than 64 characters";
 const INSTRUMENT_UNIT_INVALID_CHAR: &str = "characters in instrument unit must be ASCII";
 
@@ -572,6 +576,13 @@ fn validate_bucket_boundaries(boundaries: &[f64]) -> MetricResult<()> {
     Ok(())
 }
 
+#[cfg(feature = "experimental_metrics_disable_name_validation")]
+fn validate_instrument_name(_name: &str) -> MetricResult<()> {
+    // No name restrictions when name validation is disabled
+    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 +594,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,
@@ -669,19 +681,21 @@ where
     }
 }
 
+#[allow(unused_imports)]
 #[cfg(test)]
 mod tests {
     use std::borrow::Cow;
 
     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_FIRST_ALPHABETIC, INSTRUMENT_NAME_INVALID_CHAR, INSTRUMENT_NAME_LENGTH,
+        INSTRUMENT_UNIT_INVALID_CHAR, INSTRUMENT_UNIT_LENGTH,
     };
 
     #[test]
+    #[cfg(not(feature = "experimental_metrics_disable_name_validation"))]
     fn instrument_name_validation() {
         // (name, expected error)
         let instrument_name_test_cases = vec![
@@ -694,6 +708,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_disabled() {
+        // (name, expected error)
+        let instrument_name_test_cases = vec![
+            ("validateName", ""),
+            ("_startWithNoneAlphabet", ""),
+            ("utf8char锈", ""),
+            ("a".repeat(255).leak(), ""),
+            ("a".repeat(256).leak(), ""),
+            ("invalid name", ""),
+            ("allow/slash", ""),
+            ("allow_under_score", ""),
+            ("allow.dots.ok", ""),
+            ("", ""),
+            ("\\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锈",
@@ -209,8 +210,99 @@ mod tests {
             histogram.record(1.9, &[]);
             test_context.flush_metrics();
 
-            // As bucket boundaires provided via advisory params are invalid, no
-            // metrics should be exported
+            // As bucket boundaries provided via advisory params are invalid,
+            // no metrics should be exported
+            test_context.check_no_metrics();
+        }
+    }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+    #[cfg(feature = "experimental_metrics_disable_name_validation")]
+    async fn valid_instrument_config_with_feature_experimental_metrics_disable_name_validation() {
+        // Run this test with stdout enabled to see output.
+        // cargo test valid_instrument_config_with_feature_experimental_metrics_disable_name_validation --all-features -- --nocapture
+        let invalid_instrument_names = vec![
+            "_startWithNoneAlphabet",
+            "utf8char锈",
+            "",
+            "a".repeat(256).leak(),
+            "\\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 are valid because of the feature flag, metrics should be exported
+            let resource_metrics = test_context
+                .exporter
+                .get_finished_metrics()
+                .expect("metrics expected to be exported");
+
+            assert!(!resource_metrics.is_empty(), "metrics should be exported");
+        }
+
+        // Ensuring that the Histograms with invalid bucket boundaries are not exported
+        // when using the feature flag
+        let invalid_bucket_boundaries = vec![
+            vec![1.0, 1.0],                          // duplicate boundaries
+            vec![1.0, 2.0, 3.0, 2.0],                // duplicate non consequent boundaries
+            vec![1.0, 2.0, 3.0, 4.0, 2.5],           // unsorted boundaries
+            vec![1.0, 2.0, 3.0, f64::INFINITY, 4.0], // boundaries with positive infinity
+            vec![1.0, 2.0, 3.0, f64::NAN],           // boundaries with NaNs
+            vec![f64::NEG_INFINITY, 2.0, 3.0],       // boundaries with negative infinity
+        ];
+        for bucket_boundaries in invalid_bucket_boundaries {
+            let test_context = TestContext::new(Temporality::Cumulative);
+            let histogram = test_context
+                .meter()
+                .f64_histogram("test")
+                .with_boundaries(bucket_boundaries)
+                .build();
+            histogram.record(1.9, &[]);
+            test_context.flush_metrics();
+
+            // As bucket boundaries provided via advisory params are invalid,
+            // no metrics should be exported
             test_context.check_no_metrics();
         }
     }