Resource merge precedence (#1728)

* Reversing this versus other in merge priority * revised test for new behavior * Changelog updated * Changelog updated * remove extra period * Fixing more period issues/line breaks Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
open-telemetry · Jan 28, 2021 · 92948e6 · 92948e6
1 parent 9196e56
commit 92948e6
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 19 deletions.
diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -26,6 +26,10 @@
   them to supported data types (long for int/short, double for float). For
   invalid attributes we now throw an exception instead of logging an error.
   ([#1720](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1720))
+* Merging "this" resource with an "other" resource now prioritizes the "other"
+  resource's attributes in a conflict. We've rectified to follow a recent
+  change to the spec. We previously prioritized "this" resource's tags.
+  ([#1728](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1728))
 
 ## 1.0.0-rc1.1
 

diff --git a/src/OpenTelemetry/Resources/Resource.cs b/src/OpenTelemetry/Resources/Resource.cs
@@ -57,34 +57,34 @@ internal Resource(IEnumerable<KeyValuePair<string, object>> attributes)
         public IEnumerable<KeyValuePair<string, object>> Attributes { get; }
 
         /// <summary>
-        /// Returns a new, merged <see cref="Resource"/> by merging the current <see cref="Resource"/> with the.
-        /// <code>other</code> <see cref="Resource"/>. In case of a collision the current <see cref="Resource"/> takes precedence.
+        /// Returns a new, merged <see cref="Resource"/> by merging the old <see cref="Resource"/> with the
+        /// <c>other</c> <see cref="Resource"/>. In case of a collision the other <see cref="Resource"/> takes precedence.
         /// </summary>
-        /// <param name="other">The <see cref="Resource"/> that will be merged with. <code>this</code>.</param>
+        /// <param name="other">The <see cref="Resource"/> that will be merged with <c>this</c>.</param>
         /// <returns><see cref="Resource"/>.</returns>
         public Resource Merge(Resource other)
         {
             var newAttributes = new Dictionary<string, object>();
 
-            foreach (var attribute in this.Attributes)
-            {
-                if (!newAttributes.TryGetValue(attribute.Key, out var value) || (value is string strValue && string.IsNullOrEmpty(strValue)))
-                {
-                    newAttributes[attribute.Key] = attribute.Value;
-                }
-            }
-
             if (other != null)
             {
                 foreach (var attribute in other.Attributes)
                 {
-                    if (!newAttributes.TryGetValue(attribute.Key, out var value) || (value is string strValue && string.IsNullOrEmpty(strValue)))
+                    if (!newAttributes.TryGetValue(attribute.Key, out var value))
                     {
                         newAttributes[attribute.Key] = attribute.Value;
                     }
                 }
             }
 
+            foreach (var attribute in this.Attributes)
+            {
+                if (!newAttributes.TryGetValue(attribute.Key, out var value))
+                {
+                    newAttributes[attribute.Key] = attribute.Value;
+                }
+            }
+
             return new Resource(newAttributes);
         }
 

diff --git a/test/OpenTelemetry.Tests/Resources/ResourceTest.cs b/test/OpenTelemetry.Tests/Resources/ResourceTest.cs
@@ -315,19 +315,19 @@ public void MergeResource_MultiAttributeSource_DuplicatedKeysInPrimary()
         }
 
         [Fact]
-        public void MergeResource_SecondaryCanOverridePrimaryEmptyAttributeValue()
+        public void MergeResource_UpdatingResourceOverridesCurrentResource()
         {
             // Arrange
-            var primaryAttributes = new Dictionary<string, object> { { "value", string.Empty } };
-            var secondaryAttributes = new Dictionary<string, object> { { "value", "not empty" } };
-            var primaryResource = new Resource(primaryAttributes);
-            var secondaryResource = new Resource(secondaryAttributes);
+            var currentAttributes = new Dictionary<string, object> { { "value", "currentValue" } };
+            var updatingAttributes = new Dictionary<string, object> { { "value", "updatedValue" } };
+            var currentResource = new Resource(currentAttributes);
+            var updatingResource = new Resource(updatingAttributes);
 
-            var newResource = primaryResource.Merge(secondaryResource);
+            var newResource = currentResource.Merge(updatingResource);
 
             // Assert
             Assert.Single(newResource.Attributes);
-            Assert.Contains(new KeyValuePair<string, object>("value", "not empty"), newResource.Attributes);
+            Assert.Contains(new KeyValuePair<string, object>("value", "updatedValue"), newResource.Attributes);
         }
 
         [Fact]