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

Add tracing for microprofile-rest-client #82

Closed
pavolloffay opened this issue Aug 21, 2018 · 11 comments
Closed

Add tracing for microprofile-rest-client #82

pavolloffay opened this issue Aug 21, 2018 · 11 comments
Milestone

Comments

@pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Aug 21, 2018

Add tracing for https://github.com/eclipse/microprofile-rest-client. Tracing metadata like tags/logs should be the same as for JAX-RS client.

We need to figure out how this cross-spec specification should be handled. Should it be added here or in microprofile-rest-client? I would vote to add it here.

cc @pilhuhn @kwsutter

A pointer to rest-client issue allowing tracing integration eclipse/microprofile-rest-client#35 (comment).

@kenfinnigan
Copy link
Contributor

@kenfinnigan kenfinnigan commented Aug 21, 2018

There has been some discussion in the past around spec integration and the need for a MP level spec to cover those interactions.

Doing so would also provide for a MP level TCK to verify such behavior

@pavolloffay
Copy link
Member Author

@pavolloffay pavolloffay commented Aug 21, 2018

Is there any timeline to add MP level spec?

My main concern about adding microprofile-rest-client here is that this spec shouldn't probably directly depend on the microprofile-rest-client. What if a vendor does not support mp-rest-client but it wants to use tracing only for jax-rs?

@kenfinnigan
Copy link
Contributor

@kenfinnigan kenfinnigan commented Aug 21, 2018

I don't think there is, but we can raise it again today

@andymc12
Copy link

@andymc12 andymc12 commented Aug 21, 2018

@pavolloffay It should be possible to add Open Tracing providers to the MP Rest Client with only a compile-time dependency on microprofile-rest-client. You would need the compile-time dependency in order to build the RestClientBuilderListener implementation class (which could then register the Open Tracing provider(s)), but that class would only be loaded by the MP Rest Client implementation at runtime - so there is no need for a runtime dependency.

@rafabene
Copy link

@rafabene rafabene commented Feb 6, 2019

Thorntail 2.3.0.Final documentation mentions this issue:

Currently, MicroProfile OpenTracing does not integrate with MicroProfile RestClient, so you need to use a pure JAX-RS Client. This limitation will be removed in the future, see MicroProfile OpenTracing issue #82.

Is there any opened issue on Thorntail for MP-Restclient + MP-Opentracing? It still not working on Thorntail 2.3.0.Final.

The test code is in this repo: https://github.com/redhat-developer-demos/microprofile-demo

@pavolloffay
Copy link
Member Author

@pavolloffay pavolloffay commented Feb 6, 2019

Hi @rafabene, the OT spec supports tracing for MP rest client 1.2. Here is a PR for smallrye smallrye/smallrye-opentracing#28 - at the moment is blocked by not completed rest client implementation. Once smallrye impls are in place it will be used in throntail @kenfinnigan probably knows when that should be.

@wutingbupt
Copy link

@wutingbupt wutingbupt commented Apr 15, 2019

Hi @pavolloffay, I can see the PR is merged, does there is anything new about this request?

@pavolloffay
Copy link
Member Author

@pavolloffay pavolloffay commented Apr 15, 2019

@wutingbupt the support for MP rest client is supported since 1.2. What else is missing for you?

@wutingbupt
Copy link

@wutingbupt wutingbupt commented Apr 15, 2019

@pavolloffay I have tried this morning with the open tracing 1.3.

But I got something like this:

{"stackTrace":"java.lang.NoClassDefFoundError: Failed to link io/smallrye/opentracing/SmallRyeRestClientListener (Module \"io.smallrye.opentracing\" from BootModuleLoader@6bb4dd34 for finders [JDK Module Finder, BootstrapClasspathModuleFinder, BootstrapModuleFinder(org.wildfly.swarm.bootstrap), ClasspathModuleFinder, ContainerModuleFinder(swarm.container), ApplicationModuleFinder(thorntail.application), org.wildfly.swarm.bootstrap.modules.DynamicModuleFinder@7d9f158f])
I can see the dependency is well added, so I have no idea why it will complain about this .

Br,
Tim

@pavolloffay
Copy link
Member Author

@pavolloffay pavolloffay commented Apr 15, 2019

@wutingbupt please report this issue in the implementation. This is not related to the specification. So please report this in Smallrye or thorntail depending how you are using it. A dependency tree would be helpful to investigate.

@wutingbupt
Copy link

@wutingbupt wutingbupt commented Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants