From cde31ec2b4d73e88fe4dc26b6057a8701658cb6f Mon Sep 17 00:00:00 2001 From: Zhongyang Wu Date: Fri, 30 Sep 2022 12:06:56 -0700 Subject: [PATCH] feat(metrics): add instrument validation to `InstrumentBuilder`. (#884) [spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#general-characteristics) describes some general rule on instrument configurations. This PR add those validation and throw errors when any of them fails. --- opentelemetry-api/CHANGELOG.md | 4 + .../src/metrics/instruments/mod.rs | 130 +++++++++++++++++- opentelemetry-api/src/metrics/mod.rs | 5 + opentelemetry-api/src/testing/trace.rs | 4 +- opentelemetry-api/src/trace/span_context.rs | 4 +- 5 files changed, 137 insertions(+), 10 deletions(-) diff --git a/opentelemetry-api/CHANGELOG.md b/opentelemetry-api/CHANGELOG.md index 46f8e4517b..a5605a9008 100644 --- a/opentelemetry-api/CHANGELOG.md +++ b/opentelemetry-api/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased +### Metrics +- Add instrument validation to `InstrumentBuilder` + ## v0.18.0 - API split from `opentelemetry` crate diff --git a/opentelemetry-api/src/metrics/instruments/mod.rs b/opentelemetry-api/src/metrics/instruments/mod.rs index e8d2d74b40..655c66e40b 100644 --- a/opentelemetry-api/src/metrics/instruments/mod.rs +++ b/opentelemetry-api/src/metrics/instruments/mod.rs @@ -8,6 +8,16 @@ pub(super) mod gauge; pub(super) mod histogram; pub(super) mod up_down_counter; +// 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 64 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"; +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"; + /// Configuration for building an instrument. pub struct InstrumentBuilder<'a, T> { meter: &'a Meter, @@ -21,7 +31,7 @@ impl<'a, T> InstrumentBuilder<'a, T> where T: TryFrom, { - /// Create a new counter builder + /// Create a new instrument builder pub(crate) fn new(meter: &'a Meter, name: String) -> Self { InstrumentBuilder { meter, @@ -32,32 +42,70 @@ where } } - /// Set the description for this counter + /// Set the description for this instrument pub fn with_description>(mut self, description: S) -> Self { self.description = Some(description.into()); self } - /// Set the unit for this counter. + /// Set the unit for this instrument. + /// + /// Unit is case sensitive(`kb` is not the same as `kB`). + /// + /// Unit must be: + /// - ASCII string + /// - No longer than 63 characters pub fn with_unit(mut self, unit: Unit) -> Self { self.unit = Some(unit); self } - /// Creates a new counter instrument. + /// Validate the instrument configuration and creates a new instrument. pub fn try_init(self) -> Result { + self.validate_instrument_config() + .map_err(MetricsError::InvalidInstrumentConfiguration)?; T::try_from(self) } - /// Creates a new counter instrument. + /// Creates a new instrument. /// /// # Panics /// - /// This function panics if the instrument cannot be created. Use try_init if you want to + /// This function panics if the instrument configuration is invalid or the instrument cannot be created. Use [`try_init`](InstrumentBuilder::try_init) if you want to /// handle errors. pub fn init(self) -> T { self.try_init().unwrap() } + + fn validate_instrument_config(&self) -> std::result::Result<(), &'static str> { + // validate instrument name + if self.name.is_empty() { + return Err(INSTRUMENT_NAME_EMPTY); + } + if self.name.len() > 63 { + return Err(INSTRUMENT_NAME_LENGTH); + } + if self.name.starts_with(|c: char| !c.is_ascii_alphabetic()) { + return Err(INSTRUMENT_NAME_FIRST_ALPHABETIC); + } + if self + .name + .contains(|c: char| !c.is_ascii_alphanumeric() && c != '_' && c != '.' && c != '-') + { + return Err(INSTRUMENT_NAME_INVALID_CHAR); + } + + // validate instrument unit + if let Some(unit) = &self.unit { + if unit.as_str().len() > 63 { + return Err(INSTRUMENT_UNIT_LENGTH); + } + if unit.as_str().contains(|c: char| !c.is_ascii()) { + return Err(INSTRUMENT_UNIT_INVALID_CHAR); + } + } + Ok(()) + } } impl fmt::Debug for InstrumentBuilder<'_, T> { @@ -70,3 +118,73 @@ impl fmt::Debug for InstrumentBuilder<'_, T> { .finish() } } + +#[cfg(test)] +mod tests { + use crate::metrics::instruments::{ + INSTRUMENT_NAME_FIRST_ALPHABETIC, INSTRUMENT_NAME_INVALID_CHAR, INSTRUMENT_NAME_LENGTH, + INSTRUMENT_UNIT_INVALID_CHAR, INSTRUMENT_UNIT_LENGTH, + }; + use crate::metrics::noop::NoopMeterCore; + use crate::metrics::{Counter, InstrumentBuilder, Unit}; + use crate::InstrumentationLibrary; + use std::sync::Arc; + + #[test] + fn test_instrument_config_validation() { + let meter = crate::metrics::Meter::new( + InstrumentationLibrary::default(), + Arc::new(NoopMeterCore::new()), + ); + // (name, expected error) + let instrument_name_test_cases = vec![ + ("validateName", ""), + ("_startWithNoneAlphabet", INSTRUMENT_NAME_FIRST_ALPHABETIC), + ("utf8char锈", INSTRUMENT_NAME_INVALID_CHAR), + ( + "a12345678901234567890123456789012345678901234567890123456789012", + "", + ), + ( + "a123456789012345678901234567890123456789012345678901234567890123", + INSTRUMENT_NAME_LENGTH, + ), + ("invalid name", INSTRUMENT_NAME_INVALID_CHAR), + ]; + for (name, expected_error) in instrument_name_test_cases { + let builder: InstrumentBuilder<'_, Counter> = + InstrumentBuilder::new(&meter, name.to_string()); + if expected_error.is_empty() { + assert!(builder.validate_instrument_config().is_ok()); + } else { + assert_eq!( + builder.validate_instrument_config().unwrap_err(), + expected_error + ); + } + } + + // (unit, expected error) + let instrument_unit_test_cases = vec![ + ( + "0123456789012345678901234567890123456789012345678901234567890123", + INSTRUMENT_UNIT_LENGTH, + ), + ("utf8char锈", INSTRUMENT_UNIT_INVALID_CHAR), + ("kb", ""), + ]; + + for (unit, expected_error) in instrument_unit_test_cases { + let builder: InstrumentBuilder<'_, Counter> = + InstrumentBuilder::new(&meter, "test".to_string()).with_unit(Unit::new(unit)); + if expected_error.is_empty() { + assert!(builder.validate_instrument_config().is_ok()); + } else { + assert_eq!( + builder.validate_instrument_config().unwrap_err(), + expected_error + ); + } + } + } +} diff --git a/opentelemetry-api/src/metrics/mod.rs b/opentelemetry-api/src/metrics/mod.rs index a2613224ed..ead65f474f 100644 --- a/opentelemetry-api/src/metrics/mod.rs +++ b/opentelemetry-api/src/metrics/mod.rs @@ -58,6 +58,11 @@ pub enum MetricsError { /// Fail to export metrics #[error("Metrics exporter {} failed with {0}", .0.exporter_name())] ExportErr(Box), + /// Invalid instrument configuration such invalid instrument name, invalid instrument description, invalid instrument unit, etc. + /// See [spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#general-characteristics) + /// for full list of requirements. + #[error("Invalid instrument configuration: {0}")] + InvalidInstrumentConfiguration(&'static str), } impl From for MetricsError { diff --git a/opentelemetry-api/src/testing/trace.rs b/opentelemetry-api/src/testing/trace.rs index 16d5e14f0b..065b056398 100644 --- a/opentelemetry-api/src/testing/trace.rs +++ b/opentelemetry-api/src/testing/trace.rs @@ -36,13 +36,13 @@ impl Span for TestSpan { /// Helper to create trace ids for testing impl TraceId { pub fn from_u128(num: u128) -> Self { - TraceId(num) + TraceId::from_bytes(num.to_be_bytes()) } } /// Helper to create span ids for testing impl SpanId { pub fn from_u64(num: u64) -> Self { - SpanId(num) + SpanId::from_bytes(num.to_be_bytes()) } } diff --git a/opentelemetry-api/src/trace/span_context.rs b/opentelemetry-api/src/trace/span_context.rs index af09e2e8af..de810f2d13 100644 --- a/opentelemetry-api/src/trace/span_context.rs +++ b/opentelemetry-api/src/trace/span_context.rs @@ -87,7 +87,7 @@ impl fmt::LowerHex for TraceFlags { /// /// The id is valid if it contains at least one non-zero byte. #[derive(Clone, PartialEq, Eq, Copy, Hash)] -pub struct TraceId(pub(crate) u128); +pub struct TraceId(u128); impl TraceId { /// Invalid trace id @@ -148,7 +148,7 @@ impl fmt::LowerHex for TraceId { /// /// The id is valid if it contains at least one non-zero byte. #[derive(Clone, PartialEq, Eq, Copy, Hash)] -pub struct SpanId(pub(crate) u64); +pub struct SpanId(u64); impl SpanId { /// Invalid span id