Add ResourceProvider SPI to allow for custom Resources #1548
Conversation
| @@ -74,6 +75,14 @@ private static String readVersion() { | |||
| return properties.getProperty("sdk.version"); | |||
| } | |||
|
|
|||
| private static Resource readResourceFromProviders() { | |||
| Resource result = Resource.EMPTY; | |||
| for (ResourceProvider resourceProvider : ServiceLoader.load(ResourceProvider.class)) { | |||
There was a problem hiding this comment.
We should probably add a property with a CSV list of implementation class name to exclude to allow users to disable resources they don't want to include automatically.
There was a problem hiding this comment.
Cannot the same be achieved via controlling application classpath?
There was a problem hiding this comment.
If every resource provider is in a separate artifact I think so but will that be the case? Not sure - but I sort of figured that SDK will have one extension artifact with a few providers so disabling one of them only by classpath manipulation could be tough.
There was a problem hiding this comment.
The only use-case for this that I currently know of is for auto-instrumentation to provide telemetry.auto.version. Which, btw, I wouldn't want end-users to disable :)
There was a problem hiding this comment.
There are many resources we haven't implemented in Java yet, like docker container ID or process. I figure they should also be implemented with this SPI
open-telemetry/opentelemetry-specification#811 (comment)
We'll probably have a lot of these providers for full data coverage but for startup time or possible privacy reasons presumably they should be disablable and classpath probably won't be enough. In agent we would probably bundle everything and need a property anyways I think?
There was a problem hiding this comment.
Ok, fair enough.
- Should I do this in this very same PR?
- Should new config property go to
TraceConfig?
There was a problem hiding this comment.
Think follow-up PR is fine.
Probably not TraceConfig - we'd have to stop using the static instance, and I guess this doesn't need to be programmatic (can just programmatically set Resource instead) so it's only for control from command line.
sdk/src/main/java/io/opentelemetry/sdk/resources/ResourceProvider.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1548 +/- ##
============================================
+ Coverage 91.45% 91.47% +0.01%
- Complexity 960 964 +4
============================================
Files 116 117 +1
Lines 3440 3448 +8
Branches 281 282 +1
============================================
+ Hits 3146 3154 +8
Misses 205 205
Partials 89 89
Continue to review full report at Codecov.
|
| @@ -74,6 +75,14 @@ private static String readVersion() { | |||
| return properties.getProperty("sdk.version"); | |||
| } | |||
|
|
|||
| private static Resource readResourceFromProviders() { | |||
| Resource result = Resource.EMPTY; | |||
| for (ResourceProvider resourceProvider : ServiceLoader.load(ResourceProvider.class)) { | |||
There was a problem hiding this comment.
Think follow-up PR is fine.
Probably not TraceConfig - we'd have to stop using the static instance, and I guess this doesn't need to be programmatic (can just programmatically set Resource instead) so it's only for control from command line.
|
I will be happy if this gets merged today :) Then I can finish implementing this in instrumentation repo (European) tomorrow. |
Partially addresses #1454