add resource detectors to spring starter #10277
Conversation
| @@ -12,6 +12,7 @@ dependencies { | |||
| api("org.springframework.boot:spring-boot-starter:$springBootVersion") | |||
| api("org.springframework.boot:spring-boot-starter-aop:$springBootVersion") | |||
| api(project(":instrumentation:spring:spring-boot-autoconfigure")) | |||
| implementation(project(":instrumentation:resources:library")) | |||
There was a problem hiding this comment.
why did you choose to add this as implementation when all the other dependencies are api?
secondly these resource providers use the SPI that the sdk autoconfigure module uses, but spring boot starter does not uses the sdk autoconfigure. Even if you add them do they actually do anything?
There was a problem hiding this comment.
I would see more the new dependency included in spring:spring-boot-autoconfigure.
There was a problem hiding this comment.
- implementation is more conservative (doesn't allow access to transitive dependencies), so I use that by default
- .. does not use autoconfigure ... -> I've been wondering the same - is there there a good reason for that?
- will it work? Yes, there's a guard that checks if the class exists
There was a problem hiding this comment.
@zeitlinger Could you develop one feature for OTel Spring autoconfiguration with this dependency and with one test?
There was a problem hiding this comment.
I would see more the new dependency included in
spring:spring-boot-autoconfigure.
autoconfigure modules should be as thin as possible, and starters add what's usually needed.
There was a problem hiding this comment.
autoconfigure modules should be as thin as possible, and starters add what's usually needed.
It's not the case. The Spring autoconfigure module contains all the Java code used by the OpenTelemetry starter and the Zipkin starter.
There was a problem hiding this comment.
The resource library is compileOnly in Spring autoconfiguration. Do we want to include it by default?
There was a problem hiding this comment.
will it work? Yes, there's a guard that checks if the class exists
Thanks.
autoconfigure modules should be as thin as possible, and starters add what's usually needed.
I don't have a strong opinion for that. Might as well solve it by documenting that adding the resources library dependency can be useful. Perhaps this should be discussed at the SIG meeting.
|
It would be great to document this on https://opentelemetry.io/ after the next release. |
|
@laurit We could merge this one as discussed during the last SIG meeting? I could follow up with a PR proposing to move some of the OTel starter dependencies to the autoconfiguration library. |
|
@jeanbisutti I didn't participate in the last SIG meeting, feel free to proceed according to what you agreed at the SIG meeting. If we are going to include the resource detectors I think we should also consider including https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/spring/spring-boot-resources/library |
@laurit It seems I can't merge |
|
@jeanbisutti you should be able to merge this, if you can't merge other approved PRs then we need to figure our why it is so |
|
@laurit this is intentional (though we didn't have this repo set up correctly way back when you were an approver):
|
@laurit Same thing for #10350, for which you have approved and I have not |
I also can't merge approved PRs |
I'll create a follow-up PR for this. Update: The user can't even enable the resource detector by adding the library manually, because the resource detectors not loaded generically (I'm not sure why actually). |
it's a lightweight dependency and very useful to get started