Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new ResourceDetector #6250

Closed
wants to merge 5 commits into from

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 26, 2024

Based on

  • a discussion about having resource providers that can be disabled by default
  • and common pitfalls when implementing resource providers

This PR proposes an alternative interface for ResourceProvider, called ResourceDetector.

Separating both aspects into different PRs would be possible, but would probably make the end result harder to understand.

Note: maybe ResourceProvider should be deprecated - but that's a separate question.

Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 96.96970% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 91.07%. Comparing base (1d4a2f4) to head (4d2a4ad).

Files Patch % Lines
...autoconfigure/internal/ResourceDetectorReader.java 93.93% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6250      +/-   ##
============================================
+ Coverage     91.06%   91.07%   +0.01%     
- Complexity     5695     5717      +22     
============================================
  Files           621      623       +2     
  Lines         16667    16723      +56     
  Branches       1707     1713       +6     
============================================
+ Hits          15177    15230      +53     
  Misses          997      997              
- Partials        493      496       +3     

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

@zeitlinger zeitlinger marked this pull request as ready for review February 28, 2024 07:47
@zeitlinger zeitlinger requested a review from a team February 28, 2024 07:47
@trask
Copy link
Member

trask commented Feb 29, 2024

can you add a description to the PR? it's not obvious to me what this PR is about, thanks

@zeitlinger
Copy link
Member Author

can you add a description to the PR? it's not obvious to me what this PR is about, thanks

done

@jack-berg
Copy link
Member

name and defaultEnabled implement the "disabled by default" feature in the same pattern as for instrumentations, as suggested open-telemetry/opentelemetry-java-contrib#1074 (comment)

Sorry I think there's been some miscommunication. The intent of my proposal was to make this an otel java agent specific feature. I don't think the idea of having resource providers enabled by default is a problem outside of the otel java agent. If you're not using the otel java agent, then you opt into each of the providers you want by adding a dependency to that. Its only in the otel java agent that you have the potential for a wide variety of resource providers to be bundled in automatically, some of which you're not interested in.

import java.util.Optional;
import java.util.function.Function;

public interface ResourceDetector<D> extends Ordered {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't see the current ResourceProvider design as broken, and can't tell how this would alleviate confusion.

Here's a summary of the toolkit for managing resources today:

  • ResourceProvider SPI allows you provide your own resource, which is merged with resource provided by other providers
  • The environment variables OTEL_SERVICE_NAME and OTEL_RESOURCE_ATTRIBUTES contribute to the environment resource provider, which runs last. These environment variables can be customized by AutoConfigurationCustomizer#addPropertiesSupplier, AutoConfigurationCustomizer#addPropertiesCustomizer.
  • ResourceProvider can optionally be ordered, so you can indicate priority relative to others
  • ResourceProvider can optionally be conditional, so you can decide whether or not to provide a resource based on the resource computed up till that point in time
  • All resource providers are enabled by default, and you can set a deny list with otel.java.disabled.resource.providers
  • All resource providers are enabled by default, but you can optionally change the semantic to an allow list by setting otel.java.enabled.resource.providers
  • After all resource providers have been applied, you can optionally filter out individual attribute keys with otel.experimental.resource.disabled.keys
  • After all resource providers have been applied and optional filter applied, you can customize the resource further with AutoConfigurationCustomizer#addResourceCustomizer
  • After the resource has been completely resolved and applied to Sdk{Signal}ProviderBuilder, you can further customize it with AutoConfigurationCustomizer#add{Signal}ProviderCustomizer via Sdk{Signal}ProviderBuilder's setResource and addResource methods.

That's quite a long and comprehensive list, with interactions which are hard to reason through for a casual user. I'm very reluctant to add additional complexity unless there is a very strong reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I've learned, there's one idiomatic way how to write resource providers, which is easy to get wrong.
Do the idea is to take complexity away, not to add it.
At the same time, this is not a topic that is blocking any of my current tasks.

@zeitlinger
Copy link
Member Author

If you're not using the otel java agent, then you opt into each of the providers you want by adding a dependency to that. Its only in the otel java agent that you have the potential for a wide variety of resource providers to be bundled in automatically, some of which you're not interested in.

  • In the spring starter, I'd also like to bundle all resource providers. It is possible to add those as dependencies, but it's more work for the user
  • I'll try to come up with a design that works in javaagent - and which can be re-used in the spring starter

@zeitlinger
Copy link
Member Author

@zeitlinger zeitlinger closed this Mar 5, 2024
@zeitlinger zeitlinger deleted the resource-detector branch March 5, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants