From ea151e4d6819ee2b631184b70e984d4cb0718559 Mon Sep 17 00:00:00 2001
From: Mindaugas Vinkelis <fraillt@gmail.com>
Date: Tue, 21 Jan 2025 22:26:57 +0200
Subject: [PATCH] Address review comments #1

---
 opentelemetry-sdk/src/metrics/data/mod.rs        | 10 +++++-----
 .../src/metrics/internal/aggregate.rs            | 16 ++++++++--------
 .../src/metrics/internal/collector.rs            |  7 +++++--
 .../metrics/internal/exponential_histogram.rs    | 16 +++++++++++++---
 4 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/opentelemetry-sdk/src/metrics/data/mod.rs b/opentelemetry-sdk/src/metrics/data/mod.rs
index aef35bf0f2..e5ec1fc6e1 100644
--- a/opentelemetry-sdk/src/metrics/data/mod.rs
+++ b/opentelemetry-sdk/src/metrics/data/mod.rs
@@ -54,11 +54,11 @@ pub trait Aggregation: fmt::Debug + any::Any + Send + Sync {
 }
 
 /// Allow to access data points of an [Aggregation].
-pub trait AggregationDataPoints {
+pub(crate) trait AggregationDataPoints {
     /// The type of data point in the aggregation.
-    type Point;
+    type DataPoint;
     /// The data points of the aggregation.
-    fn points(&mut self) -> &mut Vec<Self::Point>;
+    fn points(&mut self) -> &mut Vec<Self::DataPoint>;
 }
 
 /// DataPoint is a single data point in a time series.
@@ -237,9 +237,9 @@ impl<T: fmt::Debug + Send + Sync + 'static> Aggregation for ExponentialHistogram
 }
 
 impl<T> AggregationDataPoints for ExponentialHistogram<T> {
-    type Point = ExponentialHistogramDataPoint<T>;
+    type DataPoint = ExponentialHistogramDataPoint<T>;
 
-    fn points(&mut self) -> &mut Vec<Self::Point> {
+    fn points(&mut self) -> &mut Vec<Self::DataPoint> {
         &mut self.data_points
     }
 }
diff --git a/opentelemetry-sdk/src/metrics/internal/aggregate.rs b/opentelemetry-sdk/src/metrics/internal/aggregate.rs
index ef57ef6546..7d46cd9e3f 100644
--- a/opentelemetry-sdk/src/metrics/internal/aggregate.rs
+++ b/opentelemetry-sdk/src/metrics/internal/aggregate.rs
@@ -198,29 +198,29 @@ impl<T: Number> AggregateBuilder<T> {
         record_sum: bool,
     ) -> AggregateFns<T> {
         match self.temporality {
-            Temporality::Delta => ExpoHistogram {
-                aggregate_collector: Collector::new(
+            Temporality::Delta => ExpoHistogram::new(
+                Collector::new(
                     self.filter.clone(),
                     DeltaValueMap::new(ExpoHistogramBucketConfig {
                         max_size: max_size as i32,
                         max_scale,
                     }),
                 ),
-                record_min_max,
                 record_sum,
-            }
+                record_min_max,
+            )
             .into(),
-            _ => ExpoHistogram {
-                aggregate_collector: Collector::new(
+            _ => ExpoHistogram::new(
+                Collector::new(
                     self.filter.clone(),
                     CumulativeValueMap::new(ExpoHistogramBucketConfig {
                         max_size: max_size as i32,
                         max_scale,
                     }),
                 ),
-                record_min_max,
                 record_sum,
-            }
+                record_min_max,
+            )
             .into(),
         }
     }
diff --git a/opentelemetry-sdk/src/metrics/internal/collector.rs b/opentelemetry-sdk/src/metrics/internal/collector.rs
index 29ff5c1186..0ecd5ff7fa 100644
--- a/opentelemetry-sdk/src/metrics/internal/collector.rs
+++ b/opentelemetry-sdk/src/metrics/internal/collector.rs
@@ -40,7 +40,7 @@ pub(crate) trait AggregateCollector: Send + Sync + 'static {
         F: FnMut(
             Vec<KeyValue>,
             &Self::Aggr,
-        ) -> <InitAggregate::Aggr as AggregationDataPoints>::Point;
+        ) -> <InitAggregate::Aggr as AggregationDataPoints>::DataPoint;
 }
 
 pub(crate) struct Collector<AM> {
@@ -92,7 +92,10 @@ where
     ) -> (usize, Option<Box<dyn Aggregation>>)
     where
         InitAggregate: InitAggregationData,
-        F: FnMut(Vec<KeyValue>, &AM::Aggr) -> <InitAggregate::Aggr as AggregationDataPoints>::Point,
+        F: FnMut(
+            Vec<KeyValue>,
+            &AM::Aggr,
+        ) -> <InitAggregate::Aggr as AggregationDataPoints>::DataPoint,
     {
         let time = self.init_time();
         let s_data = dest.and_then(|d| d.as_mut().downcast_mut::<InitAggregate::Aggr>());
diff --git a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs
index fe1b1ccce7..31a1c19ded 100644
--- a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs
+++ b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs
@@ -349,9 +349,19 @@ pub(crate) struct ExpoHistogramBucketConfig {
 /// Each histogram is scoped by attributes and the aggregation cycle the
 /// measurements were made in.
 pub(crate) struct ExpoHistogram<AC> {
-    pub(crate) aggregate_collector: AC,
-    pub(crate) record_sum: bool,
-    pub(crate) record_min_max: bool,
+    aggregate_collector: AC,
+    record_sum: bool,
+    record_min_max: bool,
+}
+
+impl<AC> ExpoHistogram<AC> {
+    pub(crate) fn new(aggregate_collector: AC, record_sum: bool, record_min_max: bool) -> Self {
+        Self {
+            aggregate_collector,
+            record_sum,
+            record_min_max,
+        }
+    }
 }
 
 impl<AC, T> Measure<T> for ExpoHistogram<AC>