add exporter configurations to springboot autoconfigure by mabdinur · Pull Request #729 · open-telemetry/opentelemetry-java-instrumentation

mabdinur · 2020-07-19T03:00:50Z

Add Spring Boot auto configurations and configuration properties for OTLPGrpcSpanExporter, LoggingSpanExporter, JaegerSpanExporter, and ZipkinSpanExporter.

My next pull request will add spring boot starters for these exporters. Here's a preview: https://github.com/mabdinur/opentelemetry-java-instrumentation/pull/6/files :)

anuraaga · 2020-07-20T09:57:29Z

 * Configuration for OpenTelemetry Tracer
 *
- * <p>Configures LoggingExporter and sets default tracer name
+ * <p>Sets default tracer name and sampler probability


Let's go ahead and remove this, and instead add javadoc to at least the getters.

Sorry. I forgot to clean up the java docs before creating this review. I'll fix this up now

anuraaga · 2020-07-20T09:58:45Z

+
+  compileOnly deps.opentelemetryJaeger
+  compileOnly deps.opentelemetryOtlp
+  implementation group: 'io.grpc', name: 'grpc-api', version: '1.24.0'


Aren't these included transitively from the above deps? Even if not, these should be compileOnly as well, not implementation

When I set the grpc-api dependency to compileOnly I get this exception: org.springframework.beans.factory.UnsatisfiedDependencyException. I'm not sure why this dependency isn't provided transitively by the opentelemetry-java jaeger-exporter. A similar pattern could be seen in the auto-exporter-jaeger and auto-exporter-zipkin modules. I'm not sure if this was intended or if it's a bug

@trask @anuraaga Should I open an issue to add the io.grpc.grpc-api and io.grpc.grpc-netty-shaded dependencies to the opentelemetry Otlp and Jaeger exporters.

Would it make sense to keep these exporters self contained?

If so, we could also add the io.zipkin.reporter2.zipkin-sender-okhttp3 dependency to the the zipkin exporter.

Sorry for the late reply.

I think it could make sense to have those transitive dependencies on the exporters in the SDK so could be nice to file an issue.

In the meantime, we can include dependencies here if needed. Can you post the stacktrace for the UnsatisfiedDependencyException? We have e.g., ConditionalOnMissingBean etc because we want to make sure we only instrument what a user brought into their app, so we need to avoid implementation here in favor of compileOnly, etc - api or implementation will bring the dependency into the user's app regardless of their config. If you're seeing it in a test, you may need to use testRuntimeOnly or similar.

Hmmm after I rebased from master this error went away. It looks like the opentelemetry-exporters-jaeger-0.7.0-SNAPSHOT has the grpc dependencies. I'll change these dependencies to compileOnly

For reference this is the runtime error I get when I use opentelemetry-exporters-jaeger:0.6.0 and I set io.grpc.grpc-netty-shaded:1.24.0 to compileOnly I get the following error:

Caused by: io.grpc.ManagedChannelProvider$ProviderNotFoundException: No functional channel service provider found. Try adding a dependency on the grpc-okhttp, grpc-netty, or grpc-netty-shaded artifact at io.grpc.ManagedChannelProvider.provider(ManagedChannelProvider.java:60) ~[grpc-api-1.28.0.jar:1.28.0] at io.grpc.ManagedChannelBuilder.forTarget(ManagedChannelBuilder.java:76) ~[grpc-api-1.28.0.jar:1.28.0] at io.opentelemetry.instrumentation.spring.autoconfigure.exporters.jaeger.JaegerSpanExporterAutoConfiguration.otelJaegerSpanExporter(JaegerSpanExporterAutoConfiguration.java:52) ~[opentelemetry-spring-boot-autoconfigure-0.6.0-SNAPSHOT.jar:0.6.0-SNAPSHOT] at io.opentelemetry.instrumentation.spring.autoconfigure.exporters.jaeger.JaegerSpanExporterAutoConfiguration$$EnhancerBySpringCGLIB$$652d2c30.CGLIB$otelJaegerSpanExporter$0(<generated>) ~[opentelemetry-spring-boot-autoconfigure-0.6.0-SNAPSHOT.jar:0.6.0-SNAPSHOT] at io.opentelemetry.instrumentation.spring.autoconfigure.exporters.jaeger.JaegerSpanExporterAutoConfiguration$$EnhancerBySpringCGLIB$$652d2c30$$FastClassBySpringCGLIB$$3ee7321d.invoke(<generated>) ~[opentelemetry-spring-boot-autoconfigure-0.6.0-SNAPSHOT.jar:0.6.0-SNAPSHOT] at org.springframework.cglib.proxy.MethodProxy.invokeSuper(MethodProxy.java:244) ~[spring-core-5.2.7.RELEASE.jar:5.2.7.RELEASE] at org.springframework.context.annotation.ConfigurationClassEnhancer$BeanMethodInterceptor.intercept(ConfigurationClassEnhancer.java:331) ~[spring-context-5.2.7.RELEASE.jar:5.2.7.RELEASE] at io.opentelemetry.instrumentation.spring.autoconfigure.exporters.jaeger.JaegerSpanExporterAutoConfiguration$$EnhancerBySpringCGLIB$$652d2c30.otelJaegerSpanExporter(<generated>) ~[opentelemetry-spring-boot-autoconfigure-0.6.0-SNAPSHOT.jar:0.6.0-SNAPSHOT] at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na] at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:na] at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na] at java.base/java.lang.reflect.Method.invoke(Method.java:566) ~[na:na] at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:154) ~[spring-beans-5.2.7.RELEASE.jar:5.2.7.RELEASE] ... 62 common frames omitted

hey @mabdinur, can you try adding:

compileOnly group: 'io.grpc', name: 'grpc-api', version: '1.24.0'

to your spring-boot-autoconfigure.gradle?

I think that will fix the current build issue.

For reference this is the runtime error I get when I use opentelemetry-exporters-jaeger:0.6.0 and I set io.grpc.grpc-netty-shaded:1.24.0 to compileOnly

Where are you using io.grpc.grpc-netty-shaded:1.24.0? Is part of an app you are testing? In that case I don't think you want compileOnly. We just want compileOnly for our own transitive dependencies here, so that we don't affect an application's choice of gRPC version.

I need io.grpc.grpc-netty-shaded:1.24.0 to run the otlp exporter (version 0.6.0) and the jaeger exporter (0.6.0). This dependency wasn't supplied by the exporter package. I don't need to include it in the autoconfigure project. Instead I'll include it in a opentelemetry-spring-starter https://github.com/mabdinur/opentelemetry-java-instrumentation/pull/6/files

mabdinur · 2020-07-20T17:40:25Z

-  api deps.opentelemetryApi 
-  api "io.opentelemetry:opentelemetry-exporters-logging:0.5.0"


Moving these apis to an opentelemetry-spring-starter. A draft of this can be see here: mabdinur#6

udekel

Some minor comments attached.

udekel · 2020-07-22T00:32:55Z

-      return;
-    }
+  private void addSpanProcessors(List<SpanExporter> spanExporters) {
+


[Suggestion] Drop the extra newline here.

udekel · 2020-07-22T00:33:55Z

+  private void setSampler(TracerProperties tracerProperties) {
+    TraceConfig.getDefault()
+        .toBuilder()
+        .setSampler(Samplers.probability(tracerProperties.getSamplerProbability()))


[Question] Is that always guaranteed to be a valid value?

Yes, I added validators to the TracerProperties class that bounds the decimal value between 0 and 1

udekel · 2020-07-22T00:34:51Z

 * Configuration for OpenTelemetry Tracer
 *
- * <p>Configures LoggingExporter and sets default tracer name
+ * <p>Get Tracer Name {@link getName()}


Here and elsewhere: You don't need usually need to provide a list of the methods of the class in its documentation. IS there a special convention for beans?

udekel · 2020-07-22T00:35:21Z


-  public void setLoggingExporterEnabled(boolean loggingExporterEnabled) {
-    this.loggingExporterEnabled = loggingExporterEnabled;
+  public void setSamplerProbability(double samplerProbability) {


Is it worth documenting that 0.0 / 1.0 have special values?

udekel · 2020-07-22T00:36:20Z

+@ConfigurationProperties(prefix = "opentelemetry.trace.exporter.jaeger")
+public final class JaegerSpanExporterProperties {
+
+  private boolean enabled = true;


[Note] I'm not sure about the convention, but I'd consider making the defaults part of the class documentation.

I'd also add a link to where that 14250 is coming from.

udekel · 2020-07-22T00:36:31Z

+  private String serviceName = "otel-spring-boot-jaeger-exporter";
+  private String host = "localhost";
+  private int port = 14250;
+  private Duration deadline = Duration.ofMillis(1000L);


I think you can just use ofSeconds(1)

trask · 2020-07-22T19:44:29Z

hey @mabdinur, can you merge master into your branch to fix the finatra CI failure (see #747)?

trask

good stuff 👍

a couple comments on configuration properties, check out what's coming around common configuration env vars in open-telemetry/opentelemetry-specification#666, good to align the spring configuration with that where possible

trask

LGTM! Just one thing below.

trask

👍

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/jaeger/JaegerSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpGrpcSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/jaeger/JaegerSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/zipkin/ZipkinSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…st/port property javadocs

anuraaga · 2020-07-25T04:17:03Z

+  compileOnly "io.opentelemetry:opentelemetry-exporters-logging:0.5.0"
+  compileOnly deps.opentelemetryJaeger
+  compileOnly deps.opentelemetryOtlp
+  compileOnly group: 'io.grpc', name: 'grpc-api', version: '1.24.0'


It sounds like the transitive dependency problem got fixed. Doesn't it work to remove these extra compileOnly completely?

anuraaga

Thanks!

mabdinur marked this pull request as ready for review July 20, 2020 04:13

mabdinur requested review from anuraaga, iNikem, jkwatson, trask and tylerbenson as code owners July 20, 2020 04:13

anuraaga reviewed Jul 20, 2020

View reviewed changes

mabdinur commented Jul 20, 2020

View reviewed changes

mabdinur requested a review from anuraaga July 20, 2020 17:56

trask mentioned this pull request Jul 21, 2020

Inject span context into log4j2 2.13.2+ context. #735

Merged

udekel reviewed Jul 22, 2020

View reviewed changes

trask reviewed Jul 22, 2020

View reviewed changes

mabdinur force-pushed the add_exports_to_springboot_autoconfigure branch from f8f59d2 to 97ff9ae Compare July 23, 2020 00:38

mabdinur requested a review from trask July 23, 2020 05:08

trask reviewed Jul 23, 2020

View reviewed changes

Comment thread ...main/java/io/opentelemetry/instrumentation/spring/autoconfigure/TracerAutoConfiguration.java Outdated

mabdinur requested a review from trask July 23, 2020 19:25

trask approved these changes Jul 23, 2020

View reviewed changes

mabdinur and others added 12 commits July 24, 2020 21:32

add exporter configurations to springboot autoconfigure

4d5d085

clean up javadoc and jaegerspanexporter name

bbed776

format

df0a0e2

fix javadoc

e3cc7e2

Update spring-boot-autoconfigure.gradle

7b20642

format and clean up java docs

4d49d66

review changes

36f415d

edit host name

d125947

Update TracerProperties.java

925cc1c

Update instrumentation-core/spring/spring-boot-autoconfigure/src/main…

9b4d492

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/jaeger/JaegerSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update instrumentation-core/spring/spring-boot-autoconfigure/src/main…

0d6109f

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpGrpcSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

format

d52ebd7

mabdinur and others added 7 commits July 24, 2020 21:32

Update instrumentation-core/spring/spring-boot-autoconfigure/src/main…

81ac031

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpGrpcSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update instrumentation-core/spring/spring-boot-autoconfigure/src/main…

8c6696f

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/jaeger/JaegerSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update instrumentation-core/spring/spring-boot-autoconfigure/src/main…

9deec57

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/zipkin/ZipkinSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update instrumentation-core/spring/spring-boot-autoconfigure/src/main…

87f4faa

…/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/zipkin/ZipkinSpanExporterProperties.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

fixed properties

cdd4263

update active tracer config with sampling probability and clean up ho…

a370c04

…st/port property javadocs

fix duration property name to align with docs

6535eda

mabdinur force-pushed the add_exports_to_springboot_autoconfigure branch from 0e761ec to 6535eda Compare July 24, 2020 21:33

Update spring-boot-autoconfigure.gradle

4d53a80

anuraaga reviewed Jul 25, 2020

View reviewed changes

trask mentioned this pull request Jul 25, 2020

Add public constants for all exporter default configuration values open-telemetry/opentelemetry-java#1465

Closed

mabdinur added 4 commits July 25, 2020 11:20

Update spring-boot-autoconfigure.gradle

8465b75

Update spring-boot-autoconfigure.gradle

6fc41bf

add conditional on managed channel

02f3da1

conditional on managed channel

7737f3d

mabdinur requested a review from anuraaga July 27, 2020 00:01

anuraaga approved these changes Jul 27, 2020

View reviewed changes

anuraaga merged commit 72c36b4 into open-telemetry:master Jul 27, 2020

		api deps.opentelemetryApi
		api "io.opentelemetry:opentelemetry-exporters-logging:0.5.0"

Conversation

mabdinur commented Jul 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabdinur Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabdinur Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

udekel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trask commented Jul 22, 2020

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mabdinur commented Jul 19, 2020 •

edited

Loading

mabdinur Jul 20, 2020 •

edited

Loading

anuraaga Jul 24, 2020 •

edited

Loading

mabdinur Jul 25, 2020 •

edited

Loading