-
Notifications
You must be signed in to change notification settings - Fork 285
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
Add instrumentation for jakarta3 #3210
Conversation
Hi @pdenis1, and thanks for the contribution. There seems to be some files missing since the build fails to find the |
|
Thanks for your comments. I have carved out some time this week to revisit my MR and I will rebase it again on master.
Can you provide me with some documentation about the test framework you are using so I can look at implementing them?
Thanks!
Phil
… On Nov 29, 2021, at 12:04 PM, Brian Devins-Suresh ***@***.***> wrote:
RouteHandlerDecorator missing means that this needs to be rebased and updated to match the changes in the servlet instrumentation. RouteHandlerDecorator was removed recently
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#3210 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJK5CZCJJIPW7MU3CG4IHTUOPFE5ANCNFSM5IHT3IIQ>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
bc1068e
to
a17b985
Compare
I added test coverage, ran the muzzle and test targets locally and everything is passing. I pushed those changes and the circleci build failed due to jakarta.ws.rs being compiled for java 8+, so I set the minJavaVersionForTests to VERSION_1_8. I'm not sure why the above 2 tests are failing though? Any ideas? |
@pdenis1 It's nothing to worry about. Unfortunately our test suite has a bit of flakiness to it (yes we are working on removing that), so I restarted the failed tests and now they are green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let someone else take a look but this looks good! Thanks!
PS the failing tests passed after the build was rerun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thank you for the contribution @pdenis1
@pdenis1 would you mind squashing all the commits into one? I don't trust the |
de390ef
to
334dee7
Compare
@bantonsson squashed and pushed! |
Due to trademark disputes between Eclipse and Oracle over the use of the javax namespace, the major webservers have had to migrate from javax.ws.rs to the jakarta.ws.rs namespace.
https://eclipse-foundation.blog/2019/05/03/jakarta-ee-java-trademarks/
https://dzone.com/articles/from-javax-to-jakarta-a-simple-proof-of-concept
When we upgraded to Jetty 11 (and thus also to jakarta3), we lost a lot of information and granularity in our DD traces. This PR adds support for jakarta.ws.rs version 3.0+