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

Tweak naming of spring instrumentation modules #1285

Closed
snicoll opened this issue Sep 29, 2020 · 7 comments · Fixed by #6453
Closed

Tweak naming of spring instrumentation modules #1285

snicoll opened this issue Sep 29, 2020 · 7 comments · Fixed by #6453
Labels
enhancement New feature or request

Comments

@snicoll
Copy link

snicoll commented Sep 29, 2020

Please consider renaming the spring-boot-autoconfigure module, something that starts with opentelemetry looks more sensible to me.

There is also section in the documentation that describe naming for third party starters

@anuraaga anuraaga added bug Something isn't working priority:p1 labels Sep 30, 2020
@anuraaga
Copy link
Contributor

Thanks for checking in @snicoll - we publish artifacts always prefixed with opentelemetry-, as in here

https://oss.jfrog.org/libs-snapshot/io/opentelemetry/instrumentation/

Is this ok for following those conventions?

@snicoll
Copy link
Author

snicoll commented Sep 30, 2020

@anuraaga thanks for the quick feedback and sorry I overlooked the prefix in the build. I am still learning Gradle and this part is counter intuitive to my Maven background :) Perhaps if the directory name matched the published artifactId that would be more obvious?

To answer your question, an opentelemetry- prefix is exactly what I would except so that's perfect.

Looking at your link, I've noticed that opentelemetry-spring-starter could be named opentelemetry-spring-boot-starter for consistency. "spring-starter" is not really a term I've seen a lot out there.

As for the instrumentations on various components, have you considered merging these in a single artifact with optional dependencies? I'd not expect the instrumentation for opentelemetry-spring-web-3.1 to bring the spring-web artifact. I'd rather have an app using various Spring components and adding opentelemetry-spring-instrumentation (or something like that) would give me all the instrumentations that you support and that I could or couldn't use based on my claspath.

Another advantage on this approach is that if you add more instrumentations going forward, I could benefit from them without having to change my build.

@iNikem
Copy link
Contributor

iNikem commented Sep 30, 2020

I think have single artifact does make sense.

@iNikem
Copy link
Contributor

iNikem commented Sep 30, 2020

Just to be sure, @snicoll, as we publish current artifacts with opentelemetry- prefix already, does this issue still looks like P1 bug to you, or we are back to "enhancement"? :)

@snicoll
Copy link
Author

snicoll commented Sep 30, 2020

I didn't add the P1 or bug thing so I don't have an opinion about that. I confirm that having an opentelemtryprefix in the published artifact resolves my primary concern. We could also close this one and open individual issues for renaming the folder name and the single artifact if that's any easier.

Regarding the latter, I should also add that Spring Framework 3.x is EOLed since Dec 2016. I understand you want to support as many users as possible but having the version in the artifactId looks really odd IMO. A downside of the single artifact is that you'd have to compile all your instrumentations with a common version of the framework. Only supporting supported versions doesn't sound crazy to me though.

@anuraaga anuraaga changed the title Rename spring-boot-autoconfigure module Tweak naming of spring instrumentation modules Sep 30, 2020
@iNikem iNikem added enhancement New feature or request and removed bug Something isn't working priority:p1 labels Sep 30, 2020
@anuraaga
Copy link
Contributor

I marked it as P1 since we should make sure we get the artifact name correct for GA if it's just an easy rename - don't see any need to separate out issues, I just renamed this one. Definitely sounds like we should rename to spring-boot-starter at the least. Merging seems to make sense too, since users usually have the mental model of "spring 4", not so much individual components, at least from what I'm aware.

Spring is such a popular framework that I suspect we may still have users using old ones like 3.x. I guess we could consider having opentelemetry-spring-3.0, opentelemetry-spring-5.0 if we wanted to support both (target a spring framework version, not a component)

@trask
Copy link
Member

trask commented Aug 8, 2022

ping @mateuszrzeszutek since you have been thinking about spring instrumentation lately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants