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

fix: add JAXRS API as SR opentracing depends on it but didn't declare it #14323

Merged

Conversation

loicmathieu
Copy link
Contributor

@ghost
Copy link

ghost commented Jan 15, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)
  • title should not start with chore/feat/fix/docs but be a proper sentence

This message is automatically generated by a bot.

@Ladicek
Copy link
Contributor

Ladicek commented Jan 15, 2021

This looks good to me, but I'd love to get another pair of eyes. @radcortez or @kenfinnigan, could you suggest one more reviewer for this? Thanks!

@kenfinnigan
Copy link
Member

@loicmathieu can you provide some details about why you were getting a CNFE on @Path when your application wasn't using JAX-RS?

I'm wondering if it's simply a matter of using text class name instead of actual class or something like that?

@loicmathieu
Copy link
Contributor Author

This dependency is directly used by opentracing-jaxrs (that is a dependency of smallrye-opentracing) and I have a CNFE when using the @Traced annotation on a method.

I cannot extract the stacktrace as the exception was swallowed by the SR reactive messaging processor but if you look at the OpenTracingInterceptor class you can see that at line 35 is call a method skipJaxRs that uses the javax.ws.rs.Path type.

Maybe we can open an issue upsteam about this, but currently JAX-RS is needed in the classpath to use @Traced annotation.

@kenfinnigan
Copy link
Member

Thanks for the additional information.

Agree it's worth raising an issue in the upstream project to resolve in a way that doesn't require JAX-RS on the classpath, as it's an issue there.

But I appreciate right now it's currently broken for Quarkus with this use case.

Could you raise an issue upstream and add a comment to the dependency with the issue? We can then track the dependency removal when it's fixed

@loicmathieu
Copy link
Contributor Author

I openned opentracing-contrib/java-interceptors#15, I'll update the PR to add a reference to this issue inside the pom.xml

@loicmathieu loicmathieu added this to the 1.12 - master milestone Jan 15, 2021
@kenfinnigan
Copy link
Member

Thanks @loicmathieu!

@Ladicek
Copy link
Contributor

Ladicek commented Jan 15, 2021

Alternatively/additionally, we could/should fix this in SmallRye. Not sure how big of a deal that is, considering other SmallRye OpenTracing consumers.

@kenfinnigan
Copy link
Member

@Ladicek from the brief look I had SmallRye OpenTracing has a dependency on the contrib project to get the ability to process @Traced methods.

Not sure why, but @Traced handling isn't implemented directly.

@radcortez
Copy link
Member

Alternatively/additionally, we could/should fix this in SmallRye. Not sure how big of a deal that is, considering other SmallRye OpenTracing consumers.

I was thinking the same. Also, due to OpenTracing status, most likely these upstream projects are not getting much attention.

@loicmathieu loicmathieu merged commit d7c2115 into quarkusio:master Jan 18, 2021
@loicmathieu loicmathieu deleted the fix/opentracing-miss-jaxrs branch January 18, 2021 13:16
@gsmet gsmet modified the milestones: 1.12 - master, 1.11.1.Final Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants