iand675 / hs-opentelemetry

OpenTelemetry support for the Haskell programming language
https://iankduncan.com/projects/opentelemetry
BSD 3-Clause "New" or "Revised" License
76 stars 34 forks source link

Resource Merge Prioritizes Old Resource #111

Closed ehofreiter closed 2 weeks ago

ehofreiter commented 9 months ago

The function mergeResources in OpenTelemetry.Resource seems to be backwards. The documentation calls the first argument the old resource and the second argument the updating resource, meaning the second argument's attribute values should take precedence when a key is in both resources. However, due to the underlying HashMap's left-bias, the old value is taking precedence. For example:

l = mkResource ["host.name" .= ("left" :: Text)] :: Resource 'Nothing
r = mkResource ["host.name" .= ("right" :: Text)] :: Resource 'Nothing
getMaterializedResourcesAttributes . materializeResources $ mergeResources l r
-- Attributes {attributes = fromList [("host.name",AttributeValue (TextAttribute "left"))], attributesCount = 2, attributesDropped = 0}

As a result, getTracerProviderInitializationOptions' does not prioritize the user-provided resource attributes. This makes it very difficult to override the built-in resources (such as the host detector).

Similarly, I think getTracerProviderInitializationOptions' should prioritize the attributes read from OTEL_RESOURCE_ATTRIBUTES above the detected resources. Currently we have:

let allRs = mergeResources (builtInRs <> envVarRs) rs

Since <> is defined in a similar way as mergeResources and is also left-biased, both issues could be resolved by updating <> and mergeResources to be right-biased. Alternatively, the documentation for mergeResources could be updated to reflect the current behavior, and getTracerProviderInitializationOptions' could be updated to to reverse the order of arguments.