-
Notifications
You must be signed in to change notification settings - Fork 979
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
Comments
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. |
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 |
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. |
I'm surprised this isn't a problem that micrometer runs into all the time, most libraries break on major without changing coordinates?
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. |
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. |
This sounds reasonable. Would you like me to create an issue over at their tracker, or do you prefer to do it yourself? |
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. |
Other projects just maintain separate packages themselves. |
@shakuzen based on the responses from the Jetty team, it seems like it's better to add the |
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. |
I have merged the change introducing a new |
Working fine here, thanks! Do you have a date for the 1.10.0 release? |
@zugazagoitia RC1 is tomorrow, GA is mid November. |
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.
The text was updated successfully, but these errors were encountered: