RemoteControlledSampler fails with "No strategy present in response. Not updating sampler." in native mode

[ctron]

Describe the bug

When using Jaeger tracing by adding: io.quarkus:quarkus-jaeger, tracing seems to work, but a periodic error message is in the log, when using native mode.

I assume that updating the strategy is broken as well.

Expected behavior

Same as JVM mode: No error message. Update works.

Actual behavior

2020-07-01T11:31:05.711Z ERROR [RemoteControlledSampler] No strategy present in response. Not updating sampler.

To Reproduce
Steps to reproduce the behavior:

1. Enable tracing
2. Check the logs

Environment (please complete the following information):

• Output of uname -a or ver:
• Output of java -version:
• GraalVM version (if different from Java):
• Quarkus version or git rev:
• Build tool (ie. output of mvnw --version or gradlew --version):

[geoand]

cc @pavolloffay

[pavolloffay]

Could you please share what version of Jaeger backed are you using and its configuration?

[ctron]

<jaeger-client.version>1.1.0</jaeger-client.version>
<opentracing.version>0.33.0</opentracing.version>

[pavolloffay]

I mean Jaeger backend/server components.

[ctron]

[calohmn]

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 a reflection config has to be defined here.

I've gotten it to work with the following config:

  {
    "name" : "io.jaegertracing.internal.samplers.http.OperationSamplingParameters",
    "allDeclaredConstructors" : true,
    "allPublicConstructors" : true,
    "allDeclaredMethods" : true,
    "allPublicMethods" : true,
    "allDeclaredFields" : true,
    "allPublicFields" : true,
    "fields" : [
      { "name" : "defaultSamplingProbability", "allowWrite" : true },
      { "name" : "defaultLowerBoundTracesPerSecond", "allowWrite" : true },
      { "name" : "perOperationStrategies", "allowWrite" : true }
    ]
  },
  {
    "name" : "io.jaegertracing.internal.samplers.http.PerOperationSamplingParameters",
    "allDeclaredConstructors" : true,
    "allPublicConstructors" : true,
    "allDeclaredMethods" : true,
    "allPublicMethods" : true,
    "allDeclaredFields" : true,
    "allPublicFields" : true,
    "fields" : [
      { "name" : "operation", "allowWrite" : true },
      { "name" : "probabilisticSampling", "allowWrite" : true }
    ]
  },
  {
    "name" : "io.jaegertracing.internal.samplers.http.ProbabilisticSamplingStrategy",
    "allDeclaredConstructors" : true,
    "allPublicConstructors" : true,
    "allDeclaredMethods" : true,
    "allPublicMethods" : true,
    "allDeclaredFields" : true,
    "allPublicFields" : true,
    "fields" : [
      { "name" : "samplingRate", "allowWrite" : true }
    ]
  },
  {
    "name" : "io.jaegertracing.internal.samplers.http.RateLimitingSamplingStrategy",
    "allDeclaredConstructors" : true,
    "allPublicConstructors" : true,
    "allDeclaredMethods" : true,
    "allPublicMethods" : true,
    "allDeclaredFields" : true,
    "allPublicFields" : true,
    "fields" : [
      { "name" : "maxTracesPerSecond", "allowWrite" : true }
    ]
  },
  {
    "name" : "io.jaegertracing.internal.samplers.http.SamplingStrategyResponse",
    "allDeclaredConstructors" : true,
    "allPublicConstructors" : true,
    "allDeclaredMethods" : true,
    "allPublicMethods" : true,
    "allDeclaredFields" : true,
    "allPublicFields" : true,
    "fields" : [
      { "name" : "probabilisticSampling", "allowWrite" : true },
      { "name" : "rateLimitingSampling", "allowWrite" : true },
      { "name" : "operationSampling", "allowWrite" : true }
    ]
  }

The allowWrite configuration seems to be needed, since Lombok generates final fields in the above classes.
See the exception otherwise produced:

IllegalAccessException: Cannot set final field: io.jaegertracing.internal.samplers.http.ProbabilisticSamplingStrategy.samplingRate. Enable by specifying "allowWrite" for this field in the reflection configuration.

Having to define such a reflection config in the application using quarkus-jaeger of course doesn't look like a proper solution, so
I guess this should be solved by some other means inside quarkus-jaeger.

[calohmn]

Maybe it is caused by not all relevant classes being included here:

quarkus/extensions/jaeger/deployment/src/main/java/io/quarkus/jaeger/deployment/JaegerProcessor.java

Lines 57 to 61 in 27ba1cb

	public ReflectiveClassBuildItem reflectiveClasses() {
	return ReflectiveClassBuildItem
	.builder("io.jaegertracing.internal.samplers.http.SamplingStrategyResponse",
	"io.jaegertracing.internal.samplers.http.ProbabilisticSamplingStrategy")
	.finalFieldsWritable(true)

It seems to me that

"io.jaegertracing.internal.samplers.http.RateLimitingSamplingStrategy",
"io.jaegertracing.internal.samplers.http.PerOperationSamplingParameters",
"io.jaegertracing.internal.samplers.http.OperationSamplingParameters"

are missing here. I haven't yet checked whether adding these here resolves the issue, though.

@jmartisk Any comment on this (the above code was added in 05b5041) ?

[sophokles73]

@objectiser I would like to contribute a PR for adding the missing classes for reflection as pointed out by @calohmn
Which branch should I create the PR for? We are (still) using 2.6.3 so I am inclined to do it against the 2.6 branch. Does that make sense or is 2.6 already dead and we should focus on 2.7 instead?

[geoand]

All PRs go against the main branch.

[sophokles73]

How do you determine which releases the fix will go into. i.e. can I expect the fix to be part of the next 2.6.x or 2.7.x release?

[geoand]

We decide whether or not to backport to live branches based on the impact of the change. As you said, 2.6 is dead now that 2.7 is out, so the question is whether the fix would be backported to 2.7

[sophokles73]

Registering the missing Jaeger classes for reflection in the Jaeger extension (#23610) actually is not enough. It seems that access to the classes' fields also needs to be enabled explicitly.