-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 zipkin compatibility mode to open-tracing #18313
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 33 additions & 0 deletions
33
extensions/jaeger/deployment/src/main/java/io/quarkus/jaeger/deployment/ZipkinProcessor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package io.quarkus.jaeger.deployment; | ||
|
||
import java.util.function.BooleanSupplier; | ||
|
||
import io.quarkus.arc.deployment.AdditionalBeanBuildItem; | ||
import io.quarkus.deployment.annotations.BuildProducer; | ||
import io.quarkus.deployment.annotations.BuildStep; | ||
import io.quarkus.jaeger.runtime.JaegerDeploymentRecorder; | ||
import io.quarkus.jaeger.runtime.ZipkinConfig; | ||
import io.quarkus.jaeger.runtime.ZipkinReporterProvider; | ||
|
||
public class ZipkinProcessor { | ||
|
||
static final String REGISTRY_CLASS_NAME = "zipkin2.reporter.urlconnection.URLConnectionSender"; | ||
static final Class<?> REGISTRY_CLASS = JaegerDeploymentRecorder.getClassForName(REGISTRY_CLASS_NAME); | ||
|
||
public static class ZipkinEnabled implements BooleanSupplier { | ||
ZipkinConfig config; | ||
|
||
public boolean getAsBoolean() { | ||
return REGISTRY_CLASS != null && config.compatibilityMode; | ||
} | ||
} | ||
|
||
@BuildStep(onlyIf = ZipkinEnabled.class) | ||
void addZipkinClasses(BuildProducer<AdditionalBeanBuildItem> additionalBeans) { | ||
|
||
// Add Zipkin classes | ||
additionalBeans.produce(AdditionalBeanBuildItem.builder().addBeanClass(ZipkinReporterProvider.class) | ||
.setUnremovable().build()); | ||
|
||
} | ||
} |
98 changes: 98 additions & 0 deletions
98
...sions/jaeger/deployment/src/test/java/io/quarkus/jaeger/test/QuarkusJaegerTracerTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
package io.quarkus.jaeger.test; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
||
import javax.enterprise.inject.Default; | ||
import javax.enterprise.inject.Instance; | ||
import javax.enterprise.inject.spi.CDI; | ||
|
||
import org.junit.jupiter.api.Test; | ||
import org.mockito.ArgumentCaptor; | ||
import org.mockito.MockedStatic; | ||
import org.mockito.Mockito; | ||
|
||
import io.jaegertracing.Configuration; | ||
import io.jaegertracing.internal.JaegerTracer; | ||
import io.jaegertracing.internal.JaegerTracer.Builder; | ||
import io.jaegertracing.spi.Reporter; | ||
import io.jaegertracing.zipkin.ZipkinV2Reporter; | ||
import io.opentracing.Tracer; | ||
import io.quarkus.jaeger.runtime.QuarkusJaegerTracer; | ||
import io.quarkus.jaeger.runtime.ReporterFactory; | ||
import io.quarkus.jaeger.runtime.ZipkinReporterFactoryImpl; | ||
|
||
public class QuarkusJaegerTracerTest { | ||
|
||
@Test | ||
@SuppressWarnings("unchecked") | ||
public void withzipkinCompatibilityMode() { | ||
|
||
try (MockedStatic<Configuration> mockedStaticConfiguration = Mockito.mockStatic(Configuration.class); | ||
MockedStatic<CDI> mockedStaticCDI = Mockito.mockStatic(CDI.class)) { | ||
|
||
CDI<Object> mockedCDI = (CDI<Object>) Mockito.mock(CDI.class); | ||
|
||
mockedStaticCDI.when(() -> CDI.current()).thenReturn(mockedCDI); | ||
|
||
Instance instanceCDI = Mockito.mock(Instance.class); | ||
Mockito.when(instanceCDI.isAmbiguous()).thenReturn(false); | ||
Mockito.when(instanceCDI.isUnsatisfied()).thenReturn(false); | ||
Mockito.when(instanceCDI.get()).thenReturn(new ZipkinReporterFactoryImpl()); | ||
Mockito.when(mockedCDI.select(ReporterFactory.class, Default.Literal.INSTANCE)).thenReturn(instanceCDI); | ||
|
||
Configuration mockedInstanceConfiguration = Mockito.mock(Configuration.class); | ||
Builder mockedBuilder = Mockito.mock(Builder.class); | ||
Tracer mockedTracer = Mockito.mock(JaegerTracer.class); | ||
|
||
mockedStaticConfiguration.when(() -> Configuration.fromEnv()).thenReturn(mockedInstanceConfiguration); | ||
mockedStaticConfiguration.when(() -> mockedInstanceConfiguration.withMetricsFactory(Mockito.any())) | ||
.thenReturn(mockedInstanceConfiguration); | ||
mockedStaticConfiguration.when(() -> mockedInstanceConfiguration.getTracerBuilder()) | ||
.thenReturn(mockedBuilder); | ||
mockedStaticConfiguration.when(() -> mockedBuilder.withScopeManager(Mockito.any())) | ||
.thenReturn(mockedBuilder); | ||
mockedStaticConfiguration.when(() -> mockedBuilder.withReporter(Mockito.any())).thenReturn(mockedBuilder); | ||
mockedStaticConfiguration.when(() -> mockedBuilder.build()).thenReturn(mockedTracer); | ||
|
||
QuarkusJaegerTracer tracer = new QuarkusJaegerTracer(); | ||
tracer.setZipkinCompatibilityMode(true); | ||
tracer.setEndpoint("http://localhost"); | ||
tracer.toString(); | ||
tracer.close(); | ||
|
||
ArgumentCaptor<Reporter> argument = ArgumentCaptor.forClass(Reporter.class); | ||
Mockito.verify(mockedBuilder).withReporter(argument.capture()); | ||
assertEquals(ZipkinV2Reporter.class, argument.getValue().getClass()); | ||
} | ||
|
||
} | ||
|
||
@Test | ||
public void withoutZipkinCompatibilityMode() { | ||
try (MockedStatic<Configuration> mockedStaticConfiguration = Mockito.mockStatic(Configuration.class)) { | ||
Configuration mockedInstanceConfiguration = Mockito.mock(Configuration.class); | ||
Builder mockedBuilder = Mockito.mock(Builder.class); | ||
Tracer mockedTracer = Mockito.mock(JaegerTracer.class); | ||
|
||
mockedStaticConfiguration.when(() -> Configuration.fromEnv()).thenReturn(mockedInstanceConfiguration); | ||
mockedStaticConfiguration.when(() -> mockedInstanceConfiguration.withMetricsFactory(Mockito.any())) | ||
.thenReturn(mockedInstanceConfiguration); | ||
mockedStaticConfiguration.when(() -> mockedInstanceConfiguration.getTracerBuilder()) | ||
.thenReturn(mockedBuilder); | ||
mockedStaticConfiguration.when(() -> mockedBuilder.withScopeManager(Mockito.any())) | ||
.thenReturn(mockedBuilder); | ||
mockedStaticConfiguration.when(() -> mockedBuilder.withReporter(Mockito.any())).thenReturn(mockedBuilder); | ||
mockedStaticConfiguration.when(() -> mockedBuilder.build()).thenReturn(mockedTracer); | ||
|
||
QuarkusJaegerTracer tracer = new QuarkusJaegerTracer(); | ||
tracer.toString(); | ||
tracer.close(); | ||
|
||
ArgumentCaptor<Reporter> argument = ArgumentCaptor.forClass(Reporter.class); | ||
Mockito.verify(mockedBuilder).withReporter(argument.capture()); | ||
assertNull(argument.getValue()); | ||
} | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made what you requested and the results are:
without my changes:
With my changes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is extra configuration for native mode still necessary? If so, what configuration?