Facilitate merging of resource attribute sources

[breedx-splk]

So the spec is pretty clear about the resource attributes env being treated as an otel Resource that should be merged with the user-provided resource.

Currently, in the case where the environment provides OTEL_RESOURCE_ATTRIBUTES and the user has provided system property otel.resource.attributes the latter completely overwrites the former. Because this is a packed field and we do the blending at the configuration stage (instead of actually merging separate resources from each source), we need to account for this to be spec compliant. Currently, the system property can null out packed fields originating in OTEL_RESOURCE_ATTRIBUTES which is effectively an incorrect merge.

This change special cases the resource attribute config item and concatenates the system property onto any existing environment variable field. That way, when the config item is turned into a list, the latter items will replace existing but not null out existing, as is the case today.

I gave some consideration to creating a more sophisticated ConfigProperties implementation that held onto both env and system configs and default until it was time to create the resource in AutoConfiguredOpenTelemetrySdkBuilder -- but that seemed like a much larger and wide reaching change. Since we're blending configs and losing context pretty early, I thought it made a bit more chance to do this surgical change instead.

[mateuszrzeszutek]

Do you think this behavior is something we need just for resource attributes? Or maps in general?

[jkwatson]

How is the env var not also user-provided? The spec doesn't distinguish between system properties and env vars because system properties are a java-specific way to inject information into the java process. We had chosen this order of precedence (IIRC) to match what spring does, to be the strategy that might be the least surprising to users.

[breedx-splk]

How is the env var not also user-provided?

Fair nuff, completely depends on the deployment scheme and the definition of "user" versus "ops" or "admin" or whatever. Are you using that to suggest that if both env and sysprops are "user-provided" that the portion of spec I linked to is irrelevant?

The spec doesn't distinguish between system properties and env vars because system properties are a java-specific way to inject information into the java process.

Totally, I'd never expect the spec to talk specifically about jvm sysprops in that context.

We had chosen this order of precedence (IIRC) to match what spring does, to be the strategy that might be the least surprising to users.

Right, and hopefully I've maintained that ordering. What I think would be surprising to users is if they had a portion of their resource get clobbered...regardless of the order.

[codecov[bot]]

Codecov Report

Base: 90.77% // Head: 90.77% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (1eb1a8d) compared to base (cdab465). Patch coverage: 92.85% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #4758   +/-   ##
=========================================
  Coverage     90.77%   90.77%           
- Complexity     4814     4819    +5     
=========================================
  Files           551      551           
  Lines         14356    14368   +12     
  Branches       1395     1399    +4     
=========================================
+ Hits          13031    13043   +12     
  Misses          909      909           
  Partials        416      416           

Impacted Files	Coverage Δ
...try/sdk/autoconfigure/DefaultConfigProperties.java	93.85% <92.85%> (-0.26%)	:arrow_down:
...entelemetry/sdk/logs/export/BatchLogProcessor.java	89.47% <0.00%> (+0.75%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[jkwatson]

How is the env var not also user-provided?

Fair nuff, completely depends on the deployment scheme and the definition of "user" versus "ops" or "admin" or whatever. Are you using that to suggest that if both env and sysprops are "user-provided" that the portion of spec I linked to is irrelevant?

Mostly that I didn't read what you wrote carefully enough. Consider my comments as color-commentary, and not saying 'no'. I do agree we shouldn't clobber one with the other, if they aren't overlapping. I would consider that as a bug, honestly.

[breedx-splk]

Do you think this behavior is something we need just for resource attributes? Or maps in general?

I just don't know of the other use cases for Maps. A generalized solution might be better, but I wanted to be strategic and address the resource attribute here.

[jack-berg]

I think we should take a hardline stance and say you can specify configuration via system properties OR environment variables, but not both. Map type properties would be the only exception to the general rule that system properties trump environment variables since merging doesn't make sense for any of the primitive types or arrays. If a user wants to merge additional resource attributes with some base set defined via system props OR env vars, they can register a resource provider, or use AutoConfiguredOpenTelemetrySdkBuilder#addResourceCustomizer.

What I think would be surprising to users is if they had a portion of their resource get clobbered...regardless of the order.

I don't buy that. It should be obvious that they're playing with fire trying specifying one configurable parameter in two different ways. I think the priority based approach we currently use is more intuitive than a merge. Priority is obviously the only possible approach if a primitive type is configured multiple ways, and users would expect that. Why would they expect different behavior just because the parameter is a map?

[trask]

[I added to SIG agenda for tomorrow]

[breedx-splk]

We did indeed discuss this at length in the SIG meeting. I think the TL;DR is that this doesn't seem to be terribly compelling for most folks, hasn't been encountered in the wild, the spec is a tad ambiguous wrt sysprops, and some folks believe that the map should just stomp the other map.

So I am closing this with the expectation that we can find a good place to document this. We may also attempt to omit the Resource building from the configuration code and instead try out separate ResourceProviders (one sourcing from environment, one sourcing from sysprops).