optimize resource detection

[zeitlinger]

optimize resource detection by skipping providers that would add no new keys

Most of the functionality is already working with the following pattern: give the lower priority detector a higher order, so that it runs later an can check if it should be skipped

• use shouldApply
• use later order

However, the environment resource provider runs last - so if the user adds the service name using service.name - the jar service name provider would still be run in vain.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (48d41d3) 91.05% compared to head (a8b30ea) 91.05%.

Files	Patch %	Lines
.../java/io/opentelemetry/sdk/resources/Resource.java	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6222      +/-   ##
============================================
- Coverage     91.05%   91.05%   -0.01%     
+ Complexity     5687     5685       -2     
============================================
  Files           621      622       +1     
  Lines         16642    16660      +18     
  Branches       1703     1706       +3     
============================================
+ Hits          15153    15169      +16     
  Misses          998      998              
- Partials        491      493       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jack-berg]

so if the user adds the service name using service.name - the jar service name provider would still be run in vain.

Can the jar service name provider check for the presence of service.name in config properties and achieve the same affect?

[zeitlinger]

so if the user adds the service name using service.name - the jar service name provider would still be run in vain.

Can the jar service name provider check for the presence of service.name in config properties and achieve the same affect?

After re-reading the shouldApply - yes, this will work. I didn't realize the trick to look both into the "existing" and "config" parameters.
So I guess this PR is making it easier to get resource providers right.

[jack-berg]

So I guess this PR is making it easier to get resource providers right.

Maybe, but I'd argue that its making them more confusing. The behavior of ResourceProvider#supportedKeys() is not intuitive: If all of supportedKeys() have been previously provided, skip the resource provider. It interacts with the priority of the resource provider in an unexpected way. An implementer would think they're just providing the set of attribute keys for optimization, but in doing so end up accidentally having their attributes ignored when they were supposed have been higher priority.

[zeitlinger]

would think they're just providing the set of attribute keys for optimization, but in doing so end up accidentally having their attributes ignored when they were supposed have been higher priority.

that was not the idea - not providing supportedKeys should only result in some wasted CPU cycles.

[zeitlinger]

Thinking about it, an abstract superclass can probably achieve a similar level of disability for authors.

[zeitlinger]

Thinking about it, an abstract superclass can probably achieve a similar level of disability for authors.

I tried this approach out: open-telemetry/opentelemetry-java-instrumentation#10540

[zeitlinger]

Closing in favor of #6250