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 jaeger RemoteControlledSampler not working in quarkus native images #2674

Merged
merged 1 commit into from
May 26, 2021

Conversation

calohmn
Copy link
Contributor

@calohmn calohmn commented May 21, 2021

Jaeger tracing currently doesn't work when using quarkus-native images and a jaeger sampling configuration that uses the sampling strategies defined in the Jaeger Collector component (i.e. using the RemoteControlledSampler).

This seems caused by the reflection used in conjunction with the io.jaegertracing.internal.samplers.HttpSamplingManager class.
There, a SamplingStrategyResponse object is deserialized from the JSON response using gson, which makes use of reflection to do so. That means that, at least with the current quarkus-jaeger code, a reflection config has to be defined in places where the JaegerTracer is used (see also quarkusio/quarkus#10402 (comment)).

This PR adds that.

Hope would be that quarkusio/quarkus#10402 gets solved in a way that doesn't make such a configuration necessary, so that this reflection config can be removed again with a quarkus update.

Relates to:
eclipse/packages#245


I've triggered a test build here also without the jaeger maven profile. That causes some warnings (e.g. WARNING: Could not resolve io.jaegertracing.internal.samplers.http.OperationSamplingParameters for reflection configuration.), but the build succeeds.

@calohmn calohmn added Tracing Quarkus issues with the (experimental) Quarkus based components labels May 21, 2021
@calohmn calohmn added this to the 1.9.0 milestone May 21, 2021
@calohmn calohmn requested a review from sophokles73 May 21, 2021 10:13
@calohmn calohmn requested a review from dejanb as a code owner May 21, 2021 10:13
@calohmn calohmn force-pushed the PR/jaeger_reflection_config branch from 49e6824 to fe903ce Compare May 26, 2021 05:10
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

@sophokles73
Copy link
Contributor

@calohmn how about putting this into a 1.8.1 as well?

@calohmn calohmn merged commit c0fb234 into eclipse-hono:master May 26, 2021
@calohmn calohmn deleted the PR/jaeger_reflection_config branch May 26, 2021 07:00
@calohmn
Copy link
Contributor Author

calohmn commented May 26, 2021

@sophokles73 Sure, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Quarkus issues with the (experimental) Quarkus based components Tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants