Merge branch 'main' into baggage-improvements

lalitb · web-flow · commit 62f127486a75 · 2024-11-08T12:22:23.000-08:00
diff --git a/opentelemetry-otlp/CHANGELOG.md b/opentelemetry-otlp/CHANGELOG.md
@@ -2,14 +2,6 @@
 
 ## vNext
 
-## v0.26.0
-Released 2024-Sep-30
-
-- Update `opentelemetry` dependency version to 0.26
-- Update `opentelemetry_sdk` dependency version to 0.26
-- Update `opentelemetry-http` dependency version to 0.26
-- Update `opentelemetry-proto` dependency version to 0.26
-- Bump MSRV to 1.71.1 [2140](https://github.com/open-telemetry/opentelemetry-rust/pull/2140)
 - **BREAKING**: 
   - ([#2217](https://github.com/open-telemetry/opentelemetry-rust/pull/2217)) **Replaced**: The `MetricsExporterBuilder` interface is modified from `with_temporality_selector` to `with_temporality` example can be seen below:
     Previous Signature:
@@ -31,6 +23,7 @@ Released 2024-Sep-30
     ```rust
     let logger_provider: LoggerProvider = opentelemetry_otlp::new_pipeline()
       .logging()
+      .with_resource(RESOURCE.clone())
       .with_exporter(
           opentelemetry_otlp::new_exporter()
               .tonic()
@@ -40,13 +33,15 @@ Released 2024-Sep-30
     ```
     Updated Signature:
     ```rust
-    let logger_provider: LoggerProvider = LoggerProvider::builder()
-      .install_batch_exporter(
-          LogExporter::builder()
-              .with_tonic()
-              .with_endpoint("http://localhost:4317")
-              .build()?,
-      ).build();
+    let exporter = LogExporter::builder()
+        .with_tonic()
+        .with_endpoint("http://localhost:4317")
+        .build()?;
+
+    Ok(LoggerProvider::builder()
+        .with_resource(RESOURCE.clone())
+        .with_batch_exporter(exporter, runtime::Tokio)
+        .build())
     ```
   - **Renamed**
     - ([#2255](https://github.com/open-telemetry/opentelemetry-rust/pull/2255)): de-pluralize Metric types.
@@ -56,6 +51,15 @@ Released 2024-Sep-30
   - [#2263](https://github.com/open-telemetry/opentelemetry-rust/pull/2263)
   Support `hyper` client for opentelemetry-otlp. This can be enabled using flag `hyper-client`. 
   Refer example: https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-otlp/examples/basic-otlp-http
+  
+## v0.26.0
+Released 2024-Sep-30
+
+- Update `opentelemetry` dependency version to 0.26
+- Update `opentelemetry_sdk` dependency version to 0.26
+- Update `opentelemetry-http` dependency version to 0.26
+- Update `opentelemetry-proto` dependency version to 0.26
+- Bump MSRV to 1.71.1 [2140](https://github.com/open-telemetry/opentelemetry-rust/pull/2140)
 
 ## v0.25.0
 
diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md
@@ -5,7 +5,7 @@
 - Bump MSRV to 1.70 [#2179](https://github.com/open-telemetry/opentelemetry-rust/pull/2179)
 - Implement `LogRecord::set_trace_context` for `LogRecord`. Respect any trace context set on a `LogRecord` when emitting through a `Logger`.
 - Improved `LoggerProvider` shutdown handling to prevent redundant shutdown calls when `drop` is invoked. [#2195](https://github.com/open-telemetry/opentelemetry-rust/pull/2195)
-- When creating new metric instruments, SDK would return a no-op instrument if the validation fails. [#2166](https://github.com/open-telemetry/opentelemetry-rust/pull/2166)
+- When creating new metric instruments by calling `build()`, SDK would return a no-op instrument if the validation fails (eg: Invalid metric name). [#2166](https://github.com/open-telemetry/opentelemetry-rust/pull/2166)
 - **BREAKING for Metrics users**:
   - **Replaced**
     - ([#2217](https://github.com/open-telemetry/opentelemetry-rust/pull/2217)): Removed `{Delta,Cumulative}TemporalitySelector::new()` in favor of directly using `Temporality` enum to simplify the configuration of MetricsExporterBuilder with different temporalities.
diff --git a/opentelemetry-sdk/src/metrics/internal/histogram.rs b/opentelemetry-sdk/src/metrics/internal/histogram.rs
@@ -9,11 +9,7 @@ use opentelemetry::KeyValue;
 use super::ValueMap;
 use super::{Aggregator, Number};
 
-struct HistogramTracker<T> {
-    buckets: Mutex<Buckets<T>>,
-}
-
-impl<T> Aggregator for HistogramTracker<T>
+impl<T> Aggregator for Mutex<Buckets<T>>
 where
     T: Number,
 {
@@ -22,27 +18,26 @@ where
     type PreComputedValue = (T, usize);
 
     fn update(&self, (value, index): (T, usize)) {
-        let mut buckets = match self.buckets.lock() {
-            Ok(guard) => guard,
-            Err(_) => return,
-        };
+        let mut buckets = self.lock().unwrap_or_else(|err| err.into_inner());
 
-        buckets.bin(index, value);
-        buckets.sum(value);
+        buckets.total += value;
+        buckets.count += 1;
+        buckets.counts[index] += 1;
+        if value < buckets.min {
+            buckets.min = value;
+        }
+        if value > buckets.max {
+            buckets.max = value
+        }
     }
 
     fn create(count: &usize) -> Self {
-        HistogramTracker {
-            buckets: Mutex::new(Buckets::<T>::new(*count)),
-        }
+        Mutex::new(Buckets::<T>::new(*count))
     }
 
     fn clone_and_reset(&self, count: &usize) -> Self {
-        let mut current = self.buckets.lock().unwrap_or_else(|err| err.into_inner());
-        let cloned = replace(current.deref_mut(), Buckets::new(*count));
-        Self {
-            buckets: Mutex::new(cloned),
-        }
+        let mut current = self.lock().unwrap_or_else(|err| err.into_inner());
+        Mutex::new(replace(current.deref_mut(), Buckets::new(*count)))
     }
 }
 
@@ -65,62 +60,34 @@ impl<T: Number> Buckets<T> {
             ..Default::default()
         }
     }
-
-    fn sum(&mut self, value: T) {
-        self.total += value;
-    }
-
-    fn bin(&mut self, idx: usize, value: T) {
-        self.counts[idx] += 1;
-        self.count += 1;
-        if value < self.min {
-            self.min = value;
-        }
-        if value > self.max {
-            self.max = value
-        }
-    }
 }
 
 /// Summarizes a set of measurements as a histogram with explicitly defined
 /// buckets.
 pub(crate) struct Histogram<T: Number> {
-    value_map: ValueMap<HistogramTracker<T>>,
+    value_map: ValueMap<Mutex<Buckets<T>>>,
     bounds: Vec<f64>,
     record_min_max: bool,
     record_sum: bool,
     start: Mutex<SystemTime>,
 }
 
 impl<T: Number> Histogram<T> {
-    pub(crate) fn new(boundaries: Vec<f64>, record_min_max: bool, record_sum: bool) -> Self {
-        // TODO fix the bug, by first removing NaN and only then getting buckets_count
-        // once we know the reason for performance degradation
-        let buckets_count = boundaries.len() + 1;
-        let mut histogram = Histogram {
+    pub(crate) fn new(mut bounds: Vec<f64>, record_min_max: bool, record_sum: bool) -> Self {
+        bounds.retain(|v| !v.is_nan());
+        bounds.sort_by(|a, b| a.partial_cmp(b).expect("NaNs filtered out"));
+        let buckets_count = bounds.len() + 1;
+        Histogram {
             value_map: ValueMap::new(buckets_count),
-            bounds: boundaries,
+            bounds,
             record_min_max,
             record_sum,
             start: Mutex::new(SystemTime::now()),
-        };
-
-        histogram.bounds.retain(|v| !v.is_nan());
-        histogram
-            .bounds
-            .sort_by(|a, b| a.partial_cmp(b).expect("NaNs filtered out"));
-
-        histogram
+        }
     }
 
     pub(crate) fn measure(&self, measurement: T, attrs: &[KeyValue]) {
         let f = measurement.into_float();
-        // Ignore NaN and infinity.
-        // Only makes sense if T is f64, maybe this could be no-op for other cases?
-        // TODO: uncomment once we know the reason for performance degradation
-        // if f.is_infinite() || f.is_nan() {
-        //     return;
-        // }
         // This search will return an index in the range `[0, bounds.len()]`, where
         // it will return `bounds.len()` if value is greater than the last element
         // of `bounds`. This aligns with the buckets in that the length of buckets
@@ -156,17 +123,14 @@ impl<T: Number> Histogram<T> {
 
         self.value_map
             .collect_and_reset(&mut h.data_points, |attributes, aggr| {
-                let b = aggr
-                    .buckets
-                    .into_inner()
-                    .unwrap_or_else(|err| err.into_inner());
+                let b = aggr.into_inner().unwrap_or_else(|err| err.into_inner());
                 HistogramDataPoint {
                     attributes,
                     start_time: prev_start,
                     time: t,
                     count: b.count,
                     bounds: self.bounds.clone(),
-                    bucket_counts: b.counts.clone(),
+                    bucket_counts: b.counts,
                     sum: if self.record_sum {
                         b.total
                     } else {
@@ -214,7 +178,7 @@ impl<T: Number> Histogram<T> {
 
         self.value_map
             .collect_readonly(&mut h.data_points, |attributes, aggr| {
-                let b = aggr.buckets.lock().unwrap_or_else(|err| err.into_inner());
+                let b = aggr.lock().unwrap_or_else(|err| err.into_inner());
                 HistogramDataPoint {
                     attributes,
                     start_time: prev_start,
@@ -245,68 +209,25 @@ impl<T: Number> Histogram<T> {
     }
 }
 
-// TODO: uncomment once we know the reason for performance degradation
-// #[cfg(test)]
-// mod tests {
+#[cfg(test)]
+mod tests {
+    use super::*;
 
-//     use super::*;
-
-//     #[test]
-//     fn when_f64_is_nan_or_infinity_then_ignore() {
-//         struct Expected {
-//             min: f64,
-//             max: f64,
-//             sum: f64,
-//             count: u64,
-//         }
-//         impl Expected {
-//             fn new(min: f64, max: f64, sum: f64, count: u64) -> Self {
-//                 Expected {
-//                     min,
-//                     max,
-//                     sum,
-//                     count,
-//                 }
-//             }
-//         }
-//         struct TestCase {
-//             values: Vec<f64>,
-//             expected: Expected,
-//         }
-
-//         let test_cases = vec![
-//             TestCase {
-//                 values: vec![2.0, 4.0, 1.0],
-//                 expected: Expected::new(1.0, 4.0, 7.0, 3),
-//             },
-//             TestCase {
-//                 values: vec![2.0, 4.0, 1.0, f64::INFINITY],
-//                 expected: Expected::new(1.0, 4.0, 7.0, 3),
-//             },
-//             TestCase {
-//                 values: vec![2.0, 4.0, 1.0, -f64::INFINITY],
-//                 expected: Expected::new(1.0, 4.0, 7.0, 3),
-//             },
-//             TestCase {
-//                 values: vec![2.0, f64::NAN, 4.0, 1.0],
-//                 expected: Expected::new(1.0, 4.0, 7.0, 3),
-//             },
-//             TestCase {
-//                 values: vec![4.0, 4.0, 4.0, 2.0, 16.0, 1.0],
-//                 expected: Expected::new(1.0, 16.0, 31.0, 6),
-//             },
-//         ];
-
-//         for test in test_cases {
-//             let h = Histogram::new(vec![], true, true);
-//             for v in test.values {
-//                 h.measure(v, &[]);
-//             }
-//             let res = h.value_map.no_attribute_tracker.buckets.lock().unwrap();
-//             assert_eq!(test.expected.max, res.max);
-//             assert_eq!(test.expected.min, res.min);
-//             assert_eq!(test.expected.sum, res.total);
-//             assert_eq!(test.expected.count, res.count);
-//         }
-//     }
-// }
+    #[test]
+    fn check_buckets_are_selected_correctly() {
+        let hist = Histogram::<i64>::new(vec![1.0, 3.0, 6.0], false, false);
+        for v in 1..11 {
+            hist.measure(v, &[]);
+        }
+        let (count, dp) = hist.cumulative(None);
+        let dp = dp.unwrap();
+        let dp = dp.as_any().downcast_ref::<data::Histogram<i64>>().unwrap();
+        assert_eq!(count, 1);
+        assert_eq!(dp.data_points[0].count, 10);
+        assert_eq!(dp.data_points[0].bucket_counts.len(), 4);
+        assert_eq!(dp.data_points[0].bucket_counts[0], 1); // 1
+        assert_eq!(dp.data_points[0].bucket_counts[1], 2); // 2, 3
+        assert_eq!(dp.data_points[0].bucket_counts[2], 3); // 4, 5, 6
+        assert_eq!(dp.data_points[0].bucket_counts[3], 4); // 7, 8, 9, 10
+    }
+}
diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md
@@ -8,11 +8,9 @@
 - Introduced `SyncInstrument` trait to replace the individual synchronous instrument traits (`SyncCounter`, `SyncGauge`, `SyncHistogram`, `SyncUpDownCounter`) which are meant for SDK implementation. [#2207](https://github.com/open-telemetry/opentelemetry-rust/pull/2207)
 - Ensured that `observe` method on asynchronous instruments can only be called inside a callback. This was done by removing the implementation of `AsyncInstrument` trait for each of the asynchronous instruments. [#2210](https://github.com/open-telemetry/opentelemetry-rust/pull/2210)
 - Removed `PartialOrd` and `Ord` implementations for `KeyValue`. [#2215](https://github.com/open-telemetry/opentelemetry-rust/pull/2215)
-- **Breaking change** Removed `try_init` methods from the instrument builders. Users should only use `init` method to create instruments. [#2227](https://github.com/open-telemetry/opentelemetry-rust/pull/2227)
-- Updated the return types of `InstrumentProvider` trait methods to return the instrument instead of a `Result`. [#2227](https://github.com/open-telemetry/opentelemetry-rust/pull/2227)
 - **Breaking change for exporter authors:** Marked `KeyValue` related structs and enums as `non_exhaustive`. [#2228](https://github.com/open-telemetry/opentelemetry-rust/pull/2228)
 - **Breaking change for log exporter authors:** Marked `AnyValue` enum as `non_exhaustive`. [#2230](https://github.com/open-telemetry/opentelemetry-rust/pull/2230)
-- **Breaking change for Metrics users:** The `init` method used to create instruments has been renamed to `build`.
+- **Breaking change for Metrics users:** The `init` method used to create instruments has been renamed to `build`. Also, `try_init()` method is removed from instrument builders. The return types of `InstrumentProvider` trait methods modified to return the instrument struct, instead of `Result`. [#2227](https://github.com/open-telemetry/opentelemetry-rust/pull/2227)
 
 Before:
 ```rust
@@ -32,16 +30,16 @@ let counter = meter.u64_counter("my_counter").build();
   - Replaced these methods with `LoggerProvider::logger_with_scope`, `TracerProvider::logger_with_scope`, `MeterProvider::meter_with_scope`
   - Replaced `global::meter_with_version` with `global::meter_with_scope`
   - Added `global::tracer_with_scope`
+  - Refer to PR description for migration guide.
+- **Breaking change**: replaced `InstrumentationScope` public attributes by getters [#2275](https://github.com/open-telemetry/opentelemetry-rust/pull/2275)  
 
 - **Breaking change**: [#2260](https://github.com/open-telemetry/opentelemetry-rust/pull/2260)
   - Removed `global::set_error_handler` and `global::handle_error`. 
   - `global::handle_error` usage inside the opentelemetry crates has been replaced with `global::otel_info`, `otel_warn`, `otel_debug` and `otel_error` macros based on the severity of the internal logs.
-  - The default behavior of `global::handle_error` was to log the error using `eprintln!`. With otel macro, the internal logs get emitted via `tracing` macros of matching severity. Users now need to configure the `tracing` layer to capture these logs.
-  - Refer to this PR description for migration guide. Also refer to [self-diagnostics](https://github.com/open-telemetry/opentelemetry-rust/tree/main/examples/self-diagnostics) example on how to configure the tracing layer for internal logs.
-- **Breaking change**: replaced `InstrumentationScope` public attributes by getters [#2275](https://github.com/open-telemetry/opentelemetry-rust/pull/2275)
+  - The default behavior of `global::handle_error` was to log the error using `eprintln!`. With otel macros, the internal logs get emitted via `tracing` macros of matching severity. Users now need to configure a `tracing` layer/subscriber to capture these logs.
+  - Refer to PR description for migration guide. Also refer to [self-diagnostics](https://github.com/open-telemetry/opentelemetry-rust/tree/main/examples/self-diagnostics) example to learn how to view internal logs in stdout using `tracing::fmt` layer.
 
-
-- [#2266](https://github.com/open-telemetry/opentelemetry-rust/pull/2266)
+- **Breaking change for exporter/processor authors:** [#2266](https://github.com/open-telemetry/opentelemetry-rust/pull/2266)
    - Moved `ExportError` trait from `opentelemetry::ExportError` to `opentelemetry_sdk::export::ExportError`
    - Created new trait `opentelemetry::trace::ExportError` for trace API. This would be eventually be consolidated with ExportError in the SDK.
    - Moved `LogError` enum from `opentelemetry::logs::LogError` to `opentelemetry_sdk::logs::LogError`
diff --git a/opentelemetry/src/metrics/instruments/counter.rs b/opentelemetry/src/metrics/instruments/counter.rs
@@ -5,6 +5,10 @@ use std::sync::Arc;
 use super::SyncInstrument;
 
 /// An instrument that records increasing values.
+///
+/// [`Counter`] can be cloned to create multiple handles to the same instrument. If a [`Counter`] needs to be shared,
+/// users are recommended to clone the [`Counter`] instead of creating duplicate [`Counter`]s for the same metric. Creating
+/// duplicate [`Counter`]s for the same metric could lower SDK performance.
 #[derive(Clone)]
 #[non_exhaustive]
 pub struct Counter<T>(Arc<dyn SyncInstrument<T> + Send + Sync>);
diff --git a/opentelemetry/src/metrics/instruments/gauge.rs b/opentelemetry/src/metrics/instruments/gauge.rs
@@ -5,6 +5,10 @@ use std::sync::Arc;
 use super::SyncInstrument;
 
 /// An instrument that records independent values
+///
+/// [`Gauge`] can be cloned to create multiple handles to the same instrument. If a [`Gauge`] needs to be shared,
+/// users are recommended to clone the [`Gauge`] instead of creating duplicate [`Gauge`]s for the same metric. Creating
+/// duplicate [`Gauge`]s for the same metric could lower SDK performance.
 #[derive(Clone)]
 #[non_exhaustive]
 pub struct Gauge<T>(Arc<dyn SyncInstrument<T> + Send + Sync>);
diff --git a/opentelemetry/src/metrics/instruments/histogram.rs b/opentelemetry/src/metrics/instruments/histogram.rs
@@ -5,6 +5,10 @@ use std::sync::Arc;
 use super::SyncInstrument;
 
 /// An instrument that records a distribution of values.
+///
+/// [`Histogram`] can be cloned to create multiple handles to the same instrument. If a [`Histogram`] needs to be shared,
+/// users are recommended to clone the [`Histogram`] instead of creating duplicate [`Histogram`]s for the same metric. Creating
+/// duplicate [`Histogram`]s for the same metric could lower SDK performance.
 #[derive(Clone)]
 #[non_exhaustive]
 pub struct Histogram<T>(Arc<dyn SyncInstrument<T> + Send + Sync>);
diff --git a/opentelemetry/src/metrics/instruments/up_down_counter.rs b/opentelemetry/src/metrics/instruments/up_down_counter.rs
@@ -5,6 +5,10 @@ use std::sync::Arc;
 use super::SyncInstrument;
 
 /// An instrument that records increasing or decreasing values.
+///
+/// [`UpDownCounter`] can be cloned to create multiple handles to the same instrument. If a [`UpDownCounter`] needs to be shared,
+/// users are recommended to clone the [`UpDownCounter`] instead of creating duplicate [`UpDownCounter`]s for the same metric. Creating
+/// duplicate [`UpDownCounter`]s for the same metric could lower SDK performance.
 #[derive(Clone)]
 #[non_exhaustive]
 pub struct UpDownCounter<T>(Arc<dyn SyncInstrument<T> + Send + Sync>);
diff --git a/opentelemetry/src/metrics/meter.rs b/opentelemetry/src/metrics/meter.rs
