Add public API to access environment resource #5554
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5554 +/- ##
=========================================
Coverage 91.39% 91.40%
- Complexity 4963 4966 +3
=========================================
Files 550 551 +1
Lines 14569 14571 +2
Branches 1358 1358
=========================================
+ Hits 13316 13318 +2
Misses 866 866
Partials 387 387
☔ View full report in Codecov by Sentry. |
| import java.util.HashSet; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.function.BiFunction; | ||
|
|
||
| /** Auto-configuration for the OpenTelemetry {@link Resource}. */ | ||
| final class ResourceConfiguration { | ||
| public final class ResourceConfiguration { |
There was a problem hiding this comment.
Is your thinking that if we have any other resources that we want to auto-configured in this module, that we could add more methods? Otherwise, it might make sense to name this "EnvironmentResource"?
There was a problem hiding this comment.
My thinking is that this module should be reserved for interpreting the spec environment variables which do not have a natural home in a different module. For example, the zipkin, otlp, jaeger, env variables belong in those respective modules.
Users should be able to access components configured according to the environment with or without AutoConfiguredOpenTelemetrySdk. This means they can cherry pick their config - maybe they do programatic config everywhere except the environment resource (as you've done), or except span limits.
My first pass was to add an EnvironmentResource class, but on further consideration, that pattern doesn't scale when we get requests to access other components configured from the environment, like SpanLimits. It'd be more natural to add a public method TracerProviderConfiguration#createSpanLimits(ConfigProperties) than to add a dedicated EnvironmentSpanLimits class.
I don't anticipate any other types of resource configuration being done from environment variables, so I'd be surprised if we ever had to add more methods.
There was a problem hiding this comment.
🤔 If I, as an outside user, knew nothing of this discussion or how code was organized, would I be surprised that a class in the autoconfigure called "ResourceConfiguration" would be the place to go for a Resource instance configured from environment variables? I'm honestly not sure...just trying to think about the API surface we're giving to users before we are locked in.
Do you envision this class also being able to parse file-based configurations and return resources from it?
There was a problem hiding this comment.
I'm honestly not sure...just trying to think about the API surface we're giving to users before we are locked in.
Yeah I'm not super confident either. I don't feel particularly strong about this.
Do you envision this class also being able to parse file-based configurations and return resources from it?
No. File based configuration is going to be an all or nothing thing. You'll be able to reference environment variables in the file using special syntax, but if you choose to use file configuration, the environment variable based config scheme will be ignored. This suggests that the code interpreting a file configuration model would be split out into its own package, and have a really small API where a file or ConfigurationModel is passed in, and a full configured OpenTelemetrySdk is returned.
There was a problem hiding this comment.
cool. I don't know that I have a much better name at this point, so let's move forward with this and hopefully get some feedback from users!
Related to #5464.
Implementation of my preferred solution to provide public access to resource configured from the environment, as discussed here.
Adds new public method
ResourceConfiguration#createEnvironmentResource()to autoconfigure.