Skip to content
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 an example of otel instrumentation usage #660

Merged

Conversation

grouzen
Copy link
Contributor

@grouzen grouzen commented Feb 28, 2023

It is a part of: #654

@grouzen grouzen force-pushed the opentelemetry-autoinstrumentation-example branch from c494bf4 to cdbc1b5 Compare February 28, 2023 13:51
@dmytr
Copy link
Contributor

dmytr commented May 6, 2023

Hi @grouzen,

Automatic instrumentation example should work with this patch: example.patch.txt (had to add .txt so GH would allow me to upload it).

Basically everything seemed to be correct, only that OTEL doesn't have instrumentation for async-http-client. On JVM 11+ it's possible to use Java's built-in async HTTP client as sttp backend. And Jaeger setup could be simplified.

I've got both client and server spans:

image

Update:

Actually there is instrumentation for async-http-client: https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/async-http-client. I'm not sure why it didn't work with this backend...

@grouzen
Copy link
Contributor Author

grouzen commented May 6, 2023

Thanks a lot @dmytr ! I'll try to run it with your patch and let you know about the results.

@grouzen grouzen changed the title Add an example how to use otel instrumentation Add an example of otel instrumentation usage May 6, 2023
@grouzen
Copy link
Contributor Author

grouzen commented May 18, 2023

@dmytr Hey! I finally managed to try your solution, and it works great!

Unfortunately, I encountered another issue with manipulating traces manually via Tracing and ContextStorage.openTelemetryContext. It seems that StorageContext.openTelemetryContext fails to restore context properly when combining auto instrumentation and manual span management.
For example, we expect https://github.com/zio/zio-telemetry/pull/660/files#diff-05c0c9e710ad79f69a2b78edfb2428484268088d4ae606771a3c6167d1a32e54R33 piece of code to add the event and some attributes to newly created internal child span, but it doesn't.

Also, I've tried to use openTelemetryContext instead of fiberRef in TracingTest. Running it by sbt -J-javaagent:$OTEL_AGENT_PATH test makes all tests fail with errors like:

Exception in thread "zio-fiber-542" java.lang.RuntimeException: Current context is root!

@dmytr
Copy link
Contributor

dmytr commented May 20, 2023

Hi @grouzen,

The problem with the current version of this example seems to be that it creates a separate tracer which is not linked to all the machinery provided by OTEL JVM agent.

Tracer has to be obtained using GlobalOpenTelemetry.getTracer(), which is instrumented by the agent.

Theoretically, manually created tracer should work as well, as it should be using the same Context storage. I'm not sure why it doesn't 🤷‍♂️

Also there is no need to extract trace in HTTP handler as this is handled by the agent.

And there is no need to add any extra dependencies to opentelemetryInstrumentationExample as everything is already provided by the agent. We only need to add a compatible Tapir backend.

With this patch everything works:

trace

Regarding test, I'd expect that it's the same problem - it uses a separate tracer.

@grouzen
Copy link
Contributor Author

grouzen commented May 20, 2023

@dmytr Thanks! You saved me a lot of hair on my head, GlobalOpenTelemetry.getTracer() works like a charm!

Theoretically, manually created tracer should work as well, as it should be using the same Context storage. I'm not sure why it doesn't man_shrugging

It is a good topic to investigate.

Also there is no need to extract trace in HTTP handler as this is handled by the agent.

Yeah, I know. My goal was to show how to manually create child spans from those provided by instrumentation.

And there is no need to add any extra dependencies to opentelemetryInstrumentationExample as everything is already provided by the agent. We only need to add a compatible Tapir backend.

Right!

Now I have the correct span hierarchy:
Screenshot 2023-05-20 at 18-12-05 Jaeger UI

@grouzen grouzen force-pushed the opentelemetry-autoinstrumentation-example branch from 8aaec3c to f871fb7 Compare May 20, 2023 17:35
@grouzen grouzen marked this pull request as ready for review May 20, 2023 17:41
@grouzen grouzen requested a review from a team as a code owner May 20, 2023 17:41
@grouzen grouzen force-pushed the opentelemetry-autoinstrumentation-example branch 2 times, most recently from 6f8587c to fd76ba7 Compare May 20, 2023 18:02
@grouzen grouzen force-pushed the opentelemetry-autoinstrumentation-example branch from fd76ba7 to 490a90f Compare May 20, 2023 18:06
@grouzen grouzen merged commit 9cd34a2 into zio:series/2.x May 20, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants