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

@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+### Metrics
+- Add instrument validation to `InstrumentBuilder`
+
 ## v0.18.0
 
 - API split from `opentelemetry` crate

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<Self, Error = MetricsError>,
 {
-    /// 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<S: Into<String>>(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<T> {
+        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<T> fmt::Debug for InstrumentBuilder<'_, T> {
@@ -70,3 +118,73 @@ impl<T> 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<u64>> =
+                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<u64>> =
+                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
+                );
+            }
+        }
+    }
+}

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<dyn ExportError>),
+    /// 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<T: ExportError> From<T> for MetricsError {

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())
     }
 }

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