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

Support for Jetty 11 #3234

Closed
dzikoysk opened this issue Jun 16, 2022 · 13 comments
Closed

Support for Jetty 11 #3234

dzikoysk opened this issue Jun 16, 2022 · 13 comments
Labels
enhancement A general enhancement module: jetty11
Milestone

Comments

@dzikoysk
Copy link

dzikoysk commented Jun 16, 2022

Please describe the feature request.
Provide implementation for Jetty 11 users. Due to several big changes, modules written for Jetty 9 are incompatible with Jetty 11 (mostly because of Jakarta EE 9) and it includes the current impl:

Rationale

We're migrating to Jetty 11 from Jetty 9 that is based on top of Jakarta EE 9.
Unfortunately micrometer currently supports only 9 and it kinda makes it impossible to operate on that module in our case.
Because we're developing an open-source library, not a private product, it will mostly affect Micrometer's users that would like to use it in further releases of Javalin framework.

Additional context
Should be probably provided as a new implementation as there are still Jetty 9 users that don't want to migrate.

@shakuzen shakuzen added the enhancement A general enhancement label Jun 17, 2022
@shakuzen shakuzen added this to the 1.10 backlog milestone Jun 17, 2022
@shakuzen
Copy link
Member

We were able to support two major versions with the same instrumentation when it came to Jersey server by removing references to the javax package classes from API signatures - #2766. It seems it may be tricky to do the same with Jetty. It's not a problem to have a separate class for instrumenting Jetty 11. A pull request would be welcome if someone wants to work on this.

@shakuzen shakuzen added the help wanted An issue that a contributor can help us with label Jun 17, 2022
@tipsy
Copy link

tipsy commented Jul 9, 2022

Jetty 9 is has reached end of life, so this is becoming important: https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.48.v20220622

@shakuzen
Copy link
Member

I'm not sure how we can support both Jetty 9 and Jetty 11 in the same module since the jetty-server artifact has not changed coordinates. We don't want to suddenly remove our instrumentation for Jetty 9. So we're in a bit of a tough position about how to proceed. We could make a micrometer-jetty11 module, but I would like to avoid that since eventually we would want to get rid of it.

@tipsy
Copy link

tipsy commented Jul 12, 2022

I'm not sure how we can support both Jetty 9 and Jetty 11 in the same module since the jetty-server artifact has not changed coordinates.

I'm surprised this isn't a problem that micrometer runs into all the time, most libraries break on major without changing coordinates?

We don't want to suddenly remove our instrumentation for Jetty 9. So we're in a bit of a tough position about how to proceed. We could make a micrometer-jetty11 module, but I would like to avoid that since eventually we would want to get rid of it.

A new module might not be too bad. Jetty 9 is probably going to be around for another decade. The last time I checked, Java8 is still bigger than Java11, and Jetty11 requires Java11.

@shakuzen
Copy link
Member

I'm surprised this isn't a problem that micrometer runs into all the time, most libraries break on major without changing coordinates?

There's a variety of factors that make this not a frequent issue. Instrumentation living in the respective projects instead of here in the Micrometer repo is the best way to avoid this issue and handle general evolution of the API used for instrumentation. If the Jetty project were willing to have Micrometer instrumentation in Jetty itself or a module maintained by the Jetty team, this problem could be avoided. I searched their GitHub issues but it looks like no one has asked for this before, perhaps because we have had instrumentation in Micrometer itself.
Another reason it may not be an issue with other libraries is that while there may be breaking changes in some API, the API we use for instrumentation is the only API we effectively need to care about. Yet another reason is, there is not nearly as much pressure to support 9 year old major versions of other libraries as there is for something like Jetty.

@tipsy
Copy link

tipsy commented Jul 13, 2022

If the Jetty project were willing to have Micrometer instrumentation in Jetty itself or a module maintained by the Jetty team, this problem could be avoided.

This sounds reasonable. Would you like me to create an issue over at their tracker, or do you prefer to do it yourself?

@shakuzen
Copy link
Member

I think it'd be better if the request came from you, since you maintain a framework that has integration with Jetty and Micrometer. Feel free to mention me in the issue, though. I am happy to answer any Micrometer questions if they come up.

@shakuzen shakuzen added waiting for feedback We need additional information before we can continue and removed help wanted An issue that a contributor can help us with labels Jul 13, 2022
@joakime
Copy link
Contributor

joakime commented Jul 29, 2022

I'm not sure how we can support both Jetty 9 and Jetty 11 in the same module since the jetty-server artifact has not changed coordinates. We don't want to suddenly remove our instrumentation for Jetty 9. So we're in a bit of a tough position about how to proceed. We could make a micrometer-jetty11 module, but I would like to avoid that since eventually we would want to get rid of it.

Other projects just maintain separate packages themselves.

Example: https://github.com/dropwizard/metrics

@tipsy
Copy link

tipsy commented Aug 9, 2022

@shakuzen based on the responses from the Jetty team, it seems like it's better to add the help wanted label back.

@shakuzen shakuzen added waiting for team An issue we need members of the team to review and removed waiting for feedback We need additional information before we can continue labels Aug 10, 2022
@shakuzen shakuzen added help wanted An issue that a contributor can help us with and removed waiting for team An issue we need members of the team to review labels Sep 1, 2022
@shakuzen
Copy link
Member

shakuzen commented Sep 1, 2022

Thank you for raising the issue with the Jetty team, @tipsy. If the Jetty team does not want a module there, we will make one here. I still think it would be better for users if the module were versioned with other jetty modules, so there is less chance for mismatched versions, and that way, there doesn't need to be different coordinates for every new incompatible version of Jetty we try to support.

@shakuzen shakuzen removed the help wanted An issue that a contributor can help us with label Sep 2, 2022
@shakuzen shakuzen modified the milestones: 1.10 backlog, 1.10.0-RC1 Oct 7, 2022
@shakuzen
Copy link
Member

shakuzen commented Oct 7, 2022

I have merged the change introducing a new micrometer-jetty11 module. This will be part of the upcoming 1.10.0-RC1 release candidate. Let us know if there is any feedback. It is really just a copy of the existing instrumentation adapted to Jetty 11 changes.

@zugazagoitia
Copy link

Working fine here, thanks! Do you have a date for the 1.10.0 release?

@jonatan-ivanov
Copy link
Member

@zugazagoitia RC1 is tomorrow, GA is mid November.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement module: jetty11
Projects
None yet
Development

No branches or pull requests

6 participants