Describe metric with unit and description

Signed-off-by: Ludvig Liljenberg <[email protected]>

Author: ludfjig

src/hyperlight_host/src/func/guest_err.rs

@@ -18,7 +18,7 @@ use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode;
 
 use crate::error::HyperlightError::{GuestError, OutBHandlingError, StackOverflow};
 use crate::mem::shared_mem::HostSharedMemory;
-use crate::metrics::{CounterMetric, Metric};
+use crate::metrics::{CounterMetric, EmittableMetric};
 use crate::sandbox::mem_mgr::MemMgrWrapper;
 use crate::{log_then_return, Result};
 /// Check for a guest error and return an `Err` if one was found,

src/hyperlight_host/src/hypervisor/mod.rs

@@ -19,7 +19,7 @@ use tracing::{instrument, Span};
 
 use crate::error::HyperlightError::ExecutionCanceledByHost;
 use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags};
-use crate::metrics::{CounterMetric, Metric};
+use crate::metrics::{CounterMetric, EmittableMetric};
 use crate::{log_then_return, new_error, HyperlightError, Result};
 
 /// Util for handling x87 fpu state

src/hyperlight_host/src/metrics/metrics_macro.rs

@@ -1,21 +1,33 @@
+/// Crate internal trait for defining metrics.
 pub(crate) trait NamedMetric {
-    /// Returns the name of the metric as a static string
+    /// The name of the metric.
     fn name(&self) -> &'static str;
+    /// The description of the metric.
+    fn description(&self) -> &'static str;
+    /// The unit of the metric.
+    fn unit(&self) -> metrics::Unit;
 }
 
 /// A macro to define metrics with variants and optional fields.
 #[macro_export]
 macro_rules! define_metrics {
     (
-        $( $metric_type:ident $( < $( $gen:tt ),+ > )? {
-            $(
-                $( #[cfg($feature:meta)] )?
-                $variant:ident $( { $($field_name:ident : $field_ty:ty),* $(,)? } )? => $name:expr
-            ),* $(,)?
-        } )*
+        $(
+            $metric_type:ident $( < $( $gen:tt ),+ > )? {
+                $(
+                    $( #[cfg($feature:meta)] )?
+                    $variant:ident $( { $($field_name:ident : $field_ty:ty),* $(,)? } )? => {
+                        name: $name:expr,
+                        description: $description:expr,
+                        unit: $unit:expr $(,)?
+                    }
+                ),* $(,)?
+            }
+        )*
     ) => {
         $(
             #[derive(Debug, Clone)]
+            #[allow(dead_code)]
             pub(crate) enum $metric_type $( < $( $gen ),+ > )? {
                 $(
                     $( #[cfg($feature)] )?
@@ -32,6 +44,24 @@ macro_rules! define_metrics {
                         )*
                     }
                 }
+
+                fn description(&self) -> &'static str {
+                    match self {
+                        $(
+                            $( #[cfg($feature)] )?
+                            Self::$variant { .. } => $description,
+                        )*
+                    }
+                }
+
+                fn unit(&self) -> metrics::Unit {
+                    match self {
+                        $(
+                            $( #[cfg($feature)] )?
+                            Self::$variant { .. } => $unit,
+                        )*
+                    }
+                }
             }
         )*
     };

src/hyperlight_host/src/metrics/mod.rs

@@ -1,6 +1,5 @@
+use std::sync::Once;
 use std::time::Duration;
-#[cfg(feature = "function_call_metrics")]
-use std::time::Instant;
 
 use metrics_macro::NamedMetric;
 
@@ -10,33 +9,58 @@ mod metrics_macro;
 // These defines all types of metrics in this crate
 define_metrics! {
     CounterMetric {
-        GuestErrors { code: u64} => "guest_errors_total",
-        GuestCancellations => "guest_cancellations_total",
+        GuestErrors { code: u64 } => {
+            name: "guest_errors_total",
+            description: "Number of errors encountered by guests",
+            unit: metrics::Unit::Count,
+        },
+        GuestCancellations => {
+            name: "guest_cancellations_total",
+            description: "Number of times guest execution was cancelled due to timeout",
+            unit: metrics::Unit::Count,
+        },
     }
     HistogramMetric {
-        GuestCallDuration { name: String, duration: Duration } => "guest_call_duration_seconds",
-        HostCallDuration { name: String, duration: Duration} => "host_call_duration_seconds",
+        GuestCallDuration { name: String, duration: Duration } => {
+            name: "guest_call_duration_seconds",
+            description: "Duration of guest function calls",
+            unit: metrics::Unit::Seconds,
+        },
+        HostCallDuration { name: String, duration: Duration} => {
+            name: "host_call_duration_seconds",
+            description: "Duration of host function calls",
+            unit: metrics::Unit::Seconds,
+        },
     }
 }
 
 impl CounterMetric {
     /// Create a new guest error metric, ready to be emitted
     #[must_use]
-    pub(crate) fn guest_error(code: u64) -> CounterMetric {
+    pub(crate) fn guest_error(code: u64) -> Self {
         CounterMetric::GuestErrors { code }
     }
     /// Create a new guest cancellation metric, ready to be emitted
     #[must_use]
-    pub(crate) fn guest_cancellation() -> CounterMetric {
+    pub(crate) fn guest_cancellation() -> Self {
         CounterMetric::GuestCancellations
     }
 }
 
 impl HistogramMetric {
-    /// Time a guest call and emit its duration as a metric
-    pub(crate) fn time_and_emit_guest_call<T, F: FnOnce() -> T>(name: String, f: F) -> T {
+    /// Measures the time to execute the given closure, and then emits the duration
+    /// as a guest call metric.
+    ///
+    /// Note: If the `function_call_metrics` feature is not enabled, this function
+    /// will simply execute the closure without measuring time or emitting metrics.
+    pub(crate) fn time_and_emit_guest_call<T, F: FnOnce() -> T>(
+        #[allow(unused_variables)] name: String,
+        f: F,
+    ) -> T {
         cfg_if::cfg_if! {
             if #[cfg(feature = "function_call_metrics")] {
+                use std::time::Instant;
+
                 let start = Instant::now();
                 let result = f();
                 let duration = start.elapsed();
@@ -48,10 +72,19 @@ impl HistogramMetric {
         }
     }
 
-    /// Time a host call and emit its duration as a metric
-    pub(crate) fn time_and_emit_host_call<T, F: FnOnce() -> T>(name: String, f: F) -> T {
+    /// Measures the time to execute the given closure, and then emits the duration
+    /// as a host call metric.
+    ///
+    /// Note: If the `function_call_metrics` feature is not enabled, this function
+    /// will simply execute the closure without measuring time or emitting metrics.
+    pub(crate) fn time_and_emit_host_call<T, F: FnOnce() -> T>(
+        #[allow(unused_variables)] name: String,
+        f: F,
+    ) -> T {
         cfg_if::cfg_if! {
             if #[cfg(feature = "function_call_metrics")] {
+                use std::time::Instant;
+
                 let start = Instant::now();
                 let result = f();
                 let duration = start.elapsed();
@@ -78,36 +111,68 @@ impl HistogramMetric {
 }
 
 /// A metric which can be emitted to the underlying metrics system
-pub(crate) trait Metric {
-    /// Emits the metric to the underlying metrics system
+pub(crate) trait EmittableMetric {
+    /// Emits the metric to the underlying metrics system.
+    /// The first time this is called for a given metric variant, it will
+    /// also describe the metric to the underlying metrics system.
     fn emit(self);
 }
 
-impl Metric for CounterMetric {
+impl EmittableMetric for CounterMetric {
     /// Increases the counter represented by `self` by 1
     fn emit(self) {
         let name = self.name();
+        let unit = self.unit();
+        let description = self.description();
+
         match self {
             CounterMetric::GuestErrors { code } => {
-                // label keys
-                static LABEL_ERROR_CODE: &str = "code";
+                // Describe each metric variant only once
+                static DESCRIBE: Once = Once::new();
+                DESCRIBE.call_once(|| {
+                    metrics::describe_counter!(name, unit, description);
+                });
 
+                static LABEL_ERROR_CODE: &str = "code";
                 metrics::counter!(name, LABEL_ERROR_CODE => code.to_string()).increment(1);
             }
             CounterMetric::GuestCancellations => {
+                // Describe each metric variant only once
+                static DESCRIBE: Once = Once::new();
+                DESCRIBE.call_once(|| {
+                    metrics::describe_counter!(name, unit, description);
+                });
+
                 metrics::counter!(name).increment(1);
             }
         }
     }
 }
 
-impl Metric for HistogramMetric {
+impl EmittableMetric for HistogramMetric {
     fn emit(self) {
         let metric_name = self.name();
+        let unit = self.unit();
+        let description = self.description();
+
+        static LABEL_FUNCTION_NAME: &str = "function_name";
+
         match self {
-            HistogramMetric::GuestCallDuration { name, duration }
-            | HistogramMetric::HostCallDuration { name, duration } => {
-                static LABEL_FUNCTION_NAME: &str = "function_name";
+            HistogramMetric::GuestCallDuration { name, duration } => {
+                // Describe each metric variant only once
+                static DESCRIBE: Once = Once::new();
+                DESCRIBE.call_once(|| {
+                    metrics::describe_histogram!(metric_name, unit, description);
+                });
+
+                metrics::histogram!(metric_name, LABEL_FUNCTION_NAME => name).record(duration);
+            }
+            HistogramMetric::HostCallDuration { name, duration } => {
+                // Describe each metric variant only once
+                static DESCRIBE: Once = Once::new();
+                DESCRIBE.call_once(|| {
+                    metrics::describe_histogram!(metric_name, unit, description);
+                });
 
                 metrics::histogram!(metric_name, LABEL_FUNCTION_NAME => name).record(duration);
             }
@@ -117,7 +182,9 @@ impl Metric for HistogramMetric {
 
 #[cfg(test)]
 mod tests {
-    use metrics::{Key, Label};
+    use std::sync::atomic::AtomicU64;
+
+    use metrics::{Counter, Gauge, Histogram, Key, Label};
     use metrics_util::CompositeKey;
 
     use super::*;
@@ -207,4 +274,93 @@ mod tests {
             "Histogram metric does not match expected value"
         );
     }
+
+    /// Makes sure that the description function is called only once for each metric variant.
+    /// This helps performance
+    #[test]
+    fn test_description_called_once() {
+        struct DescriptionCounterRecorder {
+            num_descriptions: AtomicU64,
+        }
+
+        impl metrics::Recorder for DescriptionCounterRecorder {
+            fn describe_counter(
+                &self,
+                _key: metrics::KeyName,
+                _unit: Option<metrics::Unit>,
+                _description: metrics::SharedString,
+            ) {
+                self.num_descriptions
+                    .fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+            }
+
+            fn describe_gauge(
+                &self,
+                _key: metrics::KeyName,
+                _unit: Option<metrics::Unit>,
+                _description: metrics::SharedString,
+            ) {
+                self.num_descriptions
+                    .fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+            }
+
+            fn describe_histogram(
+                &self,
+                _key: metrics::KeyName,
+                _unit: Option<metrics::Unit>,
+                _description: metrics::SharedString,
+            ) {
+                self.num_descriptions
+                    .fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+            }
+
+            fn register_counter(
+                &self,
+                _key: &Key,
+                _metadata: &metrics::Metadata<'_>,
+            ) -> metrics::Counter {
+                Counter::noop()
+            }
+
+            fn register_gauge(
+                &self,
+                _key: &Key,
+                _metadata: &metrics::Metadata<'_>,
+            ) -> metrics::Gauge {
+                Gauge::noop()
+            }
+
+            fn register_histogram(
+                &self,
+                _key: &Key,
+                _metadata: &metrics::Metadata<'_>,
+            ) -> metrics::Histogram {
+                Histogram::noop()
+            }
+        }
+
+        let recorder = DescriptionCounterRecorder {
+            num_descriptions: AtomicU64::new(0),
+        };
+
+        metrics::with_local_recorder(&recorder, || {
+            CounterMetric::guest_error(404).emit();
+            CounterMetric::guest_error(500).emit();
+            CounterMetric::GuestCancellations.emit();
+            CounterMetric::GuestCancellations.emit();
+            HistogramMetric::guest_call("GuestFunc1".to_string(), Duration::from_secs(1)).emit();
+            HistogramMetric::guest_call("GuestFunc2".to_string(), Duration::from_secs(2)).emit();
+            HistogramMetric::host_call("HostFunc1".to_string(), Duration::from_secs(3)).emit();
+            HistogramMetric::host_call("HostFunc2".to_string(), Duration::from_secs(4)).emit();
+        });
+
+        // Despite 8 emitted metrics above, we expect only 4 descriptions to be recorded
+        assert_eq!(
+            recorder
+                .num_descriptions
+                .load(std::sync::atomic::Ordering::Relaxed),
+            4,
+            "Expected each metric variant to be described only once"
+        );
+    }
 }