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

Make the JDBC driver config work with the OTel starter #9625

Merged

Conversation

jeanbisutti
Copy link
Member

No description provided.

@jeanbisutti jeanbisutti requested a review from a team October 8, 2023 20:22

tasks.test {
useJUnitPlatform()
setForkEvery(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

I have tested with @DirtiesContext without success (telemetry missing for one test). Perhaps this topic could be revisited later with the help of #8962 and #8963.

@jeanbisutti
Copy link
Member Author

@open-telemetry/java-instrumentation-maintainers and @open-telemetry/java-instrumentation-approvers, would you have other comments?

@jeanbisutti
Copy link
Member Author

I have tried to do an implementation in the same way as for logging with an ApplicationListener, but is does not work.

instrumentation/jdbc/library/README.md Outdated Show resolved Hide resolved
Comment on lines +23 to +26
@Bean
OpenTelemetryInjector injectOtelIntoJdbcDriver() {
return openTelemetry -> OpenTelemetryDriver.install(openTelemetry);
}
Copy link
Member

Choose a reason for hiding this comment

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

can the approach used for logging work here?

@Bean
ApplicationListener<ApplicationReadyEvent> log4jOtelAppenderInitializer(
OpenTelemetry openTelemetry) {
return event -> {
io.opentelemetry.instrumentation.log4j.appender.v2_17.OpenTelemetryAppender.install(
openTelemetry);
};
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the first thing I have tried. It does not work.

@trask trask added this to the v1.32.0 milestone Oct 20, 2023
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Copy link

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

I've tested it with Quarkus with no side effects. Thanks @jeanbisutti !

@jeanbisutti
Copy link
Member Author

I've tested it with Quarkus with no side effects. Thanks @jeanbisutti !

Thank you @brunobat for having tested that these changes won't break things on Quarkus!

@@ -48,6 +52,8 @@ public final class OpenTelemetryDriver implements Driver {
// visible for testing
static final OpenTelemetryDriver INSTANCE = new OpenTelemetryDriver();

private OpenTelemetry openTelemetry = OpenTelemetry.noop();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be volatile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not ure about all the use cases of the OpenTelemetryDriver. So, I have added volatile to be more confident. Thank you. I have noticed that volatile is not used for logging here and here, but it's perhaps less problematic.

Copy link
Member

Choose a reason for hiding this comment

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

I have noticed that volatile is not used for logging here and here, but it's perhaps less problematic.

ideally those should be volatile also since could be accessed across different threads, would you mind sending a separate PR to update them?

@trask trask merged commit e6ec4f5 into open-telemetry:main Oct 23, 2023
47 checks passed
Abhishekkr3003 pushed a commit to Abhishekkr3003/opentelemetry-java-instrumentation that referenced this pull request Nov 7, 2023
…y#9625)

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
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.

5 participants