opentracing can not be disabled with runtime config.

[sean-scott-lr]

Describe the bug
Setting the property -Dquarkus.jaeger.enabled=false when executing the jar produced by running [my-package]-runner.jar, or via native-image, does not disable jaeger.

Expected behavior
jaeger is disabled

Actual behavior
The value contained with application.properties is used.

To Reproduce
create a quarkus project
add the opentracing extension
build the executable jar or native image
start with -Dquarkus.jaeger.enabled=false
observe that the tracer is created and proceeds with tracing

Environment (please complete the following information):

• Output of uname -a or ver: Darwin USMACEA013SSCOT 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64
• Output of java -version: openjdk version "11.0.3" 2019-04-16
  OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.3+7)
  OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.3+7, mixed mode, sharing)
• GraalVM version (if different from Java):
• Quarkus version or git rev: 1.0.0.CR1

[sean-scott-lr]

I am just getting started digging into how quarkus works... but it kind of appears that the check of the config item is done during "augmentation" vs at runtime.

c597314

[machi1990]

Yes, the enablement / disabling is done during build time. It should be noted too that with #5387 the property is moved from RUNTIME config phase to BUILD_AND_RUNTIME_FIXED phase.

/cc @loicmathieu @dmlloyd

[sean-scott-lr]

#3995

[sean-scott-lr]

Why wouldnt I just remove the dependency on opentracing if this is build-time configuration?

[dmlloyd]

I only moved it because the implementation was already treating it as a build time property. I'm definitely not opposed to having it be a run time configuration and working at run time, but the extension needs some fixing in this case.

[loicmathieu]

For what I understand, opentracing is boostraped at build time to allow build time optimisation.

But you can disabled jaeger at runtime, in this case a no-op tracer is used, if when using -Dquarkus.jaeger.enabled=false you still saw traces send to Jaeger, then it's a bug.

[sean-scott-lr]

Thanks @loicmathieu.... Yeah, the code for the check for enablement happens at build time, not run time. Which is why I feel this is a bug.

[loicmathieu]

This is a runtime config, used as a RUNTIME_INIT build step.
So this is runtime.

The build step will be run during the maven/gradle buid of your project, the output of the buildstep will be 'recorded' then executed at runtime, so when you start your JAR.

This is how Quarkus works, so for Quarkus this is runtime even if the check (seems to be) is done inside the deployment module.

According to me, there is no bug.
Can we close the issue ?

[dmlloyd]

This is a runtime config, used as a RUNTIME_INIT build step.
So this is runtime.

That is incorrect unfortunately. It is in fact used in a RUNTIME_INIT build step, but the step is executed at build time, and it currently exploits a bug in config processing that doesn't raise an error when the run time config is accessed at build time.

The build step will be run during the maven/gradle buid of your project, the output of the buildstep will be 'recorded' then executed at runtime, so when you start your JAR.

Only calls to the recorder object are recorded. All the other field references and method calls are not recorded and are executed immediately.

According to me, there is no bug.
Can we close the issue ?

No, as stated above.

[loicmathieu]

@dmlloyd if it exploit a bug this is scary as it works now and maybe it will not works later anymore ... and this construct can be seen in other extensions I think ...

[sean-scott-lr]

@loicmathieu I think you misunderstand the issue... this setting ONLY works in the quarkus:dev runtime... if you build a jar or run in native mode, you can not change the value of that config item and have quarkus honor the setting.

[dmlloyd]

Right it only appears to work, it doesn't actually work. If you change the flag at run time it has no effect. My PR #5387 simply formalizes the existing, actual behavior by marking this property as a build time property.

[loicmathieu]

@sean-scott-lr sorry, I misunderstand it as I use this config item ... in dev mode only ...

@dmlloyd for this to works it should be a config with ConfigPhase.BUILD_AND_RUN_TIME_FIXED right ? This is how we do for activating/disactivating the different security related extension. Or will it still only works on dev mode ?

[dmlloyd]

Setting it to BUILD_AND_RUN_TIME_FIXED avoids it becoming a build error after #5387 is merged but otherwise changes nothing: the property is only examined during build. To fix it to work at run time, the configuration object as a whole has to be passed to a recorder object which then does some run time decision based on the value.

[loicmathieu]

@dmlloyd I was afraid that you answer this ...

Regarding #5387 I'm not comfortable with changing RUN_TIME to BUILD_TIME config even if it fixes the bug as your fix changes the intent of the developer. I understand that you cannot make it works as expected easily, maybe you should open followup issue for the extension maintainer to have a look and, if possible, make it works as intended (so at runtime).

We also use this construct heavily inside the security extension to be able to switch implementation from JDBC/OAuth2/Properties at runtime .... we thought ... as it is mainly used to switch between dev mode and prod mode we didn't detect that it was an error :(. So there should be a lot more issues like this when we think that something is configurable at runtime but it is only configurable at build time and devmode/testmode ...

[dmlloyd]

@dmlloyd I was afraid that you answer this ...

Regarding #5387 I'm not comfortable with changing RUN_TIME to BUILD_TIME config even if it fixes the bug as your fix changes the intent of the developer. I understand that you cannot make it works as expected easily, maybe you should open followup issue for the extension maintainer to have a look and, if possible, make it works as intended (so at runtime).

Unfortunately it's going to start being an error to access run time config from a build step (unless it's just to proxy the object in to a recorder). If you do want to fix it before #5387 is merged, I understand.

We also use this construct heavily inside the security extension to be able to switch implementation from JDBC/OAuth2/Properties at runtime .... we thought ... as it is mainly used to switch between dev mode and prod mode we didn't detect that it was an error :(. So there should be a lot more issues like this when we think that something is configurable at runtime but it is only configurable at build time and devmode/testmode ...

We always should have had a strict check for this, however we also wanted to be able to send run time config objects in to recorders so they need to be proxied to do that.

I think we should instead support injecting run time config objects directly into recorders, and then completely disallow usage of run time config objects in build steps. In fact I'll file an issue to that effect.

[geoand]

I am going to close this as OpenTracing has been replaced by OpenTelemetry