Add support for OpenTelemetry #65

n0tl3ss · 2022-04-06T10:06:11Z

This PR adds OpenTelemetry in the `micronaut-tracing` module.

Modules

Five new modules are added:

micronaut-tracing-annotation - Annotation that are used both inside the Open Telemetry and Open Tracing.
micronaut-tracing-opentelemetry -Adds Open Telemetry default instrumentation and uses Open Telemetry Auto configure SDK.
micronaut-tracing-opentelemetry-annotation - Mapper and Transformer for Open Telemetry @WithSpan and @SpanAttribute annotations.
micronaut-tracing-opentelemetry-http - Open Telemetry Instrumentation for HTTP Client and Server.
micronaut-tracing-opentelemetry-grpc - Open Telemetry Instrumentation for GRPC Client and Server.

Supported Features

Starting Span objects on the client/server HTTP requests and ending them on the client/server HTTP response.
Starting Span objects on the client/server GRPC requests and ending them on the client/server GRPC response.
The Open Telemetry @WithSpan and @SpanAttribute annotations are fully supported. The @SpanTag , @NewSpan, @ContinueSpan from the Open Tracing are also usable.
Adding custom headers inside Span Objects defined inside application.yml file
Open Telemetry Auto configure SDK config variables can also be defined inside application.yml.
Defaults that are defined inside this module: otel.traces.exporter = none, otel.metrics.exporter = none, otel.logs.exporte = none, otel.service.name = value of the application.name. This values can be overridden through application.yml file.

Other

Added tests that covers OpenTelemetry integration and tests old and new annotations.
Usage is described inside module documentation.

donbeave · 2022-04-09T00:41:55Z

Maybe it's better to add additional modules to initialize different OTEL standalone instrumentations. For example, if you want to active OTEL for gRPC, you should add micronaut-tracing-opentelemetry-grpc-server, which will add the official library io.opentelemetry.instrumentation:opentelemetry-grpc-1.6 library to the classpath and initialize grpc ServerInterceptor, for client side support should add micronaut-tracing-opentelemetry-grpc-client. With this approach, it will reuse official libraries and will not add extra unused dependencies to the classpath.

With current Micronaut features ecosystem I can see a list of such modules:

micronaut-tracing-opentelemetry-aws-sdk
micronaut-tracing-opentelemetry-grpc-server
micronaut-tracing-opentelemetry-grpc-client
micronaut-tracing-opentelemetry-graphql
micronaut-tracing-opentelemetry-jdbc
micronaut-tracing-opentelemetry-lettuce
micronaut-tracing-opentelemetry-log4j
micronaut-tracing-opentelemetry-logback
micronaut-tracing-opentelemetry-mongo
micronaut-tracing-opentelemetry-reactor
micronaut-tracing-opentelemetry-rxjava1
micronaut-tracing-opentelemetry-rxjava2
micronaut-tracing-opentelemetry-rxjava3

graemerocher · 2022-04-12T08:40:23Z

tracing-core/src/main/java/io/micronaut/tracing/DefaultConfiguration.java

+ * @since 1.0
+ */
+@ConfigurationProperties(PREFIX)
+public class DefaultConfiguration implements Toggleable {


Rename this to something more specific. DefaultConfiguration will too generic a name.
Member

I would probably also use an interface here as it will be simpler:

@ConfigurationProperties(PREFIX) interface TracingConfiguration extends Toggleable { String PREFIX = "tracing"; }

graemerocher · 2022-04-12T08:40:48Z

tracing-opentelemetry/src/main/java/io/micronaut/tracing/DefaultOpenTelemetryFactory.java

+     */
+    @Singleton
+    @Primary
+    OpenTelemetry defaultOpenTelemetry() {


Probably worth making protected if the intention is to allow the user to subclass and override

graemerocher · 2022-04-12T08:41:00Z

tracing-opentelemetry/src/main/java/io/micronaut/tracing/DefaultTelemetryTracer.java

+public class DefaultTelemetryTracer {
+
+    @Property(name = APPLICATION_NAME)
+    String applicationName;


Try not to use field injection. Better to make this final and use constructor injection making the type immutable.

graemerocher · 2022-04-12T08:42:15Z

...telemetry/src/main/java/io/micronaut/tracing/opentelemetry/interceptor/TraceInterceptor.java

+        } else {
+            // must be new
+            String operationName = newSpan.stringValue().orElse(null);
+            Optional<String> hystrixCommand = context.stringValue(HYSTRIX_ANNOTATION);


You can remove this logic, hystrix is deprecated

graemerocher · 2022-04-12T08:42:43Z

...telemetry/src/main/java/io/micronaut/tracing/opentelemetry/interceptor/TraceInterceptor.java

+        context.stringValue(HYSTRIX_ANNOTATION, "group").ifPresent(s ->
+                span.setAttribute(TAG_HYSTRIX_GROUP, s)
+        );
+        context.stringValue(HYSTRIX_ANNOTATION, "threadPool").ifPresent(s ->
+                span.setAttribute(TAG_HYSTRIX_THREAD_POOL, s)
+        );


you can remove this hystrix is deprecated

graemerocher · 2022-04-12T08:43:17Z

...lemetry/src/main/java/io/micronaut/tracing/opentelemetry/util/ScopePropagationPublisher.java

+        }
+
+        try (Scope ignored = span.makeCurrent()) {
+            publisher.subscribe(new Subscriber<T>() {


needs to implement CoreSubscriber from reactor for propagation to work

graemerocher · 2022-04-12T08:43:46Z

...ng-opentelemetry/src/main/java/io/micronaut/tracing/opentelemetry/util/TracingPublisher.java

+     * The tracing subscriber.
+     */
+    @Internal
+    protected class TracingSubscriber implements Subscriber<T> {


needs to implement CoreSubscriber

donbeave

@n0tl3ss please also check the example of gRPC interceptors, which also reuse the official standalone gRPC instrumentation library: https://github.com/tailrocks/micronaut-opentelemetry/tree/7f1e83e70e023375b3fe6684a8c273fc649d1682/opentelemetry/src/main/java/io/micronaut/opentelemetry/instrumentation/grpc

donbeave · 2022-04-09T00:53:45Z

...lemetry/src/main/java/io/micronaut/tracing/opentelemetry/http/AbstractOpenTracingFilter.java

+ * @author Nemanja Mikic
+ * @since 1.0
+ */
+public abstract class AbstractOpenTracingFilter implements HttpFilter {


I suggest don't use a custom implementation of OTEL instrumenter, we can reuse the official library here. The benefit of this approach is that Micronaut traces will have the same format as the official OTEL implementation. For those companies who combine different frameworks and run multiple applications, some with OTEL javaagent they will not see differences in OTEL spans created in Micronaut.
Please check my comments for OpenTracingClientFilter and OpenTracingServerFilter, which provide links to examples.

donbeave · 2022-04-13T21:38:57Z

...lemetry/src/main/java/io/micronaut/tracing/opentelemetry/http/OpenTelemetryClientFilter.java

+ */
+@Filter(CLIENT_PATH)
+@Requires(beans = Tracer.class)
+public class OpenTelemetryClientFilter extends AbstractOpenTracingFilter implements HttpClientFilter {


It's better to use the official opentelemetry-instrumentation-api library here, which provides API to simplify creating new instrumentation. You can check my example which uses this library: https://github.com/tailrocks/micronaut-opentelemetry/blob/7f1e83e70e023375b3fe6684a8c273fc649d1682/opentelemetry/src/main/java/io/micronaut/opentelemetry/instrumentation/http/client/OpenTelemetryClientFilter.java#L41

If you need some explanation why it's much better than re-implement everything from scratch, please tag me and I will provide some examples.

donbeave · 2022-04-13T21:39:56Z

...lemetry/src/main/java/io/micronaut/tracing/opentelemetry/http/OpenTelemetryServerFilter.java

+ */
+@Filter(SERVER_PATH)
+@Requires(beans = Tracer.class)
+public class OpenTelemetryServerFilter extends AbstractOpenTracingFilter implements HttpServerFilter {


Please take a look at this example which reuse official opentelemetry-instrumentation-api library: https://github.com/tailrocks/micronaut-opentelemetry/blob/7f1e83e70e023375b3fe6684a8c273fc649d1682/opentelemetry/src/main/java/io/micronaut/opentelemetry/instrumentation/http/server/OpenTelemetryServerFilter.java#L42

...emetry/src/main/java/io/micronaut/tracing/opentelemetry/instrument/util/MdcInstrumenter.java

...io/micronaut/tracing/opentelemetry/instrument/util/TracingInvocationInstrumenterFactory.java

...ronaut/tracing/opentelemetry/instrument/util/ThreadTracingInvocationInstrumenterFactory.java

donbeave

I'll try this branch in some of my Micronaut applications and provide you with some feedback.

donbeave · 2022-04-18T09:48:16Z

tracing-opentelemetry/build.gradle

+dependencies {
+    api mn.micronaut.runtime
+    api mn.micronaut.http.client
+    api mn.micronaut.grpc.server.runtime


I think we better split it into modules or add all initializers into one module but activate only if the class is provided in the classpath. For example to activate gRPC interceptors you should add:

implementation("io.opentelemetry.instrumentation:opentelemetry-grpc-1.6")

Same for Micronaut HTTP client as well.

donbeave · 2022-04-18T09:51:31Z

.../io/micronaut/tracing/opentelemetry/instrument/grpc/GrpcClientTracingInterceptorFactory.java

+ * @author Nemanja Mikic
+ */
+@Factory
+public class GrpcClientTracingInterceptorFactory {


It's better to write a test, this can be an example: https://github.com/micronaut-projects/micronaut-grpc/blob/master/grpc-client-runtime/src/test/groovy/io/micronaut/grpc/server/tracing/GrpcTracingSpec.groovy

donbeave · 2022-04-18T09:52:31Z

gradle/libs.versions.toml

@@ -16,7 +18,14 @@ opentracing = { module = 'io.opentracing:opentracing-api', version.ref = 'opentr
 opentracing-util = { module = 'io.opentracing:opentracing-util', version.ref = 'opentracing' }
 zipkin-brave-instrumentation = { module = 'io.zipkin.brave:brave-instrumentation-http', version.ref = 'brave-instrumentation' }
 zipkin-reporter = { module = 'io.zipkin.reporter2:zipkin-reporter', version.ref = 'zipkin-reporter' }
-
+opentelemetry-api = {module='io.opentelemetry:opentelemetry-api', version.ref ='opentelemetry'}


Seems it's not formatted as usual. Please apply the same format.

donbeave · 2022-04-18T09:52:59Z

gradle/libs.versions.toml

+opentelemetry-sdk-logs = {module='io.opentelemetry:opentelemetry-sdk-logs', version.ref = 'opentelemetry-alpha'}
+opentelemetry-extension-annotations = {module='io.opentelemetry:opentelemetry-extension-annotations', version.ref = 'opentelemetry'}
+opentelemetry-instrumentation-api = {module='io.opentelemetry.instrumentation:opentelemetry-instrumentation-api', version.ref = 'opentelemetry-alpha'}
+opentelemetry-instrumentation-grpc = {module='io.opentelemetry.instrumentation:opentelemetry-grpc-1.6', version.ref = 'opentelemetry-alpha'}
 [plugins]


Maybe add a new line before [plugins]?

n0tl3ss · 2022-04-18T12:07:40Z

@donbeave I will try to add grpc tests. Do you want me to create tracing-opentelemetry-grpc module and move everything related to the grpc there?

With test I have an issue with generating protobuff classes.

Here is an error:


An exception occurred applying plugin request [id: 'io.micronaut.build.internal.docs']
> Failed to apply plugin class 'io.micronaut.build.docs.JavadocAggregatorPlugin'.
   > A problem occurred configuring project ':tracing-bom'.
      > A problem occurred configuring project ':tracing-opentelemetry'.
         > A problem occurred evaluating project ':tracing-opentelemetry'.
            > Could not find method protobuf() for arguments [build_96ccfw8zuqrkx91x0900im2yo$_run_closure3@1472beea] on project ':tracing-opentelemetry' of type org.gradle.api.Project.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

n0tl3ss · 2022-04-18T12:18:07Z

@donbeave I will try to add grpc tests. Do you want me to create tracing-opentelemetry-grpc module and move everything related to the grpc there?

Also this test io.micronaut.tracing.instrument.util.AnnotationMappingSpec test map WithSpan annotation is falling somehow on CI/CD but locally its passes: https://ge.micronaut.io/s/hcasuab4pcl2k

donbeave · 2022-04-18T12:46:30Z

Do you want me to create tracing-opentelemetry-grpc module and move everything related to the grpc there?

I'm not sure which way is better, maybe @graemerocher can give us some feedback.

I see two approaches

separate plugin for each stack, like micronaut-tracing-opentelemetry-http-client, micronaut-tracing-opentelemetry-http-server, micronaut-tracing-opentelemetry-grpc, micronaut-tracing-opentelemetry-graphql, etc.

or

put everything in micronaut-tracing-opentelemetry, but activates only if related classes are provided in the classpath, for example to activates gRPC user should add implementation("io.opentelemetry.instrumentation:opentelemetry-grpc-1.6"), to activate GraphQL user should add implementation("io.opentelemetry.instrumentation:opentelemetry-graphql-java-12.0"), etc

donbeave · 2022-04-18T16:51:51Z

...ng-opentelemetry/src/main/java/io/micronaut/tracing/processing/WithSpanAnnotationMapper.java

+    @Override
+    public List<AnnotationValue<?>> map(AnnotationValue<WithSpan> annotation, VisitorContext visitorContext) {
+        AnnotationValue<InterceptorBinding> interceptorBinding = AnnotationValue.builder(InterceptorBinding.class)
+            .value(ContinueSpan.class)


Looks like it's not correct to map WithSpan to ContinueSpan annotation, it should be mapped to NewSpan annotation instead, based on its description from javadoc:

This annotation marks that an execution of this method or constructor should result in a new io.opentelemetry.api.trace.Span. Application developers can use this annotation to signal OpenTelemetry auto-instrumentation that a new span should be created whenever marked method is executed.

I have changed WithSpan to match the NewSpan. Grpc test added, and I don't know how to fix test that is falling on ci / cd because localy it is passing: https://ge.micronaut.io/s/53d2qu5xuicqy. The issue is in mapping/transforming annotations.

...elemetry-grpc/src/test/groovy/io/micronaut/tracing/util/TestDefaultOpenTelemetryFactory.java

...ava/io/micronaut/tracing/opentelemetry/instrument/http/client/OpenTelemetryClientFilter.java

...ava/io/micronaut/tracing/opentelemetry/instrument/http/server/OpenTelemetryServerFilter.java

...metry/src/main/java/io/micronaut/tracing/opentelemetry/instrument/util/TracingPublisher.java

...ronaut/tracing/opentelemetry/instrument/http/client/MicronautHttpClientTelemetryBuilder.java

dstepanov · 2022-05-23T12:12:41Z

tracing-opentelemetry/src/main/java/io/micronaut/tracing/DefaultOpenTelemetryFactory.java

+@Factory
+public class DefaultOpenTelemetryFactory {
+
+    private static final String SERVICE_NAME_KEY = "otel.service.name";


Can we have opentelemetry. instead of otel.? WDYT @graemerocher
I would also have a constant PREFIX.

The "otel" prefix comes from https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md and this factory overrides some defaults of this library.

Agree, I think if we want to reuse the original official OpenTelemetry configuration properties we should use otel as well. Maybe it will be nice to support the configuration of most of these properties directly in the application.yml file.

@donbeave We already support most of linked properties through application.yml

Fine with me

tracing-aws/src/main/java/io.micronaut.tracing/AwsOpenTelemetryFactory.java

...ronaut/tracing/opentelemetry/instrument/http/client/MicronautHttpClientTelemetryBuilder.java

…nto feature/opentelemetry

n0tl3ss · 2022-05-24T13:29:06Z

I tried to apply all of your suggestions @graemerocher @dstepanov. Can you please double check everything?

src/main/docs/guide/opentelemetry/http.html

...io/micronaut/tracing/opentelemetry/instrument/http/client/OpenTelemetryHttpClientConfig.java

...ronaut/tracing/opentelemetry/instrument/http/server/MicronautHttpServerAttributesGetter.java

...cronaut/tracing/opentelemetry/instrument/http/server/MicronautServerAttributesExtractor.java

...io/micronaut/tracing/opentelemetry/instrument/http/server/OpenTelemetryHttpServerConfig.java

...ava/io/micronaut/tracing/opentelemetry/instrument/http/server/OpenTelemetryServerFilter.java

...ronaut/tracing/opentelemetry/instrument/http/client/MicronautHttpClientTelemetryFactory.java

...ava/io/micronaut/tracing/opentelemetry/instrument/http/client/OpenTelemetryClientFilter.java

...ronaut/tracing/opentelemetry/instrument/http/server/MicronautHttpServerTelemetryFactory.java

...ava/io/micronaut/tracing/opentelemetry/instrument/http/server/OpenTelemetryServerFilter.java

dstepanov · 2022-05-25T08:42:09Z

tracing-opentelemetry/src/main/java/io/micronaut/tracing/DefaultOpenTelemetryFactory.java

+@Factory
+public class DefaultOpenTelemetryFactory {
+
+    private static final String SERVICE_NAME_KEY = "otel.service.name";


Fine with me

...metry/src/main/java/io/micronaut/tracing/opentelemetry/instrument/util/TracingPublisher.java

...telemetry/src/main/java/io/micronaut/tracing/opentelemetry/interceptor/TraceInterceptor.java

donbeave · 2022-05-25T12:22:34Z

gradle/libs.versions.toml

 opentracing = '0.33.0'
+managed-opentracing-grpc = '0.2.3'
+managed-protobuf = '0.8.17'


I think this does not need to be managed.

donbeave · 2022-05-25T12:37:58Z

...telemetry/src/main/java/io/micronaut/tracing/opentelemetry/interceptor/TraceInterceptor.java

+
+/**
+ * Implements tracing logic for <code>ContinueSpan</code> and <code>NewSpan</code>
+ * using the Open Tracing API.


Open Tracing API.

Is it a typo? Correct is OpenTelemetry API, right?

graemerocher · 2022-05-25T12:50:33Z

If nobody has any further objections this looks like a good initial start to me

burtbeckwith

graemerocher · 2022-05-26T15:14:10Z

if folks would like to try it out before final release a 4.2.0-SNAPSHOT version of this module is available on the snapshot repo. See https://docs.micronaut.io/latest/guide/#usingsnapshots

n0tl3ss added 3 commits March 31, 2022 16:05

Add tracing-opentelemetry

0b37c1e

Add tracing-opentelemetry

64eae73

Add tracing-opentelemetry

3dea544

n0tl3ss requested a review from graemerocher April 6, 2022 10:06

n0tl3ss added 2 commits April 6, 2022 12:23

Add tracing-opentelemetry

93aefc0

Add tracing-opentelemetry

f03ab3f

graemerocher mentioned this pull request Apr 8, 2022

Move from Open Tracing to Open Telemetry #7

Closed

6 tasks

donbeave mentioned this pull request Apr 9, 2022

Add tracing-opentelemetry #66

Closed

graemerocher requested changes Apr 12, 2022

View reviewed changes

Add support for opentelemetry annotations

3b5d915

donbeave suggested changes Apr 13, 2022

View reviewed changes

do opentelemetry instrumentation right way

4007dfb

n0tl3ss requested a review from graemerocher April 15, 2022 17:29

donbeave suggested changes Apr 17, 2022

View reviewed changes

n0tl3ss added 3 commits April 18, 2022 10:36

remove surplus classes

94c2da7

remove surplus classes

63b27c1

fix checkstyle

1829768

donbeave reviewed Apr 18, 2022

View reviewed changes

fix style and test

4b3ddcc

donbeave reviewed Apr 18, 2022

View reviewed changes

n0tl3ss added 6 commits April 18, 2022 19:01

add grpc test

a1ef159

verify that issue is inside WithSpan annotation

886dec1

WithSpan = NewSpan annotation

c53cd93

Test fix

3035d4b

Test fix

911cd0b

Test fix

bd87160

dstepanov requested changes May 23, 2022

View reviewed changes

graemerocher removed the request for review from burtbeckwith May 23, 2022 13:41

graemerocher changed the title ~~Feature/opentelemetry~~ Add support for OpenTelemetry May 23, 2022

melix and others added 4 commits May 23, 2022 18:08

Only disable binary compatibility reports for new modules

4ed7598

Accept binary breaking changes

cf51e5c

Merge remote-tracking branch 'origin/n0tl3ss-feature/opentelemetry' i…

697cea4

…nto feature/opentelemetry

code review improvements

5bbebc9

n0tl3ss requested review from graemerocher and dstepanov May 24, 2022 13:27

n0tl3ss requested a review from burtbeckwith May 24, 2022 14:22

code review improvements

e155181

graemerocher requested changes May 25, 2022

View reviewed changes

dstepanov requested changes May 25, 2022

View reviewed changes

code review improvements

d522685

n0tl3ss requested review from dstepanov and graemerocher May 25, 2022 11:53

graemerocher approved these changes May 25, 2022

View reviewed changes

donbeave reviewed May 25, 2022

View reviewed changes

dstepanov approved these changes May 25, 2022

View reviewed changes

graemerocher marked this pull request as ready for review May 25, 2022 12:50

code review improvements

ad294f1

donbeave approved these changes May 25, 2022

View reviewed changes

burtbeckwith approved these changes May 25, 2022

View reviewed changes

graemerocher merged commit 1d3545f into micronaut-projects:master May 26, 2022

donbeave mentioned this pull request Jun 3, 2022

Support for the Micronaut framework? open-telemetry/opentelemetry-java-instrumentation#3424

Closed

This was referenced Dec 23, 2022

Add @Around again to support @NewSpan and @ContinueSpan on interfaces #216

Closed

Add @around again to support @NewSpan and @ContinueSpan on interfaces #217

Merged

Add support for OpenTelemetry #65

Add support for OpenTelemetry #65

Conversation

n0tl3ss commented Apr 6, 2022 • edited Loading

This PR adds OpenTelemetry in the micronaut-tracing module.

Modules

Supported Features

Other

donbeave commented Apr 9, 2022

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

donbeave left a 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

donbeave left a 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

n0tl3ss commented Apr 18, 2022 • edited Loading

n0tl3ss commented Apr 18, 2022

donbeave commented Apr 18, 2022

donbeave Apr 18, 2022 • edited Loading

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

n0tl3ss commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graemerocher commented May 25, 2022

burtbeckwith left a comment

Choose a reason for hiding this comment

graemerocher commented May 26, 2022

n0tl3ss commented Apr 6, 2022 •

edited

Loading

This PR adds OpenTelemetry in the `micronaut-tracing` module.

n0tl3ss commented Apr 18, 2022 •

edited

Loading

donbeave Apr 18, 2022 •

edited

Loading