From af9d925736905fce12bf5f22bd24ead017156b06 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 20 Nov 2024 17:33:37 -0800 Subject: [PATCH 1/2] chore: update proto definition to v1.4.0 (#2315) --- opentelemetry-proto/CHANGELOG.md | 3 +++ .../src/proto/opentelemetry-proto | 2 +- .../tonic/opentelemetry.proto.metrics.v1.rs | 27 ++++++++++++++++--- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/opentelemetry-proto/CHANGELOG.md b/opentelemetry-proto/CHANGELOG.md index bc5fdb454f..e4734a7479 100644 --- a/opentelemetry-proto/CHANGELOG.md +++ b/opentelemetry-proto/CHANGELOG.md @@ -2,6 +2,9 @@ ## vNext +- Update proto definitions to v1.4.0 [#2315](https://github.com/open-telemetry/opentelemetry-rust/pull/2315) + + ## 0.27.0 Released 2024-Nov-11 diff --git a/opentelemetry-proto/src/proto/opentelemetry-proto b/opentelemetry-proto/src/proto/opentelemetry-proto index 40b3c1b746..0adf6aac00 160000 --- a/opentelemetry-proto/src/proto/opentelemetry-proto +++ b/opentelemetry-proto/src/proto/opentelemetry-proto @@ -1 +1 @@ -Subproject commit 40b3c1b746767cbc13c2e39da3eaf1a23e54ffdd +Subproject commit 0adf6aac004578b28267394514b2e55ee9cc012f diff --git a/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.metrics.v1.rs b/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.metrics.v1.rs index 0c2200ae41..66e316689d 100644 --- a/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.metrics.v1.rs +++ b/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.metrics.v1.rs @@ -3,6 +3,24 @@ /// storage, OR can be embedded by other protocols that transfer OTLP metrics /// data but do not implement the OTLP protocol. /// +/// MetricsData +/// └─── ResourceMetrics +/// ├── Resource +/// ├── SchemaURL +/// └── ScopeMetrics +/// ├── Scope +/// ├── SchemaURL +/// └── Metric +/// ├── Name +/// ├── Description +/// ├── Unit +/// └── data +/// ├── Gauge +/// ├── Sum +/// ├── Histogram +/// ├── ExponentialHistogram +/// └── Summary +/// /// The main difference between this message and collector protocol is that /// in this message there will not be any "control" or "metadata" specific to /// OTLP protocol. @@ -71,7 +89,6 @@ pub struct ScopeMetrics { /// /// /// -/// /// The data model and relation between entities is shown in the /// diagram below. Here, "DataPoint" is the term used to refer to any /// one of the specific data point value types, and "points" is the term used @@ -83,7 +100,7 @@ pub struct ScopeMetrics { /// - DataPoint contains timestamps, attributes, and one of the possible value type /// fields. /// -/// Metric +/// Metric /// +------------+ /// |name | /// |description | @@ -277,6 +294,9 @@ pub struct ExponentialHistogram { /// data type. These data points cannot always be merged in a meaningful way. /// While they can be useful in some applications, histogram data points are /// recommended for new applications. +/// Summary metrics do not have an aggregation temporality field. This is +/// because the count and sum fields of a SummaryDataPoint are assumed to be +/// cumulative values. #[cfg_attr(feature = "with-schemars", derive(schemars::JsonSchema))] #[cfg_attr(feature = "with-serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "with-serde", serde(rename_all = "camelCase"))] @@ -587,7 +607,8 @@ pub mod exponential_histogram_data_point { } } /// SummaryDataPoint is a single data point in a timeseries that describes the -/// time-varying values of a Summary metric. +/// time-varying values of a Summary metric. The count and sum fields represent +/// cumulative values. #[cfg_attr(feature = "with-schemars", derive(schemars::JsonSchema))] #[cfg_attr(feature = "with-serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "with-serde", serde(rename_all = "camelCase"))] From 465fcc2eda4e6e1f98ec5fdb74472f52cdc5207c Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 20 Nov 2024 19:41:40 -0800 Subject: [PATCH 2/2] AttributeSet cleanup, better perf for overflows (#2313) --- opentelemetry-sdk/benches/metrics_counter.rs | 2 +- opentelemetry-sdk/src/metrics/internal/mod.rs | 14 ++++- opentelemetry-sdk/src/metrics/mod.rs | 60 +------------------ 3 files changed, 13 insertions(+), 63 deletions(-) diff --git a/opentelemetry-sdk/benches/metrics_counter.rs b/opentelemetry-sdk/benches/metrics_counter.rs index 166ae7a826..b4517d379b 100644 --- a/opentelemetry-sdk/benches/metrics_counter.rs +++ b/opentelemetry-sdk/benches/metrics_counter.rs @@ -9,7 +9,7 @@ |--------------------------------|-------------| | Counter_Add_Sorted | 172 ns | | Counter_Add_Unsorted | 183 ns | - | Counter_Overflow | 898 ns | + | Counter_Overflow | 562 ns | | ThreadLocal_Random_Generator_5 | 37 ns | */ diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index 8b6136d7ce..4eaea7972c 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -18,8 +18,6 @@ pub(crate) use exponential_histogram::{EXPO_MAX_SCALE, EXPO_MIN_SCALE}; use once_cell::sync::Lazy; use opentelemetry::{otel_warn, KeyValue}; -use crate::metrics::AttributeSet; - pub(crate) static STREAM_OVERFLOW_ATTRIBUTES: Lazy> = Lazy::new(|| vec![KeyValue::new("otel.metric.overflow", "true")]); @@ -95,7 +93,7 @@ where } // Try to retrieve and update the tracker with the attributes sorted. - let sorted_attrs = AttributeSet::from(attributes).into_vec(); + let sorted_attrs = sort_and_dedup(attributes); if let Some(tracker) = trackers.get(sorted_attrs.as_slice()) { tracker.update(value); return; @@ -198,6 +196,16 @@ fn prepare_data(data: &mut Vec, list_len: usize) { } } +fn sort_and_dedup(attributes: &[KeyValue]) -> Vec { + // Use newly allocated vec here as incoming attributes are immutable so + // cannot sort/de-dup in-place. TODO: This allocation can be avoided by + // leveraging a ThreadLocal vec. + let mut sorted = attributes.to_vec(); + sorted.sort_unstable_by(|a, b| a.key.cmp(&b.key)); + sorted.dedup_by(|a, b| a.key == b.key); + sorted +} + /// Marks a type that can have a value added and retrieved atomically. Required since /// different types have different backing atomic mechanisms pub(crate) trait AtomicTracker: Sync + Send + 'static { diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index ec0b2d180f..bbbfdac27c 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -77,11 +77,7 @@ pub use view::*; // #[cfg(not(feature = "spec_unstable_metrics_views"))] // pub(crate) use view::*; -use std::collections::hash_map::DefaultHasher; -use std::collections::HashSet; -use std::hash::{Hash, Hasher}; - -use opentelemetry::KeyValue; +use std::hash::Hash; /// Defines the window that an aggregation was calculated over. #[derive(Debug, Copy, Clone, Default, PartialEq, Eq, Hash)] @@ -106,60 +102,6 @@ pub enum Temporality { LowMemory, } -/// A unique set of attributes that can be used as instrument identifiers. -/// -/// This must implement [Hash], [PartialEq], and [Eq] so it may be used as -/// HashMap keys and other de-duplication methods. -#[derive(Clone, Default, Debug, PartialEq, Eq)] -pub(crate) struct AttributeSet(Vec, u64); - -impl From<&[KeyValue]> for AttributeSet { - fn from(values: &[KeyValue]) -> Self { - let mut seen_keys = HashSet::with_capacity(values.len()); - let vec = values - .iter() - .rev() - .filter_map(|kv| { - if seen_keys.insert(kv.key.clone()) { - Some(kv.clone()) - } else { - None - } - }) - .collect::>(); - - AttributeSet::new(vec) - } -} - -fn calculate_hash(values: &[KeyValue]) -> u64 { - let mut hasher = DefaultHasher::new(); - values.iter().fold(&mut hasher, |mut hasher, item| { - item.hash(&mut hasher); - hasher - }); - hasher.finish() -} - -impl AttributeSet { - fn new(mut values: Vec) -> Self { - values.sort_unstable_by(|a, b| a.key.cmp(&b.key)); - let hash = calculate_hash(&values); - AttributeSet(values, hash) - } - - /// Returns the underlying Vec of KeyValue pairs - pub(crate) fn into_vec(self) -> Vec { - self.0 - } -} - -impl Hash for AttributeSet { - fn hash(&self, state: &mut H) { - state.write_u64(self.1) - } -} - #[cfg(all(test, feature = "testing"))] mod tests { use self::data::{DataPoint, HistogramDataPoint, ScopeMetrics};