From de681cefa4474c52e646d0044a20e8884b3ffa81 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 4 Mar 2024 23:20:16 -0800 Subject: [PATCH 1/7] Add more tests to Metric SDK aggregation --- opentelemetry-sdk/src/metrics/mod.rs | 186 +++++++++++++++++++++++++++ 1 file changed, 186 insertions(+) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 7d41cdf18d..cbed8e557c 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -60,6 +60,8 @@ pub use view::*; #[cfg(all(test, feature = "testing"))] mod tests { + use std::borrow::Cow; + use self::data::ScopeMetrics; use super::*; use crate::metrics::data::{ResourceMetrics, Temporality}; use crate::metrics::reader::TemporalitySelector; @@ -210,6 +212,178 @@ mod tests { // Expecting 1 time-series. assert_eq!(sum.data_points.len(), 1); + let datapoint = &sum.data_points[0]; + assert_eq!(datapoint.value, 15); + } + + // "multi_thread" tokio flavor must be used else flush won't + // be able to make progress! + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn counter_duplicate_instrument_different_meter_no_merge() { + // Arrange + let exporter = InMemoryMetricsExporter::default(); + let reader = PeriodicReader::builder(exporter.clone(), runtime::Tokio).build(); + let meter_provider = SdkMeterProvider::builder().with_reader(reader).build(); + + // Act + let meter1 = meter_provider.meter("test.meter1"); + let meter2 = meter_provider.meter("test.meter2"); + let counter1 = meter1 + .u64_counter("my_counter") + .with_unit(Unit::new("my_unit")) + .with_description("my_description") + .init(); + + let counter2 = meter2 + .u64_counter("my_counter") + .with_unit(Unit::new("my_unit")) + .with_description("my_description") + .init(); + + let attribute = vec![KeyValue::new("key1", "value1")]; + counter1.add(10, &attribute); + counter2.add(5, &attribute); + + meter_provider.force_flush().unwrap(); + + // Assert + let resource_metrics = exporter + .get_finished_metrics() + .expect("metrics are expected to be exported."); + assert!( + resource_metrics[0].scope_metrics.len() == 2, + "There should be 2 separate scope" + ); + assert!( + resource_metrics[0].scope_metrics[0].metrics.len() == 1, + "There should be single metric for the scope" + ); + assert!( + resource_metrics[0].scope_metrics[1].metrics.len() == 1, + "There should be single metric for the scope" + ); + + let scope1 = find_scope_metric(&resource_metrics[0].scope_metrics, "test.meter1"); + let scope2 = find_scope_metric(&resource_metrics[0].scope_metrics, "test.meter2"); + + if let Some(scope1) = scope1 { + let metric1 = &scope1.metrics[0]; + assert_eq!(metric1.name, "my_counter"); + assert_eq!(metric1.unit.as_str(), "my_unit"); + assert_eq!(metric1.description, "my_description"); + let sum1 = metric1 + .data + .as_any() + .downcast_ref::>() + .expect("Sum aggregation expected for Counter instruments by default"); + + // Expecting 1 time-series. + assert_eq!(sum1.data_points.len(), 1); + + let datapoint1 = &sum1.data_points[0]; + assert_eq!(datapoint1.value, 10); + } else { + assert!(false, "No MetricScope found for 'test.meter1'"); + } + + if let Some(scope2) = scope2 { + let metric2 = &scope2.metrics[0]; + assert_eq!(metric2.name, "my_counter"); + assert_eq!(metric2.unit.as_str(), "my_unit"); + assert_eq!(metric2.description, "my_description"); + let sum2 = metric2 + .data + .as_any() + .downcast_ref::>() + .expect("Sum aggregation expected for Counter instruments by default"); + + // Expecting 1 time-series. + assert_eq!(sum2.data_points.len(), 1); + + let datapoint2 = &sum2.data_points[0]; + assert_eq!(datapoint2.value, 5); + } else { + assert!(false, "No MetricScope found for 'test.meter2'"); + } + } + + // "multi_thread" tokio flavor must be used else flush won't + // be able to make progress! + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn instrumentation_scope_identity_test() { + // Arrange + let exporter = InMemoryMetricsExporter::default(); + let reader = PeriodicReader::builder(exporter.clone(), runtime::Tokio).build(); + let meter_provider = SdkMeterProvider::builder().with_reader(reader).build(); + + // Act + // Meters are identical except for scope attributes, but scope attributes are not an identifying property. + // Hence there should be a single metric stream output for this test. + let meter1 = meter_provider.versioned_meter( + "test.meter1", + Some("v0.1.0"), + Some("schema_url"), + Some(vec![KeyValue::new("key", "value1")]), + ); + let meter2 = meter_provider.versioned_meter( + "test.meter1", + Some("v0.1.0"), + Some("schema_url"), + Some(vec![KeyValue::new("key", "value2")]), + ); + let counter1 = meter1 + .u64_counter("my_counter") + .with_unit(Unit::new("my_unit")) + .with_description("my_description") + .init(); + + let counter2 = meter2 + .u64_counter("my_counter") + .with_unit(Unit::new("my_unit")) + .with_description("my_description") + .init(); + + let attribute = vec![KeyValue::new("key1", "value1")]; + counter1.add(10, &attribute); + counter2.add(5, &attribute); + + meter_provider.force_flush().unwrap(); + + // Assert + let resource_metrics = exporter + .get_finished_metrics() + .expect("metrics are expected to be exported."); + println!("resource_metrics: {:?}", resource_metrics); + assert!( + resource_metrics[0].scope_metrics.len() == 1, + "There should be a single scope as the meters are identical" + ); + assert!( + resource_metrics[0].scope_metrics[0].metrics.len() == 1, + "There should be single metric for the scope as instruments are identical" + ); + + let scope = &resource_metrics[0].scope_metrics[0].scope; + assert_eq!(scope.name, "test.meter1"); + assert_eq!(scope.version, Some(Cow::Borrowed("v0.1.0"))); + assert_eq!(scope.schema_url, Some(Cow::Borrowed("schema_url"))); + + // Should we validate this, as this is a user error + assert_eq!(scope.attributes, vec![KeyValue::new("key", "value1")]); + + let metric = &resource_metrics[0].scope_metrics[0].metrics[0]; + assert_eq!(metric.name, "my_counter"); + assert_eq!(metric.unit.as_str(), "my_unit"); + assert_eq!(metric.description, "my_description"); + let sum = metric + .data + .as_any() + .downcast_ref::>() + .expect("Sum aggregation expected for Counter instruments by default"); + + // Expecting 1 time-series. + assert_eq!(sum.data_points.len(), 1); + let datapoint = &sum.data_points[0]; assert_eq!(datapoint.value, 15); } @@ -692,6 +866,18 @@ mod tests { assert!(resource_metrics.is_empty(), "No metrics should be exported as no new measurements were recorded since last collect."); } + fn find_scope_metric<'a>( + metrics: &'a Vec, + name: &'a str, + ) -> Option<&'a ScopeMetrics> { + for scope_metric in metrics { + if scope_metric.scope.name == name { + return Some(scope_metric); + } + } + None + } + struct DeltaTemporalitySelector(); impl TemporalitySelector for DeltaTemporalitySelector { fn temporality(&self, _kind: InstrumentKind) -> Temporality { From 73347e71af762a94c2c74f1ee09c7bbefd55e977 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 4 Mar 2024 23:21:39 -0800 Subject: [PATCH 2/7] lint fix --- opentelemetry-sdk/src/metrics/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index cbed8e557c..806bdf8755 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -60,7 +60,6 @@ pub use view::*; #[cfg(all(test, feature = "testing"))] mod tests { - use std::borrow::Cow; use self::data::ScopeMetrics; use super::*; use crate::metrics::data::{ResourceMetrics, Temporality}; @@ -72,6 +71,7 @@ mod tests { metrics::{MeterProvider as _, Unit}, KeyValue, }; + use std::borrow::Cow; // "multi_thread" tokio flavor must be used else flush won't // be able to make progress! @@ -214,7 +214,7 @@ mod tests { let datapoint = &sum.data_points[0]; assert_eq!(datapoint.value, 15); - } + } // "multi_thread" tokio flavor must be used else flush won't // be able to make progress! From 00ce441a74e13803e5b7e47f187d506fc15030c2 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 4 Mar 2024 23:23:26 -0800 Subject: [PATCH 3/7] fid --- opentelemetry-sdk/src/metrics/mod.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 806bdf8755..ce8ae6890b 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -870,12 +870,7 @@ mod tests { metrics: &'a Vec, name: &'a str, ) -> Option<&'a ScopeMetrics> { - for scope_metric in metrics { - if scope_metric.scope.name == name { - return Some(scope_metric); - } - } - None + metrics.iter().find(|&scope_metric| scope_metric.scope.name == name) } struct DeltaTemporalitySelector(); From 6829ed19525388c219593e47903e7e6a98131ded Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 4 Mar 2024 23:24:41 -0800 Subject: [PATCH 4/7] fix --- opentelemetry-sdk/src/metrics/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index ce8ae6890b..9872882b06 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -283,7 +283,7 @@ mod tests { let datapoint1 = &sum1.data_points[0]; assert_eq!(datapoint1.value, 10); } else { - assert!(false, "No MetricScope found for 'test.meter1'"); + panic!("No MetricScope found for 'test.meter1'"); } if let Some(scope2) = scope2 { @@ -303,7 +303,7 @@ mod tests { let datapoint2 = &sum2.data_points[0]; assert_eq!(datapoint2.value, 5); } else { - assert!(false, "No MetricScope found for 'test.meter2'"); + panic!("No MetricScope found for 'test.meter2'"); } } From e89805eba8a5cdaae6cfe2357c492e83139f5b98 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 4 Mar 2024 23:25:59 -0800 Subject: [PATCH 5/7] use slice --- opentelemetry-sdk/src/metrics/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 9872882b06..0557d49cd6 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -867,7 +867,7 @@ mod tests { } fn find_scope_metric<'a>( - metrics: &'a Vec, + metrics: &'a [ScopeMetrics], name: &'a str, ) -> Option<&'a ScopeMetrics> { metrics.iter().find(|&scope_metric| scope_metric.scope.name == name) From 88f55c4057eeb57d09bf5a2a4562b87c87d4adb1 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 4 Mar 2024 23:45:11 -0800 Subject: [PATCH 6/7] fmt --- opentelemetry-sdk/src/metrics/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 0557d49cd6..0906abdd1b 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -870,7 +870,9 @@ mod tests { metrics: &'a [ScopeMetrics], name: &'a str, ) -> Option<&'a ScopeMetrics> { - metrics.iter().find(|&scope_metric| scope_metric.scope.name == name) + metrics + .iter() + .find(|&scope_metric| scope_metric.scope.name == name) } struct DeltaTemporalitySelector(); From f92e64e3a16d50eda97af4197829398946bb3bb1 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Mon, 11 Mar 2024 22:24:35 -0700 Subject: [PATCH 7/7] comments --- opentelemetry-sdk/src/metrics/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 0906abdd1b..50673f7d25 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -320,13 +320,13 @@ mod tests { // Meters are identical except for scope attributes, but scope attributes are not an identifying property. // Hence there should be a single metric stream output for this test. let meter1 = meter_provider.versioned_meter( - "test.meter1", + "test.meter", Some("v0.1.0"), Some("schema_url"), Some(vec![KeyValue::new("key", "value1")]), ); let meter2 = meter_provider.versioned_meter( - "test.meter1", + "test.meter", Some("v0.1.0"), Some("schema_url"), Some(vec![KeyValue::new("key", "value2")]), @@ -364,11 +364,12 @@ mod tests { ); let scope = &resource_metrics[0].scope_metrics[0].scope; - assert_eq!(scope.name, "test.meter1"); + assert_eq!(scope.name, "test.meter"); assert_eq!(scope.version, Some(Cow::Borrowed("v0.1.0"))); assert_eq!(scope.schema_url, Some(Cow::Borrowed("schema_url"))); - // Should we validate this, as this is a user error + // This is validating current behavior, but it is not guaranteed to be the case in the future, + // as this is a user error and SDK reserves right to change this behavior. assert_eq!(scope.attributes, vec![KeyValue::new("key", "value1")]); let metric = &resource_metrics[0].scope_metrics[0].metrics[0];