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 shouldApply to ResourceProvider interface #4718

Conversation

breedx-splk
Copy link
Contributor

Opening this draft to start a conversation.

So, instrumentation agents may be built with many various ResourceProvider implementations. Some of these are optimistic implementations that will only return data (a non-empty Resource) when running under certain conditions. Examples of this include: running in a containerized environment, running in a specific cloud provider, or running in a certain type of web/servlet container.

To further complicate things, ResourceProviders can vary in quality/accuracy and might want to act as a "fallback strategy". For example, one ResourceProvider might populate the webengine.name attribute by reading a file from the classpath (fast) while another, more complicated implementation might attempt to find the same attribute by performing a sophisticated series of http requests. We generally wouldn't want to perform the expensive/slow strategy if the earlier/faster one is successful. In our specific case, we have some fallback strategies that attempt to populate the service.name attribute via a variety of strategies.

While the order() is useful to perform these strategies in a predetermined and consistent order, scenarios can exist that introduce conflicts in certain environments...and we only want the later ResourceProviders to return data iff a prior one has not yet returned certain data. The current API is insufficient -- there's no real means for a ResourceProvider to determine if an attribute has already been set.

So this adds a new method called shouldApply(config, resource) to the ResourceDetector interface. It defaults to returning true, so existing implementations are unchanged. The shouldApply method is checked and only if it returns true does createResource() get called.

I don't think this change is destabilizing or wide reaching -- it doesn't modify or remove any existing APIs/SPIs, but it does add one small SPI.

Definitely curious what other folks think or ideas they have about solving the broader problem. Thanks for looking!

@jack-berg
Copy link
Member

The fast / slow implementation case could be solved by having a single resource provider with internal logic to perform the fast implementation before the slow one. Is there a scenario where its impractical to package a fast / slow implementation into a single resource provider?

@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #4718 (3277bed) into main (3a80230) will decrease coverage by 0.00%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##               main    #4718      +/-   ##
============================================
- Coverage     90.14%   90.13%   -0.01%     
- Complexity     5107     5108       +1     
============================================
  Files           590      591       +1     
  Lines         15743    15745       +2     
  Branches       1511     1512       +1     
============================================
+ Hits          14191    14192       +1     
  Misses         1095     1095              
- Partials        457      458       +1     
Impacted Files Coverage Δ
...metry/sdk/autoconfigure/ResourceConfiguration.java 96.66% <50.00%> (-3.34%) ⬇️
...emetry/sdk/autoconfigure/spi/ResourceProvider.java 100.00% <100.00%> (ø)

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

@breedx-splk
Copy link
Contributor Author

The fast / slow implementation case could be solved by having a single resource provider with internal logic to perform the fast implementation before the slow one. Is there a scenario where its impractical to package a fast / slow implementation into a single resource provider?

I don't have a concrete example presently. It's also not exclusively motivated by speed/performance tho -- it also forces implementers to mix concerns to violate SRP.

The case we do have is a set of providers that are trying to guess the service.name based on some heuristics (all of which are fallbacks for the user not having set the appropriate env var or sysprop...but we want to make it simple and improve the out-of-the-box experience). For example, one provider might try to discover servlet/application context to guess a name, one might do spring application name detection, and one might fall back to war or jar filename. We could put all those eggs in one basket, but it doesn't feel very scalable that way.

By allowing a resource provider to check if a service.name has already been set by a prior component, the lower priority fallback checks can be skipped.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 6, 2022
@jack-berg
Copy link
Member

@breedx-splk closing this now that we've merged #4731. Thanks for the idea! Once we get a handful of use cases we can consider promoting it to the official SPI.

@jack-berg jack-berg closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants