PR feedback

smithy-lang · Nov 21, 2024 · 9f0df17 · 9f0df17
1 parent 9df046d
commit 9f0df17
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 60 deletions.
diff --git a/rust-runtime/aws-smithy-observability-otel/src/lib.rs b/rust-runtime/aws-smithy-observability-otel/src/lib.rs
@@ -46,7 +46,7 @@ mod tests {
 
         // Set the global TelemetryProvider and then get it back out
         let _ = set_telemetry_provider(sdk_tp);
-        let global_tp = get_telemetry_provider();
+        let global_tp = get_telemetry_provider().unwrap();
 
         // Create an instrument and record a value
         let global_meter = global_tp

diff --git a/rust-runtime/aws-smithy-observability-otel/src/meter.rs b/rust-runtime/aws-smithy-observability-otel/src/meter.rs
@@ -7,6 +7,7 @@
 
 use std::fmt::Debug;
 use std::ops::Deref;
+use std::sync::Arc;
 
 use crate::attributes::kv_from_option_attr;
 use aws_smithy_observability::attributes::{Attributes, Context};
@@ -147,7 +148,7 @@ impl Meter for MeterWrap {
         callback: Box<dyn Fn(&dyn AsyncMeasure<Value = f64>) + Send + Sync>,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = f64>> {
+    ) -> Arc<dyn AsyncMeasure<Value = f64>> {
         let mut builder = self.f64_observable_gauge(name).with_callback(
             move |input: &dyn OtelAsyncInstrument<f64>| {
                 callback(&AsyncInstrumentWrap(input));
@@ -162,15 +163,15 @@ impl Meter for MeterWrap {
             builder = builder.with_unit(u);
         }
 
-        Box::new(GaugeWrap(builder.init()))
+        Arc::new(GaugeWrap(builder.init()))
     }
 
     fn create_up_down_counter(
         &self,
         name: String,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn UpDownCounter> {
+    ) -> Arc<dyn UpDownCounter> {
         let mut builder = self.i64_up_down_counter(name);
         if let Some(desc) = description {
             builder = builder.with_description(desc);
@@ -180,7 +181,7 @@ impl Meter for MeterWrap {
             builder = builder.with_unit(u);
         }
 
-        Box::new(UpDownCounterWrap(builder.init()))
+        Arc::new(UpDownCounterWrap(builder.init()))
     }
 
     fn create_async_up_down_counter(
@@ -189,7 +190,7 @@ impl Meter for MeterWrap {
         callback: Box<dyn Fn(&dyn AsyncMeasure<Value = i64>) + Send + Sync>,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = i64>> {
+    ) -> Arc<dyn AsyncMeasure<Value = i64>> {
         let mut builder = self.i64_observable_up_down_counter(name).with_callback(
             move |input: &dyn OtelAsyncInstrument<i64>| {
                 callback(&AsyncInstrumentWrap(input));
@@ -204,15 +205,15 @@ impl Meter for MeterWrap {
             builder = builder.with_unit(u);
         }
 
-        Box::new(AsyncUpDownCounterWrap(builder.init()))
+        Arc::new(AsyncUpDownCounterWrap(builder.init()))
     }
 
     fn create_monotonic_counter(
         &self,
         name: String,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn MonotonicCounter> {
+    ) -> Arc<dyn MonotonicCounter> {
         let mut builder = self.u64_counter(name);
         if let Some(desc) = description {
             builder = builder.with_description(desc);
@@ -222,7 +223,7 @@ impl Meter for MeterWrap {
             builder = builder.with_unit(u);
         }
 
-        Box::new(MonotonicCounterWrap(builder.init()))
+        Arc::new(MonotonicCounterWrap(builder.init()))
     }
 
     fn create_async_monotonic_counter(
@@ -231,7 +232,7 @@ impl Meter for MeterWrap {
         callback: Box<dyn Fn(&dyn AsyncMeasure<Value = u64>) + Send + Sync>,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = u64>> {
+    ) -> Arc<dyn AsyncMeasure<Value = u64>> {
         let mut builder = self.u64_observable_counter(name).with_callback(
             move |input: &dyn OtelAsyncInstrument<u64>| {
                 callback(&AsyncInstrumentWrap(input));
@@ -246,15 +247,15 @@ impl Meter for MeterWrap {
             builder = builder.with_unit(u);
         }
 
-        Box::new(AsyncMonotonicCounterWrap(builder.init()))
+        Arc::new(AsyncMonotonicCounterWrap(builder.init()))
     }
 
     fn create_histogram(
         &self,
         name: String,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn Histogram> {
+    ) -> Arc<dyn Histogram> {
         let mut builder = self.f64_histogram(name);
         if let Some(desc) = description {
             builder = builder.with_description(desc);
@@ -264,7 +265,7 @@ impl Meter for MeterWrap {
             builder = builder.with_unit(u);
         }
 
-        Box::new(HistogramWrap(builder.init()))
+        Arc::new(HistogramWrap(builder.init()))
     }
 }
 
@@ -301,8 +302,8 @@ impl AwsSdkOtelMeterProvider {
 }
 
 impl ProvideMeter for AwsSdkOtelMeterProvider {
-    fn get_meter(&self, scope: &'static str, _attributes: Option<&Attributes>) -> Box<dyn Meter> {
-        Box::new(MeterWrap(self.meter_provider.meter(scope)))
+    fn get_meter(&self, scope: &'static str, _attributes: Option<&Attributes>) -> Arc<dyn Meter> {
+        Arc::new(MeterWrap(self.meter_provider.meter(scope)))
     }
 
     fn as_any(&self) -> &dyn std::any::Any {

diff --git a/rust-runtime/aws-smithy-observability/src/attributes.rs b/rust-runtime/aws-smithy-observability/src/attributes.rs
@@ -25,17 +25,15 @@ pub enum AttributeValue {
 
 /// Structured telemetry metadata.
 #[non_exhaustive]
-#[derive(Clone)]
+#[derive(Clone, Default)]
 pub struct Attributes {
     attrs: HashMap<String, AttributeValue>,
 }
 
 impl Attributes {
     /// Create a new empty instance of [Attributes].
     pub fn new() -> Self {
-        Self {
-            attrs: HashMap::new(),
-        }
+        Self::default()
     }
 
     /// Set an attribute.
@@ -59,12 +57,6 @@ impl Attributes {
     }
 }
 
-impl Default for Attributes {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
 /// Delineates a logical scope that has some beginning and end
 /// (e.g. a function or block of code).
 pub trait Scope {

diff --git a/rust-runtime/aws-smithy-observability/src/global.rs b/rust-runtime/aws-smithy-observability/src/global.rs
@@ -33,17 +33,16 @@ pub fn set_telemetry_provider(new_provider: TelemetryProvider) {
     let _ = mem::replace(&mut *old_provider, new_global_provider);
 }
 
-/// Get an [Arc] reference to the current global [TelemetryProvider].
-///
-/// This can panic if called when another thread is calling [set_telemetry_provider].
-pub fn get_telemetry_provider() -> Arc<TelemetryProvider> {
-    // TODO(smithyObservability): would probably be nicer to return a Result here, but the Guard held by the error from
+/// Get an [Arc] reference to the current global [TelemetryProvider]. [None] is returned if the [RwLock] containing
+/// the [GlobalTelemetryProvider] is poisoned or is currently locked by a writer.
+pub fn get_telemetry_provider() -> Option<Arc<TelemetryProvider>> {
+    // TODO(smithyObservability): would probably make more sense to return a Result rather than an Option here, but the Guard held by the error from
     // .try_read is not Send so I struggled to build an ObservabilityError from it
-    GLOBAL_TELEMETRY_PROVIDER
-        .try_read()
-        .expect("GLOBAL_TELEMETRY_PROVIDER RwLock Poisoned")
-        .telemetry_provider()
-        .clone()
+    if let Ok(tp) = GLOBAL_TELEMETRY_PROVIDER.try_read() {
+        Some(tp.telemetry_provider().clone())
+    } else {
+        None
+    }
 }
 
 #[cfg(test)]
@@ -66,7 +65,7 @@ mod tests {
     #[test]
     #[serial]
     fn can_get_global_telemetry_provider() {
-        let curr_provider = get_telemetry_provider();
+        let curr_provider = get_telemetry_provider().unwrap();
 
         // Use the global provider to create an instrument and record a value with it
         let curr_mp = curr_provider.meter_provider();

diff --git a/rust-runtime/aws-smithy-observability/src/meter.rs b/rust-runtime/aws-smithy-observability/src/meter.rs
@@ -7,12 +7,12 @@
 //! real time.
 
 use crate::attributes::{Attributes, Context};
-use std::fmt::Debug;
+use std::{fmt::Debug, sync::Arc};
 
 /// Provides named instances of [Meter].
 pub trait ProvideMeter: Send + Sync + Debug {
     /// Get or create a named [Meter].
-    fn get_meter(&self, scope: &'static str, attributes: Option<&Attributes>) -> Box<dyn Meter>;
+    fn get_meter(&self, scope: &'static str, attributes: Option<&Attributes>) -> Arc<dyn Meter>;
 
     /// Foo
     fn as_any(&self) -> &dyn std::any::Any;
@@ -28,15 +28,15 @@ pub trait Meter: Send + Sync + Debug {
         callback: Box<dyn Fn(&dyn AsyncMeasure<Value = f64>) + Send + Sync>,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = f64>>;
+    ) -> Arc<dyn AsyncMeasure<Value = f64>>;
 
     /// Create a new [UpDownCounter].
     fn create_up_down_counter(
         &self,
         name: String,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn UpDownCounter>;
+    ) -> Arc<dyn UpDownCounter>;
 
     /// Create a new AsyncUpDownCounter.
     #[allow(clippy::type_complexity)]
@@ -46,15 +46,15 @@ pub trait Meter: Send + Sync + Debug {
         callback: Box<dyn Fn(&dyn AsyncMeasure<Value = i64>) + Send + Sync>,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = i64>>;
+    ) -> Arc<dyn AsyncMeasure<Value = i64>>;
 
     /// Create a new [MonotonicCounter].
     fn create_monotonic_counter(
         &self,
         name: String,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn MonotonicCounter>;
+    ) -> Arc<dyn MonotonicCounter>;
 
     /// Create a new AsyncMonotonicCounter.
     #[allow(clippy::type_complexity)]
@@ -64,15 +64,15 @@ pub trait Meter: Send + Sync + Debug {
         callback: Box<dyn Fn(&dyn AsyncMeasure<Value = u64>) + Send + Sync>,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = u64>>;
+    ) -> Arc<dyn AsyncMeasure<Value = u64>>;
 
     /// Create a new [Histogram].
     fn create_histogram(
         &self,
         name: String,
         units: Option<String>,
         description: Option<String>,
-    ) -> Box<dyn Histogram>;
+    ) -> Arc<dyn Histogram>;
 }
 
 /// Collects a set of events with an event count and sum for all events.
@@ -106,6 +106,6 @@ pub trait AsyncMeasure: Send + Sync + Debug {
         context: Option<&dyn Context>,
     );
 
-    /// Stop recording , unregister callback.
+    /// Stop recording, unregister callback.
     fn stop(&self);
 }
diff --git a/rust-runtime/aws-smithy-observability/src/noop.rs b/rust-runtime/aws-smithy-observability/src/noop.rs
@@ -5,8 +5,8 @@
 
 //! An noop implementation of the Meter traits
 
-use std::fmt::Debug;
 use std::marker::PhantomData;
+use std::{fmt::Debug, sync::Arc};
 
 use crate::{
     attributes::{Attributes, Context},
@@ -16,8 +16,8 @@ use crate::{
 #[derive(Debug)]
 pub(crate) struct NoopMeterProvider;
 impl ProvideMeter for NoopMeterProvider {
-    fn get_meter(&self, _scope: &'static str, _attributes: Option<&Attributes>) -> Box<dyn Meter> {
-        Box::new(NoopMeter)
+    fn get_meter(&self, _scope: &'static str, _attributes: Option<&Attributes>) -> Arc<dyn Meter> {
+        Arc::new(NoopMeter)
     }
 
     fn as_any(&self) -> &dyn std::any::Any {
@@ -34,17 +34,17 @@ impl Meter for NoopMeter {
         _callback: Box<dyn Fn(&dyn AsyncMeasure<Value = f64>) + Send + Sync>,
         _units: Option<String>,
         _description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = f64>> {
-        Box::new(NoopAsyncMeasurement(PhantomData::<f64>))
+    ) -> Arc<dyn AsyncMeasure<Value = f64>> {
+        Arc::new(NoopAsyncMeasurement(PhantomData::<f64>))
     }
 
     fn create_up_down_counter(
         &self,
         _name: String,
         _units: Option<String>,
         _description: Option<String>,
-    ) -> Box<dyn UpDownCounter> {
-        Box::new(NoopUpDownCounter)
+    ) -> Arc<dyn UpDownCounter> {
+        Arc::new(NoopUpDownCounter)
     }
 
     fn create_async_up_down_counter(
@@ -53,17 +53,17 @@ impl Meter for NoopMeter {
         _callback: Box<dyn Fn(&dyn AsyncMeasure<Value = i64>) + Send + Sync>,
         _units: Option<String>,
         _description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = i64>> {
-        Box::new(NoopAsyncMeasurement(PhantomData::<i64>))
+    ) -> Arc<dyn AsyncMeasure<Value = i64>> {
+        Arc::new(NoopAsyncMeasurement(PhantomData::<i64>))
     }
 
     fn create_monotonic_counter(
         &self,
         _name: String,
         _units: Option<String>,
         _description: Option<String>,
-    ) -> Box<dyn MonotonicCounter> {
-        Box::new(NoopMonotonicCounter)
+    ) -> Arc<dyn MonotonicCounter> {
+        Arc::new(NoopMonotonicCounter)
     }
 
     fn create_async_monotonic_counter(
@@ -72,17 +72,17 @@ impl Meter for NoopMeter {
         _callback: Box<dyn Fn(&dyn AsyncMeasure<Value = u64>) + Send + Sync>,
         _units: Option<String>,
         _description: Option<String>,
-    ) -> Box<dyn AsyncMeasure<Value = u64>> {
-        Box::new(NoopAsyncMeasurement(PhantomData::<u64>))
+    ) -> Arc<dyn AsyncMeasure<Value = u64>> {
+        Arc::new(NoopAsyncMeasurement(PhantomData::<u64>))
     }
 
     fn create_histogram(
         &self,
         _name: String,
         _units: Option<String>,
         _description: Option<String>,
-    ) -> Box<dyn Histogram> {
-        Box::new(NoopHistogram)
+    ) -> Arc<dyn Histogram> {
+        Arc::new(NoopHistogram)
     }
 }