InstrumentationScope to include attributes in hash and eq check

Author: cijothomas

opentelemetry-sdk/src/metrics/meter_provider.rs

@@ -562,6 +562,25 @@ mod tests {
         assert_eq!(provider.inner.meters.lock().unwrap().len(), 5);
     }
 
+    #[test]
+    fn same_meter_reused_same_scope_attributes() {
+        let meter_provider = super::SdkMeterProvider::builder().build();
+        let make_scope = |attributes| {
+            InstrumentationScope::builder("test.meter")
+                .with_version("v0.1.0")
+                .with_schema_url("http://example.com")
+                .with_attributes(attributes)
+                .build()
+        };
+
+        let _meter1 =
+            meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")]));
+        let _meter2 =
+            meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")]));
+
+        assert_eq!(meter_provider.inner.meters.lock().unwrap().len(), 1);
+    }
+
     #[test]
     fn with_resource_multiple_calls_ensure_additive() {
         let builder = SdkMeterProvider::builder()

opentelemetry-sdk/src/metrics/mod.rs

@@ -782,7 +782,7 @@ mod tests {
             .build();
 
         // Act
-        // Meters are identical except for scope attributes, but scope attributes are not an identifying property.
+        // Meters are identical.
         // Hence there should be a single metric stream output for this test.
         let make_scope = |attributes| {
             InstrumentationScope::builder("test.meter")
@@ -795,7 +795,7 @@ mod tests {
         let meter1 =
             meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")]));
         let meter2 =
-            meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value2")]));
+            meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")]));
 
         let counter1 = meter1
             .u64_counter("my_counter")

opentelemetry/CHANGELOG.md

@@ -6,6 +6,7 @@
 - *Breaking* Moved `TraceError` enum from `opentelemetry::trace::TraceError` to `opentelemetry_sdk::trace::TraceError`
 - *Breaking* Moved `TraceResult` type alias from `opentelemetry::trace::TraceResult` to `opentelemetry_sdk::trace::TraceResult`
 - {PLACEHOLDER} - Remove the above completely. // TODO fill this when changes are actually in.
+- Bug Fix: `InstrumentationScope` implementation for `PartialEq` and `Hash` fixed to include Attributes also.
 
 ## 0.28.0
 

opentelemetry/src/common.rs

@@ -427,22 +427,33 @@ pub struct InstrumentationScope {
     attributes: Vec<KeyValue>,
 }
 
-// Uniqueness for InstrumentationScope does not depend on attributes
-impl Eq for InstrumentationScope {}
-
 impl PartialEq for InstrumentationScope {
     fn eq(&self, other: &Self) -> bool {
         self.name == other.name
             && self.version == other.version
             && self.schema_url == other.schema_url
+            && {
+                let mut self_attrs = self.attributes.clone();
+                let mut other_attrs = other.attributes.clone();
+                self_attrs.sort_unstable_by(|a, b| a.key.cmp(&b.key));
+                other_attrs.sort_unstable_by(|a, b| a.key.cmp(&b.key));
+                self_attrs == other_attrs
+            }
     }
 }
 
+impl Eq for InstrumentationScope {}
+
 impl hash::Hash for InstrumentationScope {
     fn hash<H: hash::Hasher>(&self, state: &mut H) {
         self.name.hash(state);
         self.version.hash(state);
         self.schema_url.hash(state);
+        let mut sorted_attrs = self.attributes.clone();
+        sorted_attrs.sort_unstable_by(|a, b| a.key.cmp(&b.key));
+        for attribute in sorted_attrs {
+            attribute.hash(state);
+        }
     }
 }
 
@@ -561,3 +572,69 @@ impl InstrumentationScopeBuilder {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::hash::{Hash, Hasher};
+
+    use crate::{InstrumentationScope, KeyValue};
+
+    #[test]
+    fn instrumentation_scope_equality() {
+        let scope1 = InstrumentationScope::builder("my-crate")
+            .with_version("v0.1.0")
+            .with_schema_url("https://opentelemetry.io/schemas/1.17.0")
+            .with_attributes([KeyValue::new("k", "v")])
+            .build();
+        let scope2 = InstrumentationScope::builder("my-crate")
+            .with_version("v0.1.0")
+            .with_schema_url("https://opentelemetry.io/schemas/1.17.0")
+            .with_attributes([KeyValue::new("k", "v")])
+            .build();
+        assert_eq!(scope1, scope2);
+    }
+
+    #[test]
+    fn instrumentation_scope_equality_attributes_diff_order() {
+        let scope1 = InstrumentationScope::builder("my-crate")
+            .with_version("v0.1.0")
+            .with_schema_url("https://opentelemetry.io/schemas/1.17.0")
+            .with_attributes([KeyValue::new("k1", "v1"), KeyValue::new("k2", "v2")])
+            .build();
+        let scope2 = InstrumentationScope::builder("my-crate")
+            .with_version("v0.1.0")
+            .with_schema_url("https://opentelemetry.io/schemas/1.17.0")
+            .with_attributes([KeyValue::new("k2", "v2"), KeyValue::new("k1", "v1")])
+            .build();
+        assert_eq!(scope1, scope2);
+
+        // assert hash are same for both scopes
+        let mut hasher1 = std::collections::hash_map::DefaultHasher::new();
+        scope1.hash(&mut hasher1);
+        let mut hasher2 = std::collections::hash_map::DefaultHasher::new();
+        scope2.hash(&mut hasher2);
+        assert_eq!(hasher1.finish(), hasher2.finish());
+    }
+
+    #[test]
+    fn instrumentation_scope_equality_different_attributes() {
+        let scope1 = InstrumentationScope::builder("my-crate")
+            .with_version("v0.1.0")
+            .with_schema_url("https://opentelemetry.io/schemas/1.17.0")
+            .with_attributes([KeyValue::new("k1", "v1"), KeyValue::new("k2", "v2")])
+            .build();
+        let scope2 = InstrumentationScope::builder("my-crate")
+            .with_version("v0.1.0")
+            .with_schema_url("https://opentelemetry.io/schemas/1.17.0")
+            .with_attributes([KeyValue::new("k2", "v3"), KeyValue::new("k4", "v5")])
+            .build();
+        assert_ne!(scope1, scope2);
+
+        // assert hash are same for both scopes
+        let mut hasher1 = std::collections::hash_map::DefaultHasher::new();
+        scope1.hash(&mut hasher1);
+        let mut hasher2 = std::collections::hash_map::DefaultHasher::new();
+        scope2.hash(&mut hasher2);
+        assert_ne!(hasher1.finish(), hasher2.finish());
+    }
+}