Skip to content

Add jersey server jakarta module#1458

Merged
codefromthecrypt merged 4 commits intoopenzipkin:masterfrom
dennyac:add-jersey-server-jakarta-module
Apr 24, 2025
Merged

Add jersey server jakarta module#1458
codefromthecrypt merged 4 commits intoopenzipkin:masterfrom
dennyac:add-jersey-server-jakarta-module

Conversation

@dennyac
Copy link
Copy Markdown
Contributor

@dennyac dennyac commented Apr 7, 2025

A copy of jersey-server module to work with jakarta namespace. There are references to javax classes like WebApplicationException in the jersey-server module

@dennyac
Copy link
Copy Markdown
Contributor Author

dennyac commented Apr 7, 2025

@codefromthecrypt - Would appreciate a review. Happy to accommodate any changes or alternative approaches too

Copy link
Copy Markdown
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

The approach follows others like JMS. Basically @reta and/or I need to second eye review nothing was missed in the find/replace adjustment

<parent>
<groupId>io.zipkin.brave</groupId>
<artifactId>brave-instrumentation-parent</artifactId>
<version>6.1.1-SNAPSHOT</version>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need to bump all poms to 6.2.0-SNAPSHOT and verify the since javadoc on public types are since 6.2

@reta
Copy link
Copy Markdown
Contributor

reta commented Apr 8, 2025

The approach follows others like JMS. Basically @reta and/or I need to second eye review nothing was missed in the find/replace adjustment

The change looks pretty straightforward to me (mirroring brave-instrumentation-jersey-server), thanks @dennyac

### Normal configuration (`TracingApplicationEventListener`)

The `TracingApplicationEventListener` requires an instance of
`HttpTracing` to operate. With that in mind, use [standard means](https://jersey.github.io/apidocs/2.26/jersey/org/glassfish/jersey/server/monitoring/ApplicationEventListener.html)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this link needs an update, too

Copy link
Copy Markdown
Contributor

@reta reta left a comment

Choose a reason for hiding this comment

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

@dennyac could you please address a few comments from @codefromthecrypt , LGTM otherwise, thank you!

@dennyac
Copy link
Copy Markdown
Contributor Author

dennyac commented Apr 23, 2025

@reta - Addressed the comments. Updated the README and bumped all poms. Let me know if any other changes are needed. And thanks for your review

@reta
Copy link
Copy Markdown
Contributor

reta commented Apr 23, 2025

@dennyac thank you! @codefromthecrypt LGTY? thanks!

@codefromthecrypt codefromthecrypt merged commit 5d3ead2 into openzipkin:master Apr 24, 2025
4 checks passed
@codefromthecrypt
Copy link
Copy Markdown
Member

Thanks again

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.

3 participants