From 4367098562d26d0d2b13ff7fe9ff0136ad6d5f42 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 20 May 2025 18:22:13 -0700 Subject: [PATCH 1/4] feat: Move subset of Metric views outside of experimental feature --- examples/metrics-advanced/Cargo.toml | 2 +- examples/metrics-advanced/src/main.rs | 90 +++++-------------- opentelemetry-sdk/CHANGELOG.md | 9 +- opentelemetry-sdk/src/metrics/aggregation.rs | 2 +- opentelemetry-sdk/src/metrics/instrument.rs | 4 +- .../src/metrics/meter_provider.rs | 1 - opentelemetry-sdk/src/metrics/mod.rs | 58 ++++++------ opentelemetry-sdk/src/metrics/view.rs | 1 - 8 files changed, 63 insertions(+), 104 deletions(-) diff --git a/examples/metrics-advanced/Cargo.toml b/examples/metrics-advanced/Cargo.toml index 734f890748..40a4573d68 100644 --- a/examples/metrics-advanced/Cargo.toml +++ b/examples/metrics-advanced/Cargo.toml @@ -13,6 +13,6 @@ bench = false [dependencies] opentelemetry = { path = "../../opentelemetry", features = ["metrics"] } -opentelemetry_sdk = { path = "../../opentelemetry-sdk", features = ["spec_unstable_metrics_views"] } +opentelemetry_sdk = { path = "../../opentelemetry-sdk" } opentelemetry-stdout = { workspace = true, features = ["metrics"] } tokio = { workspace = true, features = ["full"] } diff --git a/examples/metrics-advanced/src/main.rs b/examples/metrics-advanced/src/main.rs index 27150b522c..19c0cd0e89 100644 --- a/examples/metrics-advanced/src/main.rs +++ b/examples/metrics-advanced/src/main.rs @@ -1,7 +1,6 @@ use opentelemetry::global; -use opentelemetry::Key; use opentelemetry::KeyValue; -use opentelemetry_sdk::metrics::{Aggregation, Instrument, SdkMeterProvider, Stream, Temporality}; +use opentelemetry_sdk::metrics::{Instrument, SdkMeterProvider, Stream, Temporality}; use opentelemetry_sdk::Resource; use std::error::Error; @@ -20,23 +19,9 @@ fn init_meter_provider() -> opentelemetry_sdk::metrics::SdkMeterProvider { }; // for example 2 - let my_view_drop_attributes = |i: &Instrument| { - if i.name == "my_counter" { - Some(Stream::new().allowed_attribute_keys(vec![Key::from("mykey1")])) - } else { - None - } - }; - - // for example 3 - let my_view_change_aggregation = |i: &Instrument| { + let my_view_change_cardinality = |i: &Instrument| { if i.name == "my_second_histogram" { - Some( - Stream::new().aggregation(Aggregation::ExplicitBucketHistogram { - boundaries: vec![0.9, 1.0, 1.1, 1.2, 1.3, 1.4, 1.5], - record_min_max: false, - }), - ) + Some(Stream::new().cardinality_limit(2)) } else { None } @@ -55,8 +40,7 @@ fn init_meter_provider() -> opentelemetry_sdk::metrics::SdkMeterProvider { .with_periodic_exporter(exporter) .with_resource(resource) .with_view(my_view_rename_and_unit) - .with_view(my_view_drop_attributes) - .with_view(my_view_change_aggregation) + .with_view(my_view_change_cardinality) .build(); global::set_meter_provider(provider.clone()); provider @@ -88,27 +72,7 @@ async fn main() -> Result<(), Box> { ], ); - // Example 2 - Drop unwanted attributes using view. - let counter = meter.u64_counter("my_counter").build(); - - // Record measurements using the Counter instrument. - // Though we are passing 4 attributes here, only 1 will be used - // for aggregation as view is configured to use only "mykey1" - // attribute. - counter.add( - 10, - &[ - KeyValue::new("mykey1", "myvalue1"), - KeyValue::new("mykey2", "myvalue2"), - KeyValue::new("mykey3", "myvalue3"), - KeyValue::new("mykey4", "myvalue4"), - ], - ); - - // Example 3 - Change Aggregation configuration using View. - // Histograms are by default aggregated using ExplicitBucketHistogram - // with default buckets. The configured view will change the aggregation to - // use a custom set of boundaries, and min/max values will not be recorded. + // Example 2 - Change cardinality using View. let histogram2 = meter .f64_histogram("my_second_histogram") .with_unit("ms") @@ -116,37 +80,23 @@ async fn main() -> Result<(), Box> { .build(); // Record measurements using the histogram instrument. - // The values recorded are in the range of 1.2 to 1.5, warranting - // the change of boundaries. - histogram2.record( - 1.5, - &[ - KeyValue::new("mykey1", "myvalue1"), - KeyValue::new("mykey2", "myvalue2"), - KeyValue::new("mykey3", "myvalue3"), - KeyValue::new("mykey4", "myvalue4"), - ], - ); + // This metric will have a cardinality limit of 2, + // as set in the view. Because of this, only the first two + // measurements will be recorded, and the rest will be folded + // into the overflow attribute. + histogram2.record(1.5, &[KeyValue::new("mykey1", "v1")]); - histogram2.record( - 1.2, - &[ - KeyValue::new("mykey1", "myvalue1"), - KeyValue::new("mykey2", "myvalue2"), - KeyValue::new("mykey3", "myvalue3"), - KeyValue::new("mykey4", "myvalue4"), - ], - ); + histogram2.record(1.2, &[KeyValue::new("mykey1", "v2")]); - histogram2.record( - 1.23, - &[ - KeyValue::new("mykey1", "myvalue1"), - KeyValue::new("mykey2", "myvalue2"), - KeyValue::new("mykey3", "myvalue3"), - KeyValue::new("mykey4", "myvalue4"), - ], - ); + histogram2.record(1.23, &[KeyValue::new("mykey1", "v3")]); + + histogram2.record(1.4, &[KeyValue::new("mykey1", "v4")]); + + histogram2.record(1.6, &[KeyValue::new("mykey1", "v5")]); + + histogram2.record(1.7, &[KeyValue::new("mykey1", "v6")]); + + histogram2.record(1.8, &[KeyValue::new("mykey1", "v7")]); // Metrics are exported by default every 30 seconds when using stdout exporter, // however shutting down the MeterProvider here instantly flushes diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index c813c74625..a062bf23bf 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -43,8 +43,13 @@ also modified to suppress telemetry before invoking exporters. [#2928](https://github.com/open-telemetry/opentelemetry-rust/pull/2928) - TODO: Placeholder for View related changelog. Polish this after all - changes are done. - Hide public fields from `Stream` struct. + - The `Stream` struct now has its public fields hidden. + - Core view functionality is now available by default—users can change the + name, unit, description, and cardinality limit of a metric via views without + enabling the `spec_unstable_metrics_views` feature flag. Advanced view + features, such as custom aggregation or attribute filtering, still require + the `spec_unstable_metrics_views` feature. + - TODO: Add Stream::builder() pattern change, validation when done. - *Breaking* `Aggregation` enum moved behind feature flag "spec_unstable_metrics_views". This was only required when using Views. diff --git a/opentelemetry-sdk/src/metrics/aggregation.rs b/opentelemetry-sdk/src/metrics/aggregation.rs index 4c0804f319..02bdb1e215 100644 --- a/opentelemetry-sdk/src/metrics/aggregation.rs +++ b/opentelemetry-sdk/src/metrics/aggregation.rs @@ -150,9 +150,9 @@ impl Aggregation { #[cfg(test)] mod tests { + use super::Aggregation; use crate::metrics::error::{MetricError, MetricResult}; use crate::metrics::internal::{EXPO_MAX_SCALE, EXPO_MIN_SCALE}; - use crate::metrics::Aggregation; #[test] fn validate_aggregation() { diff --git a/opentelemetry-sdk/src/metrics/instrument.rs b/opentelemetry-sdk/src/metrics/instrument.rs index 63464fa5f1..2fffbeca42 100644 --- a/opentelemetry-sdk/src/metrics/instrument.rs +++ b/opentelemetry-sdk/src/metrics/instrument.rs @@ -83,7 +83,6 @@ impl InstrumentKind { /// ``` #[derive(Clone, Default, Debug, PartialEq)] #[non_exhaustive] -#[allow(unreachable_pub)] pub struct Instrument { /// The human-readable identifier of the instrument. pub name: Cow<'static, str>, @@ -208,7 +207,6 @@ pub struct Stream { pub(crate) cardinality_limit: Option, } -#[cfg(feature = "spec_unstable_metrics_views")] impl Stream { /// Create a new stream with empty values. pub fn new() -> Self { @@ -233,12 +231,14 @@ impl Stream { self } + #[cfg(feature = "spec_unstable_metrics_views")] /// Set the stream aggregation. pub fn aggregation(mut self, aggregation: Aggregation) -> Self { self.aggregation = Some(aggregation); self } + #[cfg(feature = "spec_unstable_metrics_views")] /// Set the stream allowed attribute keys. /// /// Any attribute recorded for the stream with a key not in this set will be diff --git a/opentelemetry-sdk/src/metrics/meter_provider.rs b/opentelemetry-sdk/src/metrics/meter_provider.rs index c4d5f86c32..cc6e85f0d2 100644 --- a/opentelemetry-sdk/src/metrics/meter_provider.rs +++ b/opentelemetry-sdk/src/metrics/meter_provider.rs @@ -287,7 +287,6 @@ impl MeterProviderBuilder { self } - #[cfg(feature = "spec_unstable_metrics_views")] /// Associates a [View] with a [MeterProvider]. /// /// [View]s are appended to existing ones in a [MeterProvider] if this option is diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index c98d7affa8..7d4a0e058a 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -82,11 +82,7 @@ pub use periodic_reader::*; #[cfg(feature = "experimental_metrics_custom_reader")] pub use pipeline::Pipeline; -#[cfg(feature = "experimental_metrics_custom_reader")] -pub use instrument::InstrumentKind; - -#[cfg(feature = "spec_unstable_metrics_views")] -pub use instrument::*; +pub use instrument::{Instrument, InstrumentKind, Stream}; #[cfg(feature = "spec_unstable_metrics_views")] pub use view::*; @@ -137,6 +133,7 @@ mod tests { use std::sync::{Arc, Mutex}; use std::thread; use std::time::Duration; + use super::view::View; // Run all tests in this mod // cargo test metrics::tests --features=testing,spec_unstable_metrics_views @@ -883,23 +880,28 @@ mod tests { } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + #[ignore = "TODO: This test should be fixed when Stream is moved to proper builder pattern."] async fn histogram_aggregation_with_invalid_aggregation_should_proceed_as_if_view_not_exist() { // Run this test with stdout enabled to see output. // cargo test histogram_aggregation_with_invalid_aggregation_should_proceed_as_if_view_not_exist --features=testing -- --nocapture // Arrange let exporter = InMemoryMetricExporter::default(); - let criteria = Instrument::new().name("test_histogram"); - let stream_invalid_aggregation = Stream::new() - .aggregation(aggregation::Aggregation::ExplicitBucketHistogram { - boundaries: vec![0.9, 1.9, 1.2, 1.3, 1.4, 1.5], // invalid boundaries - record_min_max: false, - }) - .name("test_histogram_renamed") - .unit("test_unit_renamed"); - - let view = - new_view(criteria, stream_invalid_aggregation).expect("Expected to create a new view"); + let view = |i: &Instrument| { + if i.name == "test_histogram" { + Some( + Stream::new() + .aggregation(aggregation::Aggregation::ExplicitBucketHistogram { + boundaries: vec![0.9, 1.9, 1.2, 1.3, 1.4, 1.5], // invalid boundaries + record_min_max: false, + }) + .name("test_histogram_renamed") + .unit("test_unit_renamed"), + ) + } else { + None + } + }; let meter_provider = SdkMeterProvider::builder() .with_periodic_exporter(exporter.clone()) .with_view(view) @@ -938,12 +940,14 @@ mod tests { // Arrange let exporter = InMemoryMetricExporter::default(); - let criteria = Instrument::new().name("my_observable_counter"); // View drops all attributes. - let stream_invalid_aggregation = Stream::new().allowed_attribute_keys(vec![]); - - let view = - new_view(criteria, stream_invalid_aggregation).expect("Expected to create a new view"); + let view = |i: &Instrument| { + if i.name == "my_observable_counter" { + Some(Stream::new().allowed_attribute_keys(vec![])) + } else { + None + } + }; let meter_provider = SdkMeterProvider::builder() .with_periodic_exporter(exporter.clone()) .with_view(view) @@ -1012,12 +1016,14 @@ mod tests { // Arrange let exporter = InMemoryMetricExporter::default(); - let criteria = Instrument::new().name("my_counter"); // View drops all attributes. - let stream_invalid_aggregation = Stream::new().allowed_attribute_keys(vec![]); - - let view = - new_view(criteria, stream_invalid_aggregation).expect("Expected to create a new view"); + let view = |i: &Instrument| { + if i.name == "my_counter" { + Some(Stream::new().allowed_attribute_keys(vec![])) + } else { + None + } + }; let meter_provider = SdkMeterProvider::builder() .with_periodic_exporter(exporter.clone()) .with_view(view) diff --git a/opentelemetry-sdk/src/metrics/view.rs b/opentelemetry-sdk/src/metrics/view.rs index eab2916664..a3ab2a4577 100644 --- a/opentelemetry-sdk/src/metrics/view.rs +++ b/opentelemetry-sdk/src/metrics/view.rs @@ -45,7 +45,6 @@ fn empty_view(_inst: &Instrument) -> Option { /// let provider = SdkMeterProvider::builder().with_view(my_view).build(); /// # drop(provider) /// ``` -#[allow(unreachable_pub)] pub trait View: Send + Sync + 'static { /// Defines how data should be collected for certain instruments. /// From 243232f36d9c45eff60059f3c70201b76f93b04d Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 20 May 2025 18:26:55 -0700 Subject: [PATCH 2/4] removed async. --- opentelemetry-sdk/src/metrics/mod.rs | 44 ++++++++++++---------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 7d4a0e058a..e193481bdc 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -117,6 +117,7 @@ mod tests { use self::data::{HistogramDataPoint, ScopeMetrics, SumDataPoint}; use super::data::MetricData; use super::internal::Number; + use super::view::View; use super::*; use crate::metrics::data::ResourceMetrics; use crate::metrics::internal::AggregatedMetricsAccess; @@ -133,7 +134,6 @@ mod tests { use std::sync::{Arc, Mutex}; use std::thread; use std::time::Duration; - use super::view::View; // Run all tests in this mod // cargo test metrics::tests --features=testing,spec_unstable_metrics_views @@ -1469,8 +1469,8 @@ mod tests { ); } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_rename() { + #[test] + fn view_test_rename() { test_view_customization( |i| { if i.name == "my_counter" { @@ -1483,11 +1483,10 @@ mod tests { "my_unit", "my_description", ) - .await; } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_change_unit() { + #[test] + fn view_test_change_unit() { test_view_customization( |i| { if i.name == "my_counter" { @@ -1500,11 +1499,10 @@ mod tests { "my_unit_new", "my_description", ) - .await; } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_change_description() { + #[test] + fn view_test_change_description() { test_view_customization( |i| { if i.name == "my_counter" { @@ -1517,11 +1515,10 @@ mod tests { "my_unit", "my_description_new", ) - .await; } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_change_name_unit() { + #[test] + fn view_test_change_name_unit() { test_view_customization( |i| { if i.name == "my_counter" { @@ -1534,11 +1531,10 @@ mod tests { "my_unit_new", "my_description", ) - .await; } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_change_name_unit_desc() { + #[test] + fn view_test_change_name_unit_desc() { test_view_customization( |i| { if i.name == "my_counter" { @@ -1556,11 +1552,10 @@ mod tests { "my_unit_new", "my_description_new", ) - .await; } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_match_unit() { + #[test] + fn view_test_match_unit() { test_view_customization( |i| { if i.unit == "my_unit" { @@ -1573,11 +1568,10 @@ mod tests { "my_unit_new", "my_description", ) - .await; } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_match_none() { + #[test] + fn view_test_match_none() { test_view_customization( |i| { if i.name == "not_expected_to_match" { @@ -1590,11 +1584,10 @@ mod tests { "my_unit", "my_description", ) - .await; } - #[tokio::test(flavor = "multi_thread", worker_threads = 1)] - async fn view_test_match_multiple() { + #[test] + fn view_test_match_multiple() { test_view_customization( |i| { if i.name == "my_counter" && i.unit == "my_unit" { @@ -1607,11 +1600,10 @@ mod tests { "my_unit", "my_description", ) - .await; } /// Helper function to test view customizations - async fn test_view_customization( + fn test_view_customization( view_function: F, expected_name: &str, expected_unit: &str, From e33b04b7fb48ddf24658d2a99644cc71a0a360fa Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 20 May 2025 18:31:47 -0700 Subject: [PATCH 3/4] todo --- opentelemetry-sdk/src/metrics/pipeline.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index e4d62b3955..562ccb1ea3 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -363,6 +363,9 @@ where kind: InstrumentKind, mut stream: Stream, ) -> MetricResult>>> { + // TODO: Create a separate pub (crate) Stream struct for the pipeline, + // as Stream will not have any Optional fields as None at this point and + // new struct can better reflect this. let mut agg = stream .aggregation .take() From 3c81b57adab59ebf014a740aade90d7a303a70d2 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Tue, 20 May 2025 21:01:35 -0700 Subject: [PATCH 4/4] Update opentelemetry-sdk/src/metrics/pipeline.rs --- opentelemetry-sdk/src/metrics/pipeline.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index 562ccb1ea3..e012211ffc 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -364,7 +364,7 @@ where mut stream: Stream, ) -> MetricResult>>> { // TODO: Create a separate pub (crate) Stream struct for the pipeline, - // as Stream will not have any Optional fields as None at this point and + // as Stream will not have any optional fields as None at this point and // new struct can better reflect this. let mut agg = stream .aggregation