Calls to with_resource for signal builders (Metrics, Logs, Traces) are now additive (#2677)

Co-authored-by: Lalit Kumar Bhasin <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>

Author: 3 people

opentelemetry-sdk/CHANGELOG.md

@@ -2,7 +2,9 @@
 
 ## vNext
 
-- *Breaking* Make `force_flush()` in `PushMetricExporter` synchronous
+- Calls to `MeterProviderBuilder::with_resource`, `TracerProviderBuilder::with_resource`, 
+  `LoggerProviderBuilder::with_resource` are now additive ([#2677](https://github.com/open-telemetry/opentelemetry-rust/pull/2677)).
+- *Breaking*: Make `force_flush()` in `PushMetricExporter` synchronous
 
 ## 0.28.0
 

opentelemetry-sdk/src/logs/logger_provider.rs

@@ -235,11 +235,16 @@ impl LoggerProviderBuilder {
     }
 
     /// The `Resource` to be associated with this Provider.
+    ///
+    /// *Note*: Calls to this method are additive, each call merges the provided
+    /// resource with the previous one.
     pub fn with_resource(self, resource: Resource) -> Self {
-        LoggerProviderBuilder {
-            resource: Some(resource),
-            ..self
-        }
+        let resource = match self.resource {
+            Some(existing) => Some(existing.merge(&resource)),
+            None => Some(resource),
+        };
+
+        LoggerProviderBuilder { resource, ..self }
     }
 
     /// Create a new provider from this configuration.
@@ -712,6 +717,35 @@ mod tests {
         assert_eq!(scoped_logger.instrumentation_scope().name(), "");
     }
 
+    #[test]
+    fn with_resource_multiple_calls_ensure_additive() {
+        let builder = SdkLoggerProvider::builder()
+            .with_resource(Resource::new(vec![KeyValue::new("key1", "value1")]))
+            .with_resource(Resource::new(vec![KeyValue::new("key2", "value2")]))
+            .with_resource(
+                Resource::builder_empty()
+                    .with_schema_url(vec![], "http://example.com")
+                    .build(),
+            )
+            .with_resource(Resource::new(vec![KeyValue::new("key3", "value3")]));
+
+        let resource = builder.resource.unwrap();
+
+        assert_eq!(
+            resource.get(&Key::from_static_str("key1")),
+            Some(Value::from("value1"))
+        );
+        assert_eq!(
+            resource.get(&Key::from_static_str("key2")),
+            Some(Value::from("value2"))
+        );
+        assert_eq!(
+            resource.get(&Key::from_static_str("key3")),
+            Some(Value::from("value3"))
+        );
+        assert_eq!(resource.schema_url(), Some("http://example.com"));
+    }
+
     #[derive(Debug)]
     pub(crate) struct LazyLogProcessor {
         shutdown_called: Arc<Mutex<bool>>,

opentelemetry-sdk/src/metrics/meter_provider.rs

@@ -233,9 +233,16 @@ impl MeterProviderBuilder {
     ///
     /// By default, if this option is not used, the default [Resource] will be used.
     ///
+    /// *Note*: Calls to this method are additive, each call merges the provided
+    /// resource with the previous one.
+    ///
     /// [Meter]: opentelemetry::metrics::Meter
     pub fn with_resource(mut self, resource: Resource) -> Self {
-        self.resource = Some(resource);
+        self.resource = match self.resource {
+            Some(existing) => Some(existing.merge(&resource)),
+            None => Some(resource),
+        };
+
         self
     }
 
@@ -322,6 +329,7 @@ impl fmt::Debug for MeterProviderBuilder {
 #[cfg(all(test, feature = "testing"))]
 mod tests {
     use crate::error::OTelSdkError;
+    use crate::metrics::SdkMeterProvider;
     use crate::resource::{
         SERVICE_NAME, TELEMETRY_SDK_LANGUAGE, TELEMETRY_SDK_NAME, TELEMETRY_SDK_VERSION,
     };
@@ -553,4 +561,33 @@ mod tests {
 
         assert_eq!(provider.inner.meters.lock().unwrap().len(), 5);
     }
+
+    #[test]
+    fn with_resource_multiple_calls_ensure_additive() {
+        let builder = SdkMeterProvider::builder()
+            .with_resource(Resource::new(vec![KeyValue::new("key1", "value1")]))
+            .with_resource(Resource::new(vec![KeyValue::new("key2", "value2")]))
+            .with_resource(
+                Resource::builder_empty()
+                    .with_schema_url(vec![], "http://example.com")
+                    .build(),
+            )
+            .with_resource(Resource::new(vec![KeyValue::new("key3", "value3")]));
+
+        let resource = builder.resource.unwrap();
+
+        assert_eq!(
+            resource.get(&Key::from_static_str("key1")),
+            Some(Value::from("value1"))
+        );
+        assert_eq!(
+            resource.get(&Key::from_static_str("key2")),
+            Some(Value::from("value2"))
+        );
+        assert_eq!(
+            resource.get(&Key::from_static_str("key3")),
+            Some(Value::from("value3"))
+        );
+        assert_eq!(resource.schema_url(), Some("http://example.com"));
+    }
 }

opentelemetry-sdk/src/resource/mod.rs

@@ -169,11 +169,11 @@ impl Resource {
     /// 5. If both resources do not have a schema url, the schema url will be empty.
     ///
     /// [Schema url]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.9.0/specification/schemas/overview.md#schema-url
-    fn merge<T: Deref<Target = Self>>(&self, other: T) -> Self {
-        if self.is_empty() {
+    pub(crate) fn merge<T: Deref<Target = Self>>(&self, other: T) -> Self {
+        if self.is_empty() && self.schema_url().is_none() {
             return other.clone();
         }
-        if other.is_empty() {
+        if other.is_empty() && other.schema_url().is_none() {
             return self.clone();
         }
         let mut combined_attrs = self.inner.attrs.clone();
@@ -419,18 +419,29 @@ mod tests {
     }
 
     #[rstest]
-    #[case(vec![], vec![KeyValue::new("key", "b")], "http://schema/a", None)]
-    #[case(vec![KeyValue::new("key", "a")], vec![KeyValue::new("key", "b")], "http://schema/a", Some("http://schema/a"))]
-    fn merge_resource_with_missing_attribtes(
+    #[case(vec![], vec![KeyValue::new("key", "b")], Some("http://schema/a"), None, Some("http://schema/a"))]
+    #[case(vec![KeyValue::new("key", "a")], vec![KeyValue::new("key", "b")], Some("http://schema/a"), None, Some("http://schema/a"))]
+    #[case(vec![KeyValue::new("key", "a")], vec![KeyValue::new("key", "b")], Some("http://schema/a"), None, Some("http://schema/a"))]
+    #[case(vec![KeyValue::new("key", "a")], vec![KeyValue::new("key", "b")], Some("http://schema/a"), Some("http://schema/b"), None)]
+    #[case(vec![KeyValue::new("key", "a")], vec![KeyValue::new("key", "b")], None, Some("http://schema/b"), Some("http://schema/b"))]
+    fn merge_resource_with_missing_attributes(
         #[case] key_values_a: Vec<KeyValue>,
         #[case] key_values_b: Vec<KeyValue>,
-        #[case] schema_url: &'static str,
+        #[case] schema_url_a: Option<&'static str>,
+        #[case] schema_url_b: Option<&'static str>,
         #[case] expected_schema_url: Option<&'static str>,
     ) {
-        let resource = Resource::from_schema_url(key_values_a, schema_url);
-        let other_resource = Resource::builder_empty()
-            .with_attributes(key_values_b)
-            .build();
+        let resource = match schema_url_a {
+            Some(schema) => Resource::from_schema_url(key_values_a, schema),
+            None => Resource::new(key_values_a),
+        };
+
+        let other_resource = match schema_url_b {
+            Some(schema) => Resource::builder_empty()
+                .with_schema_url(key_values_b, schema)
+                .build(),
+            None => Resource::new(key_values_b),
+        };
 
         assert_eq!(
             resource.merge(&other_resource).schema_url(),

opentelemetry-sdk/src/trace/provider.rs

@@ -293,6 +293,7 @@ impl opentelemetry::trace::TracerProvider for SdkTracerProvider {
 pub struct TracerProviderBuilder {
     processors: Vec<Box<dyn SpanProcessor>>,
     config: crate::trace::Config,
+    resource: Option<Resource>,
 }
 
 impl TracerProviderBuilder {
@@ -410,18 +411,28 @@ impl TracerProviderBuilder {
     ///
     /// By default, if this option is not used, the default [Resource] will be used.
     ///
+    /// *Note*: Calls to this method are additive, each call merges the provided
+    /// resource with the previous one.
+    ///
     /// [Tracer]: opentelemetry::trace::Tracer
     pub fn with_resource(self, resource: Resource) -> Self {
-        TracerProviderBuilder {
-            config: self.config.with_resource(resource),
-            ..self
-        }
+        let resource = match self.resource {
+            Some(existing) => Some(existing.merge(&resource)),
+            None => Some(resource),
+        };
+
+        TracerProviderBuilder { resource, ..self }
     }
 
     /// Create a new provider from this configuration.
     pub fn build(self) -> SdkTracerProvider {
         let mut config = self.config;
 
+        // Now, we can update the config with the resource.
+        if let Some(resource) = self.resource {
+            config = config.with_resource(resource);
+        };
+
         // Standard config will contain an owned [`Resource`] (either sdk default or use supplied)
         // we can optimize the common case with a static ref to avoid cloning the underlying
         // resource data for each span.
@@ -462,8 +473,8 @@ mod tests {
         SERVICE_NAME, TELEMETRY_SDK_LANGUAGE, TELEMETRY_SDK_NAME, TELEMETRY_SDK_VERSION,
     };
     use crate::trace::provider::TracerProviderInner;
-    use crate::trace::SpanData;
     use crate::trace::{Config, Span, SpanProcessor};
+    use crate::trace::{SdkTracerProvider, SpanData};
     use crate::Resource;
     use opentelemetry::trace::{Tracer, TracerProvider};
     use opentelemetry::{Context, Key, KeyValue, Value};
@@ -730,6 +741,39 @@ mod tests {
         assert!(test_tracer_1.provider().is_shutdown());
     }
 
+    #[test]
+    fn with_resource_multiple_calls_ensure_additive() {
+        let resource = SdkTracerProvider::builder()
+            .with_resource(Resource::new(vec![KeyValue::new("key1", "value1")]))
+            .with_resource(Resource::new(vec![KeyValue::new("key2", "value2")]))
+            .with_resource(
+                Resource::builder_empty()
+                    .with_schema_url(vec![], "http://example.com")
+                    .build(),
+            )
+            .with_resource(Resource::new(vec![KeyValue::new("key3", "value3")]))
+            .build()
+            .inner
+            .config
+            .resource
+            .clone()
+            .into_owned();
+
+        assert_eq!(
+            resource.get(&Key::from_static_str("key1")),
+            Some(Value::from("value1"))
+        );
+        assert_eq!(
+            resource.get(&Key::from_static_str("key2")),
+            Some(Value::from("value2"))
+        );
+        assert_eq!(
+            resource.get(&Key::from_static_str("key3")),
+            Some(Value::from("value3"))
+        );
+        assert_eq!(resource.schema_url(), Some("http://example.com"));
+    }
+
     #[derive(Debug)]
     struct CountingShutdownProcessor {
         shutdown_count: Arc<AtomicU32>,