Add zipkin compatibility mode to open-tracing #18313

raulvaldoleiros · 2021-07-01T17:12:26Z

Issue: #18278

quarkus-bot · 2021-07-01T17:12:27Z

Thanks for your pull request!

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

title should not contain an issue number (use 'Fix Allow * expression for quarkus:add-extension #1234' in the description instead)

This message is automatically generated by a bot.

kenfinnigan

Have you verified this extension works with native image build?

One concern I have is adding this into the Jaeger extension makes it difficult for dead code elimination to remove the Zipkin classes when Zipkin isn't used. If there was a way to have the Zipkin piece separate to QuarkusJaegerTracer it would be preferable, otherwise, it might need to be a whole separate extension.

What do you think @pavolloffay?

extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/JaegerConfig.java

raulvaldoleiros · 2021-07-01T19:28:57Z

Have you verified this extension works with native image build?

One concern I have is adding this into the Jaeger extension makes it difficult for dead code elimination to remove the Zipkin classes when Zipkin isn't used. If there was a way to have the Zipkin piece separate to QuarkusJaegerTracer it would be preferable, otherwise, it might need to be a whole separate extension.

What do you think @pavolloffay?

I added an integration test, it's just a simples rest with the opentracing and I executed this in native and it worked. The integration test combined with the unit test proves it works, from my analysis.

About the zipkin classes, if you want I can create a class ZipkinReporterFactory in a specific package and delegate the Report creation in that object. What do you think?

extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/JaegerConfig.java

integration-tests/pom.xml

extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/QuarkusJaegerTracer.java

kenfinnigan · 2021-07-06T15:47:32Z

extensions/jaeger/runtime/pom.xml

@@ -32,6 +32,20 @@
                </exclusion>
            </exclusions>
        </dependency>
+        <dependency>
+            <groupId>io.jaegertracing</groupId>


Can you measure the amount of additional RSS is present by introducing this dependency when running a native executable?

I'm concerned this approach to supporting Zipkin will add reasonable RSS weight to an application even when they're not using Zipkin compatibility

I made what you requested and the results are:
without my changes:

executable size: 43M

RSS on startup: 18908

RSS after first request: 22144

With my changes:

executable size: 44M

RSS on startup: 18960

RSS after first request: 24796

Thanks @raulvaldoleiros!

@n1hility, @ebullient what do you think of these numbers? Sufficiently small? I'm inclined to say yes, as adding tracing adds weight anyway, but would appreciate your thoughts

I would have add the dependency as optional, explain in the guide that to have zipkin capability you need to add the dependency to your pom.xml, and make a check at runtime that the class is present before enabling Zipkin.

OK, it's a small RSS increase, but still, if we have more and more of those addition we will ends up with a much bigger RSS.

I had thought about suggesting that, but wasn't sure if it could be done without causing an issue when it wasn't there.

If it can be done, that would be better.

@raulvaldoleiros can you try?

I tried to change the dependency scope to test or provided and it works fine in the quarkus build, but when I'm compiling an application it breaks without the dependency.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 24.495 s
[INFO] Finished at: 2021-07-09T18:24:08+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project opentracing-quickstart: Failed to build quarkus application: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[ERROR] [error]: Build step io.quarkus.jaeger.deployment.JaegerProcessor#setupTracer threw an exception: java.lang.NoClassDefFoundError: zipkin2/reporter/Sender

To make the dependency optional the only way is to use java reflection or do you have an example that I can follow?

To make the dependency optional the only way is to use java reflection or do you have an example that I can follow?

Yes, you need to guard the usage of the Zipkin classes by a check that the class exist (Class.forName("") and if an exception occurs, do nothing).

The only other solution is to create a separate extension but it can be a lot of work so maybe it's not worth it.

I changed the code for zipkin be optional, and also I have updated the documentation. Native mode needs extra configuration but it works, I tried to put that configuration in the jaeger extension but I wasn't successful.

Is extra configuration for native mode still necessary? If so, what configuration?

pavolloffay · 2021-07-07T15:58:33Z

@raulvaldoleiros why do you need this feature? To send data directly to Zipkin?

I am not maintaining Quarkus so it's hard to justify whether it is really needed. A workaround is to report data to OpenTelemetry collector with configured Jaeger receiver and Zipkin exporter.

raulvaldoleiros · 2021-07-07T16:57:06Z

@raulvaldoleiros why do you need this feature? To send data directly to Zipkin?

I am not maintaining Quarkus so it's hard to justify whether it is really needed. A workaround is to report data to OpenTelemetry collector with configured Jaeger receiver and Zipkin exporter.

I need this feature because the solution we use is only zipkin compatible and not jaeger compatible.

raulvaldoleiros · 2021-07-07T17:40:16Z

I didn't understand why the integration test failed. What makes sense to me is because I am validating some local endpoint data in the span, I removed it hopping that is the solution.
@kenfinnigan can you, please, approve the workflow again? Sorry to be this way :(

quarkus-bot · 2021-07-07T17:52:09Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `17d9873`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	JVM Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
✖	JVM Tests - JDK 16	`Build`	Test failures	Logs	Raw logs
✖	Native Tests - Misc3	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/smallrye-opentracing

✖ io.quarkus.it.rest.client.ZipkinIntegrationTest.testZipkinIntegration line 43 - More details - Source on GitHub

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/smallrye-opentracing

✖ io.quarkus.it.rest.client.ZipkinIntegrationTest.testZipkinIntegration line 43 - More details - Source on GitHub

⚙️ Native Tests - Misc3 #

📦 integration-tests/smallrye-opentracing

✖ io.quarkus.it.rest.client.ZipkinIntegrationTestIT.testZipkinIntegration - More details - Source on GitHub

ebullient · 2021-07-12T13:57:58Z

Sorry for the back and forth in this PR. Regarding handling optionally/maybe present dependencies, you should be able to tuck all/most of this in a processor, which itself is guarded by zipkin compatibility classes. An example is here:
https://github.com/quarkusio/quarkus/blob/main/extensions/micrometer/deployment/src/main/java/io/quarkus/micrometer/deployment/export/PrometheusRegistryProcessor.java

The PrometheusRegistryProcessor is packaged with the core micrometer extension (for a lot of reasons), but you'll notice that the BuildStep methods are guarded... they are only invoked if the required prometheus MeterRegistry class is present (which prevents construction of parameters, etc.) .. which means recorder methods that would reference that class are also only called if the required MeterRegistry class is present (they are further tucked away in a separate recorder that isn't touched if the Prometheus MeterRegistry isn't found.. )

raulvaldoleiros · 2021-07-13T13:47:13Z

Sorry for the back and forth in this PR. Regarding handling optionally/maybe present dependencies, you should be able to tuck all/most of this in a processor, which itself is guarded by zipkin compatibility classes. An example is here:
https://github.com/quarkusio/quarkus/blob/main/extensions/micrometer/deployment/src/main/java/io/quarkus/micrometer/deployment/export/PrometheusRegistryProcessor.java

The PrometheusRegistryProcessor is packaged with the core micrometer extension (for a lot of reasons), but you'll notice that the BuildStep methods are guarded... they are only invoked if the required prometheus MeterRegistry class is present (which prevents construction of parameters, etc.) .. which means recorder methods that would reference that class are also only called if the required MeterRegistry class is present (they are further tucked away in a separate recorder that isn't touched if the Prometheus MeterRegistry isn't found.. )

Hi @ebullient, thanks for pointing me in the right direction. I made the changes as you requested, I hope it's in the right way.

quarkus-bot · 2021-07-13T15:07:52Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `0a570b2`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	⚠️ Check →	Logs	Raw logs

raulvaldoleiros · 2021-07-13T17:59:05Z

@ebullient can you approve the workflow again please? I don't know why but I don't get the same error on my machine when I execute "./mvnw install -f extensions/jaeger/", it seems I only need to add the arc dependency to the runtime project (micrometer has it :p )

quarkus-bot · 2021-07-13T23:27:55Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `16a28d2`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Native Tests - Misc3	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Native Tests - Misc3 #

📦 integration-tests/smallrye-opentracing

✖ io.quarkus.it.rest.client.ZipkinIntegrationTestIT.testZipkinIntegration - More details - Source on GitHub

raulvaldoleiros · 2021-07-14T10:38:59Z

@ebullient I forgot to run the integration tests in native mode, my bad. I did it now.

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.869 s - in io.quarkus.it.rest.client.ZipkinIntegrationTestIT
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- maven-failsafe-plugin:3.0.0-M5:verify (default) @ quarkus-integration-test-smallrye-opentracing ---
[INFO]
[INFO] --- maven-install-plugin:2.5.2:install (default-install) @ quarkus-integration-test-smallrye-opentracing ---
[INFO] Installing /Users/raulvaldoleiros/git/quarkus/integration-tests/smallrye-opentracing/target/quarkus-integration-test-smallrye-opentracing-999-SNAPSHOT.jar to /Users/raulvaldoleiros/.m2/repository/io/quarkus/quarkus-integration-test-smallrye-opentracing/999-SNAPSHOT/quarkus-integration-test-smallrye-opentracing-999-SNAPSHOT.jar
[INFO] Installing /Users/raulvaldoleiros/git/quarkus/integration-tests/smallrye-opentracing/pom.xml to /Users/raulvaldoleiros/.m2/repository/io/quarkus/quarkus-integration-test-smallrye-opentracing/999-SNAPSHOT/quarkus-integration-test-smallrye-opentracing-999-SNAPSHOT.pom
[INFO] Installing /Users/raulvaldoleiros/git/quarkus/integration-tests/smallrye-opentracing/target/quarkus-integration-test-smallrye-opentracing-999-SNAPSHOT-sources.jar to /Users/raulvaldoleiros/.m2/repository/io/quarkus/quarkus-integration-test-smallrye-opentracing/999-SNAPSHOT/quarkus-integration-test-smallrye-opentracing-999-SNAPSHOT-sources.jar
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:54 min
[INFO] Finished at: 2021-07-14T10:55:08+01:00
[INFO] ------------------------------------------------------------------------

Also I had to create a zipkinconfig to analyse only the compatibility mode property in build time and the remaining ones in runtime.

Can you approve the workflow again please?

kenfinnigan · 2021-07-14T14:29:25Z

@raulvaldoleiros when the current CI finishes, if all is well does that mean everything works? JVM and Native work as expected with and without the jaeger-zipkin dependency?

raulvaldoleiros · 2021-07-14T15:33:26Z

@raulvaldoleiros when the current CI finishes, if all is well does that mean everything works? JVM and Native work as expected with and without the jaeger-zipkin dependency?

@kenfinnigan yes, I tested in jvm and native the following scenarios:

no dependency and zipkin mode disabled -> the span was delivered in jaeger
no dependency and zipkin mode enabled -> the span was delivered in jaeger ( i didn't changed the endpoint)
with dependency and zipkin mode enabled -> the span was delivered in zipkin
with dependency and zipkin mode disabled -> the span was delivered in jaeger

quarkus-bot · 2021-07-14T16:12:38Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `3d409ce`

Status	Name	Step	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 16	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/mailer

✖ io.quarkus.it.mailer.MailerTest. - More details - Source on GitHub

raulvaldoleiros · 2021-07-14T16:23:13Z

@kenfinnigan I don't think, this time, the error is related with my changes. In the log i found this ": org.testcontainers.containers.ContainerFetchException: Failed to pull image: reachfive/fake-smtp-server:latest" and I didn't change the project with the error and it doesn't have relation with jaeger extension. What should be done in this scenario?

kenfinnigan · 2021-07-14T16:59:29Z

@raulvaldoleiros confirmed it was just the mailer integration test that failed because it couldn't pull an image, nothing to do with this change.

kenfinnigan · 2021-07-14T17:07:09Z

docs/src/main/asciidoc/opentracing.adoc

+----
+
+It contains the dependencies to convert the request to zipkin format.
+The zipkin compatibility mode will be activate defining the config property as follows:


Minor comment on the wording, is it clearer if its "compatibility mode will be activated after defining the config property as follows"?

yes, i changed to that

kenfinnigan · 2021-07-14T17:10:21Z

extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/ZipkinReporterProvider.java

+    @Produces
+    @Singleton
+    @UnlessBuildProperty(name = "quarkus.jaeger.zipkin.compatibility-mode", stringValue = "false", enableIfMissing = false)
+    @AlternativePriority(Interceptor.Priority.APPLICATION + 100)


Is the @AlternativePriority needed? Are we expecting users to overload this with their own version?

We don't need it, i removed.

kenfinnigan · 2021-07-14T17:10:48Z

extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/ZipkinReporterProvider.java

+public class ZipkinReporterProvider {
+    @Produces
+    @Singleton
+    @UnlessBuildProperty(name = "quarkus.jaeger.zipkin.compatibility-mode", stringValue = "false", enableIfMissing = false)


Is this needed, or is the processor guard to make it unremovable enough?

its enough the processor guard, I removed it.
After all the changes I tested in a new application (jvm and native mode) the following scenarios:

no dependency and zipkin mode disabled -> the span was delivered in jaeger

no dependency and zipkin mode enabled -> the span was delivered in jaeger

with dependency and zipkin mode enabled -> the span was delivered in zipkin

with dependency and zipkin mode disabled -> the span was delivered in jaeger

kenfinnigan · 2021-07-15T17:41:34Z

Ah, one thing @raulvaldoleiros. Could you squash the commits into one commit?

raulvaldoleiros · 2021-07-16T08:42:58Z

Ah, one thing @raulvaldoleiros. Could you squash the commits into one commit?

yes @kenfinnigan , it's done :)

quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/jaeger labels Jul 1, 2021

raulvaldoleiros changed the title ~~Add zipkin compatibility mode #18278~~ Add zipkin compatibility mode to open-tracing Jul 1, 2021

kenfinnigan suggested changes Jul 1, 2021

View reviewed changes

extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/JaegerConfig.java Outdated Show resolved Hide resolved

extensions/jaeger/runtime/src/main/java/io/quarkus/jaeger/runtime/JaegerConfig.java Outdated Show resolved Hide resolved

raulvaldoleiros force-pushed the 18278 branch from f612f93 to d907777 Compare July 1, 2021 19:25

raulvaldoleiros requested a review from kenfinnigan July 2, 2021 10:08

kenfinnigan suggested changes Jul 6, 2021

View reviewed changes

raulvaldoleiros requested a review from kenfinnigan July 7, 2021 11:01

quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Jul 7, 2021

kenfinnigan mentioned this pull request Jul 9, 2021

Opentracing it #18169

Merged

quarkus-bot bot added the area/documentation label Jul 12, 2021

kenfinnigan reviewed Jul 14, 2021

View reviewed changes

Opentracing - Add zipkin compatibility mode

351c9ab

raulvaldoleiros force-pushed the 18278 branch from 484624f to 351c9ab Compare July 16, 2021 08:41

kenfinnigan approved these changes Jul 16, 2021

View reviewed changes

kenfinnigan added this to the 2.2 - main milestone Jul 16, 2021

kenfinnigan added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 16, 2021

kenfinnigan merged commit 9ef5c49 into quarkusio:main Jul 16, 2021

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 16, 2021

raulvaldoleiros mentioned this pull request Dec 20, 2022

Allow integration with zipkin #18278

Closed

Add zipkin compatibility mode to open-tracing #18313

Add zipkin compatibility mode to open-tracing #18313

Conversation

raulvaldoleiros commented Jul 1, 2021

quarkus-bot bot commented Jul 1, 2021

kenfinnigan left a comment

Choose a reason for hiding this comment

raulvaldoleiros commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Jul 7, 2021

raulvaldoleiros commented Jul 7, 2021

raulvaldoleiros commented Jul 7, 2021

quarkus-bot bot commented Jul 7, 2021 • edited Loading

Failing Jobs - Building 17d9873

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/smallrye-opentracing

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/smallrye-opentracing

⚙️ Native Tests - Misc3 #

📦 integration-tests/smallrye-opentracing

ebullient commented Jul 12, 2021 • edited Loading

raulvaldoleiros commented Jul 13, 2021

quarkus-bot bot commented Jul 13, 2021 • edited Loading

Failing Jobs - Building 0a570b2

raulvaldoleiros commented Jul 13, 2021 • edited Loading

quarkus-bot bot commented Jul 13, 2021 • edited Loading

Failing Jobs - Building 16a28d2

Test Failures

⚙️ Native Tests - Misc3 #

📦 integration-tests/smallrye-opentracing

raulvaldoleiros commented Jul 14, 2021

kenfinnigan commented Jul 14, 2021

raulvaldoleiros commented Jul 14, 2021

quarkus-bot bot commented Jul 14, 2021 • edited Loading

Failing Jobs - Building 3d409ce

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/mailer

raulvaldoleiros commented Jul 14, 2021

kenfinnigan commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenfinnigan commented Jul 15, 2021

raulvaldoleiros commented Jul 16, 2021 • edited Loading

quarkus-bot bot commented Jul 7, 2021 •

edited

Loading

Failing Jobs - Building `17d9873`

ebullient commented Jul 12, 2021 •

edited

Loading

quarkus-bot bot commented Jul 13, 2021 •

edited

Loading

Failing Jobs - Building `0a570b2`

raulvaldoleiros commented Jul 13, 2021 •

edited

Loading

quarkus-bot bot commented Jul 13, 2021 •

edited

Loading

Failing Jobs - Building `16a28d2`

quarkus-bot bot commented Jul 14, 2021 •

edited

Loading

Failing Jobs - Building `3d409ce`

raulvaldoleiros commented Jul 16, 2021 •

edited

Loading