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

Bump ratpack to 1.7 #4796

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Conversation

jsalinaspolo
Copy link
Contributor

Bump ratpack to 1.7 because will allow us to support instrumentation in Ratpack HttpClient with the interceptors

Reference

#4787

@laurit
Copy link
Contributor

laurit commented Dec 4, 2021

Also update supported libraries doc https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md

@iNikem
Copy link
Contributor

iNikem commented Dec 5, 2021

I am weakly against this change, as I see no reason in removing existing agent support for versions 1.4+. I admit that right now I don't know if any of Splunk customers use this integration or not.

@anuraaga
Copy link
Contributor

anuraaga commented Dec 5, 2021

We do need to target 1.7 for client library instrumentation, and we always need to provide library instrumentation where possible.

We could provide both versions for agent users - but I think juggling versions and muzzle is just too hard to expect from contributors. I would expect maintainers to step in to actively help with code if trying this.

For reference there are libraries where there is a tendency for users to be updating more frequently than others. I have an unverified expectation that ratpack users upgrade more consistently than tomcat users for example.

So my opinion is it's pragmatic to bump here without spending too much energy on the old versions here. But I am thinking we need a deprecation mechanism at least, if not a better defined deprecation policy with criteria of when we do it. My recommendation is to add logging in this PR that 1.7 will be the new base version (ideally only when the user uses old version if it's simple), call it out in the next release notes, and add the client library instrumentation in the next version. But it may not be the best idea open to more thoughts.

@mateuszrzeszutek
Copy link
Member

We do need to target 1.7 for client library instrumentation, and we always need to provide library instrumentation where possible.

Why don't we have separate ratpack-1.4:javaagent and ratpack-1.7:library modules then? That fixes the problem for library instrumentation and adheres to the doc introduced in #4802

@anuraaga
Copy link
Contributor

anuraaga commented Dec 6, 2021

@mateuszrzeszutek Do you mean not having 1.7 javaagent? If so it means I think, restoring javaagent only instrumentation for ratpack server which uses to be only context propagation + netty but got rewritten when migrating to server library instrumentation.

I guess it's ok but still pretty complicated in general. I've come to realize from this conversation that somehow having a clearer split between community and vendor would be beneficial here. We talk about pushing old or minor instrumentation to contrib but somehow this has started to feel like the opposite approach. Contrib is for contributions - leaving legacy stuff to contribution somehow seems weird doesn't it? For example, people pay Oracle to support Java 8u30 etc - expecting vendors to get paid to support old libraries also seems reasonable. I'm wondering why OTel contributors, many of which want to help out for free, should have to be absorbed into this world.

Perhaps we should look into creating a new repo, opentelemetry-javaagent which has all the javaagent components and we expect to basically be developed by vendors. Dealing with old libraries, including working around problems because of newer library instrumentation could be confined to this repo. The build could also become far simplified for the average contributor. -instrumentation could contain tier 1 library instrumentation and -contrib could contain contributor instrumentation, assuming that distinction is important, it may not be important to split them at that point though. This model seems to fit well with "native instrumentation" too I guess where library instrumentation is in a separate repo by default.

Food for thought, let's discuss more.

@trask
Copy link
Member

trask commented Dec 7, 2021

hi @jsalinaspolo! thanks for your patience. we discussed today and realized that the ratpack javaagent instrumentation doesn't depend on the ratpack library instrumentation, so think the best approach is to bump the ratpack library instrumentation to 1.7, and leave the ratpack javaagent instrumentation as is.

This is the resulting directory structure to target:

instrumentation/
  ratpack/
    ratpack-1.4/
      javaagent/
    ratpack-1.7/
      library/
    ratpack-testing/

If sharing ratpack-testing becomes problematic due to the different base versions (1.4 vs 1.7), feel free to make a copy for each, e.g.

instrumentation/
  ratpack/
    ratpack-1.4/
      javaagent/
      testing/
    ratpack-1.7/
      library/
      testing/

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Thanks!

@jsalinaspolo
Copy link
Contributor Author

Thanks for the quick outcome 🙇

@trask finally I've only moved instrumentation to ratpack-1.7 leaving testing inside ratpack-1.4 as the server tests are compatible

@trask trask merged commit ebe4c65 into open-telemetry:main Dec 7, 2021
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants