From 3a8123caaa17a3f8b1328618a4e4aec4d68ecca6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20K=C3=BChle?= <jkuehle90@gmail.com>
Date: Mon, 3 May 2021 20:05:12 +0200
Subject: [PATCH] Update resource merge behaviour (#537)

The resource merge behaviour changed 3 months ago to be consistent with
Span.SetAttribute. It now specifies:

> The resulting resource MUST have all attributes that are on any of the
> two input resources. If a key exists on both the old and updating
> resource, the value of the updating resource MUST be picked (even if
> the updated value is empty).

This updates sdk::Resource::merge to behave as described in the spec.
---
 opentelemetry/src/sdk/resource.rs | 47 +++++++++++--------------------
 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/opentelemetry/src/sdk/resource.rs b/opentelemetry/src/sdk/resource.rs
index 2d221fa859..a04d921777 100644
--- a/opentelemetry/src/sdk/resource.rs
+++ b/opentelemetry/src/sdk/resource.rs
@@ -19,7 +19,7 @@ use crate::sdk::EnvResourceDetector;
 use crate::{Key, KeyValue, Value};
 #[cfg(feature = "serialize")]
 use serde::{Deserialize, Serialize};
-use std::collections::{btree_map, btree_map::Entry, BTreeMap};
+use std::collections::{btree_map, BTreeMap};
 use std::time::Duration;
 
 /// Describes an entity about which identifying information and metadata is exposed.
@@ -56,7 +56,7 @@ impl Resource {
         let mut resource = Resource::empty();
 
         for kv in kvs.into_iter() {
-            resource.insert(kv);
+            resource.attrs.insert(kv.key, kv.value);
         }
 
         resource
@@ -71,7 +71,7 @@ impl Resource {
             let detected_res = detector.detect(timeout);
             for (key, value) in detected_res.into_iter() {
                 // using insert instead of merge to avoid clone.
-                resource.insert(KeyValue::new(key, value));
+                resource.attrs.insert(key, value);
             }
         }
 
@@ -80,7 +80,8 @@ impl Resource {
 
     /// Create a new `Resource` by combining two resources.
     ///
-    /// Keys from this resource have priority over keys from the merged resource.
+    /// Keys from the `other` resource have priority over keys from this resource, even if the
+    /// updated value is empty.
     pub fn merge(&self, other: &Self) -> Self {
         if self.attrs.is_empty() {
             return other.clone();
@@ -93,16 +94,10 @@ impl Resource {
 
         // attrs from self must be added first so they have priority
         for (k, v) in self.attrs.iter() {
-            resource.insert(KeyValue {
-                key: k.clone(),
-                value: v.clone(),
-            });
+            resource.attrs.insert(k.clone(), v.clone());
         }
         for (k, v) in other.attrs.iter() {
-            resource.insert(KeyValue {
-                key: k.clone(),
-                value: v.clone(),
-            });
+            resource.attrs.insert(k.clone(), v.clone());
         }
 
         resource
@@ -129,22 +124,6 @@ impl Resource {
     pub fn encoded(&self, encoder: &dyn labels::Encoder) -> String {
         encoder.encode(&mut self.into_iter())
     }
-
-    /// Insert a key-value pair into a `Resource`
-    fn insert(&mut self, item: KeyValue) {
-        match self.attrs.entry(item.key) {
-            Entry::Occupied(mut existing_item) => {
-                if let Value::String(s) = existing_item.get() {
-                    if s.is_empty() {
-                        existing_item.insert(item.value);
-                    }
-                }
-            }
-            Entry::Vacant(v) => {
-                v.insert(item.value);
-            }
-        }
-    }
 }
 
 /// An owned iterator over the entries of a `Resource`.
@@ -229,17 +208,23 @@ mod tests {
 
     #[test]
     fn merge_resource() {
-        let resource_a = Resource::new(vec![KeyValue::new("a", ""), KeyValue::new("b", "b-value")]);
+        let resource_a = Resource::new(vec![
+            KeyValue::new("a", ""),
+            KeyValue::new("b", "b-value"),
+            KeyValue::new("d", "d-value"),
+        ]);
 
         let resource_b = Resource::new(vec![
-            KeyValue::new("a", "final"),
+            KeyValue::new("a", "a-value"),
             KeyValue::new("c", "c-value"),
+            KeyValue::new("d", ""),
         ]);
 
         let mut expected_attrs = BTreeMap::new();
-        expected_attrs.insert(Key::new("a"), Value::from("final"));
+        expected_attrs.insert(Key::new("a"), Value::from("a-value"));
         expected_attrs.insert(Key::new("b"), Value::from("b-value"));
         expected_attrs.insert(Key::new("c"), Value::from("c-value"));
+        expected_attrs.insert(Key::new("d"), Value::from(""));
 
         assert_eq!(
             resource_a.merge(&resource_b),