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

Support OpenTelemetry @WithSpan in CDI #21013

Merged
merged 1 commit into from
Nov 17, 2021

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Oct 26, 2021

A few considerations:

  • The OpenTelemetry instrumentation BOM includes a lot of integration stuff with other libraries we use. We didn't try these and may cause some confusion to someone including them in their project via our BOM.
  • We probably require some more test, but wanted to push just to start the discussion on the implementation.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/tracing labels Oct 26, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 26, 2021

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

Failing Jobs - Building 6605fb8

Status Name Step Failures Logs Raw logs
Native Tests - Messaging2 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Messaging2 #

- Failing: integration-tests/reactive-messaging-rabbitmq 

📦 integration-tests/reactive-messaging-rabbitmq

io.quarkus.it.rabbitmq.RabbitMQConnectorIT.test - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a io.quarkus.it.rabbitmq.RabbitMQConnectorTest expected: <6> but was: <0> within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 27, 2021

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

Failing Jobs - Building adcbc51

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/reactive-messaging-rabbitmq 

📦 integration-tests/reactive-messaging-rabbitmq

io.quarkus.it.rabbitmq.RabbitMQConnectorTest.test line 24 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a io.quarkus.it.rabbitmq.RabbitMQConnectorTest expected: <6> but was: <3> within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 4, 2021

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

Failing Jobs - Building 735f3dd

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 17

Copy link
Member

@kenfinnigan kenfinnigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there benefits of using the Instrumentor libraries vs interacting with the tracer directly?

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 5, 2021

Failing Jobs - Building 62a0a34

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
Native Tests - Windows - hibernate-validator Build Failures Logs Raw logs

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: docs 

📦 docs

Java heap space


⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator 

📦 integration-tests/hibernate-validator

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

@radcortez
Copy link
Member Author

Are there benefits of using the Instrumentor libraries vs interacting with the tracer directly?

The benefit here is that we only need to configure the Instrumenter and define where to start and stop the Span and the Instrumenter will take care of setting the tags, picking and propagating other tags etc. They can also be enabled or disabled. When I was looking into the OTel SDK this seems to be the proper way to implement manual instrumentation: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/using-instrumenter-api.md

I also wrote a prototype in SR OpenTelemetry to try it out first: https://github.com/smallrye/smallrye-opentelemetry/tree/main/implementation

My plan was to refactor our manual Tracer code in Vert.x and use the Instrumenter instead.

@kenfinnigan
Copy link
Member

My understanding was manual instrumentation was done with https://github.com/open-telemetry/opentelemetry-java, the OTeL Java SDK directly, and not through the Instrumentor classes in the agent repository.

@radcortez
Copy link
Member Author

I believe that the Instrumeter APIs were not mature enough in the past. They seem to be better now and the way to go. For instance if you look into their Spring Web implementation they do it use the Instrumenter API: https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/spring/spring-web-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/web

@ebullient
Copy link
Member

Right.. but that is instrumentation agent... not something like quarkus (that would run outside/above/alongside the agent). Granted, we're kind of halfway in-between being an agent and not..

@kenfinnigan
Copy link
Member

I would recommend not to use auto instrumentation APIs in non-agent usage in Quarkus, but it's up to you @radcortez

@radcortez
Copy link
Member Author

Right.. but that is instrumentation agent... not something like quarkus (that would run outside/above/alongside the agent). Granted, we're kind of halfway in-between being an agent and not.

My understanding is that the Instrumenter API is intended to be used as an encapsulator to all the logic of gathering Telemetry data (either agent or not). The added benefit is that you can easily register the appropriate extractors and do not have to manually reimplement the specification.

If you take the example I gave from the Spring implementation it is implemented using a REST interceptor: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/spring/spring-web-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/web/RestTemplateInterceptor.java.

There are other modules where the API is used as a simple library. Actually it follows a pattern where if a module is named library inside the OTel implementation it is named this way. Here are a few other examples:

If we are not sure we can certainly clarify in the OTel call if the API is intended to be used this way.

@ebullient
Copy link
Member

I think my question is whether or not they are ready for the instrumentation APIs to be used this way outside of that repo (i.e. do they think it is stable).

Then we (quarkus) have another question to answer differently, which is whether or not any of those libraries could/should work as-is.

@radcortez
Copy link
Member Author

I think my question is whether or not they are ready for the instrumentation APIs to be used this way outside of that repo (i.e. do they think it is stable).

That is something we can confirm / clarify in one of the OTel calls.

Then we (quarkus) have another question to answer differently, which is whether or not any of those libraries could/should work as-is.

I believe we should follow the same rule as any other library. If it is not a Quarkus supported extension, then you are on your own. Agents are a different story.

@radcortez
Copy link
Member Author

In the last OTel call, I've confirmed with @kenfinnigan that the Instrumenter API is also intended to be used for libraries.

@kenfinnigan are you in favour to proceed with this approach?

@kenfinnigan
Copy link
Member

Entirely up to you now @radcortez!

Only comment I'd make is whether it also requires revisiting the existing implementation to utilize the same API for consistency.

@radcortez
Copy link
Member Author

Only comment I'd make is whether it also requires revisiting the existing implementation to utilize the same API for consistency.

Yes, I plan to do it as well for Vert.x in another PR. Thanks!

@radcortez radcortez merged commit 21d2725 into quarkusio:main Nov 17, 2021
@quarkus-bot quarkus-bot bot added this to the 2.6 - main milestone Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/dependencies Pull requests that update a dependency file area/tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants