diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index e1d608f681..d840a5bfc4 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/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). diff --git a/opentelemetry-sdk/Cargo.toml b/opentelemetry-sdk/Cargo.toml index 8d4d6da1ed..56491c6ac2 100644 --- a/opentelemetry-sdk/Cargo.toml +++ b/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]] diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index e644623e34..b1d3bd6f4f 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/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,6 +681,7 @@ where } } +#[allow(unused_imports)] #[cfg(test)] mod tests { use std::borrow::Cow; @@ -676,12 +689,13 @@ 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_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>| { diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 798db18acb..7712c30b85 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/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(); } }