Update Resource merge key conflict precedence (open-telemetry#1544)

srikanthccv · web-flow · commit 18761d78e30a · 2021-01-28T09:20:50.000-08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -79,6 +79,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#1536](https://github.com/open-telemetry/opentelemetry-python/pull/1536))
 - Fix TraceState to adhere to specs
   ([#1502](https://github.com/open-telemetry/opentelemetry-python/pull/1502))
+- Update Resource `merge` key conflict precedence
+  ([#1544](https://github.com/open-telemetry/opentelemetry-python/pull/1544))
 
 ### Removed
 - `opentelemetry-api` Remove ThreadLocalRuntimeContext since python3.4 is not supported.
diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py
@@ -155,16 +155,27 @@
 
 
 class Resource:
+    """A Resource is an immutable representation of the entity producing telemetry as Attributes.
+    """
+
     def __init__(self, attributes: Attributes):
         self._attributes = attributes.copy()
 
     @staticmethod
     def create(attributes: typing.Optional[Attributes] = None) -> "Resource":
+        """Creates a new `Resource` from attributes.
+
+        Args:
+            attributes: Optional zero or more key-value pairs.
+
+        Returns:
+            The newly-created Resource.
+        """
         if not attributes:
-            resource = _DEFAULT_RESOURCE
-        else:
-            resource = _DEFAULT_RESOURCE.merge(Resource(attributes))
-        resource = resource.merge(OTELResourceDetector().detect())
+            attributes = {}
+        resource = _DEFAULT_RESOURCE.merge(
+            OTELResourceDetector().detect()
+        ).merge(Resource(attributes))
         if not resource.attributes.get(SERVICE_NAME, None):
             default_service_name = "unknown_service"
             process_executable_name = resource.attributes.get(
@@ -186,11 +197,19 @@ def attributes(self) -> Attributes:
         return self._attributes.copy()
 
     def merge(self, other: "Resource") -> "Resource":
+        """Merges this resource and an updating resource into a new `Resource`.
+
+        If a key exists on both the old and updating resource, the value of the
+        updating resource will override the old resource value.
+
+        Args:
+            other: The other resource to be merged.
+
+        Returns:
+            The newly-created Resource.
+        """
         merged_attributes = self.attributes
-        # pylint: disable=protected-access
-        for key, value in other._attributes.items():
-            if key not in merged_attributes or merged_attributes[key] == "":
-                merged_attributes[key] = value
+        merged_attributes.update(other.attributes)
         return Resource(merged_attributes)
 
     def __eq__(self, other: object) -> bool:
diff --git a/opentelemetry-sdk/tests/resources/test_resources.py b/opentelemetry-sdk/tests/resources/test_resources.py
@@ -99,7 +99,7 @@ def test_resource_merge_empty_string(self):
         )
         self.assertEqual(
             left.merge(right),
-            resources.Resource({"service": "ui", "host": "service-host"}),
+            resources.Resource({"service": "not-ui", "host": "service-host"}),
         )
 
     def test_immutability(self):
@@ -154,7 +154,6 @@ def test_aggregated_resources_with_static_resource(self):
             static_resource,
         )
 
-        # Static resource values should never be overwritten
         resource_detector = mock.Mock(spec=resources.ResourceDetector)
         resource_detector.detect.return_value = resources.Resource(
             {"static_key": "try_to_overwrite_existing_value", "key": "value"}
@@ -163,7 +162,12 @@ def test_aggregated_resources_with_static_resource(self):
             resources.get_aggregated_resources(
                 [resource_detector], initial_resource=static_resource
             ),
-            resources.Resource({"static_key": "static_value", "key": "value"}),
+            resources.Resource(
+                {
+                    "static_key": "try_to_overwrite_existing_value",
+                    "key": "value",
+                }
+            ),
         )
 
     def test_aggregated_resources_multiple_detectors(self):
@@ -184,16 +188,15 @@ def test_aggregated_resources_multiple_detectors(self):
             }
         )
 
-        # New values should not overwrite existing values
         self.assertEqual(
             resources.get_aggregated_resources(
                 [resource_detector1, resource_detector2, resource_detector3]
             ),
             resources.Resource(
                 {
                     "key1": "value1",
-                    "key2": "value2",
-                    "key3": "value3",
+                    "key2": "try_to_overwrite_existing_value",
+                    "key3": "try_to_overwrite_existing_value",
                     "key4": "value4",
                 }
             ),
@@ -216,6 +219,21 @@ def test_resource_detector_raise_error(self):
             Exception, resources.get_aggregated_resources, [resource_detector]
         )
 
+    @mock.patch.dict(
+        os.environ,
+        {"OTEL_RESOURCE_ATTRIBUTES": "key1=env_value1,key2=env_value2"},
+    )
+    def test_env_priority(self):
+        resource_env = resources.Resource.create()
+        self.assertEqual(resource_env.attributes["key1"], "env_value1")
+        self.assertEqual(resource_env.attributes["key2"], "env_value2")
+
+        resource_env_override = resources.Resource.create(
+            {"key1": "value1", "key2": "value2"}
+        )
+        self.assertEqual(resource_env_override.attributes["key1"], "value1")
+        self.assertEqual(resource_env_override.attributes["key2"], "value2")
+
 
 class TestOTELResourceDetector(unittest.TestCase):
     def setUp(self) -> None:
