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.

Sign up for GitHub

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

Jump to bottom

Allow JDBC autoinstrumentation to use a custom OpenTelemetry instance to be more DI (e.g. Spring Boot) friendly #7697

Merged
merged 6 commits into from
Feb 8, 2023
Merged

Allow JDBC autoinstrumentation to use a custom OpenTelemetry instance to be more DI (e.g. Spring Boot) friendly #7697

merged 6 commits into from
Feb 8, 2023

Conversation

FranPregernik
Copy link
Contributor

Related to issue #7677

@FranPregernik FranPregernik requested a review from a team January 31, 2023 17:10
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 31, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: FranPregernik / name: Fran Pregernik (a33abc7)

@FranPregernik FranPregernik changed the title 7677 Allow JDBC autoinstrumentation to use a custom OpenTelemetry instance to be more DI (e.g. Spring Boot) friendly Jan 31, 2023
jkwatson
jkwatson reviewed Feb 1, 2023
View reviewed changes
.../src/main/java/io/opentelemetry/instrumentation/jdbc/datasource/OpenTelemetryDataSource.java
*/
public OpenTelemetryDataSource(DataSource delegate) {
public OpenTelemetryDataSource(DataSource delegate, OpenTelemetry openTelemetry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏽 👍🏽 👍🏽 👍🏽 👍🏽 👍🏽 👍🏽 👍🏽 👍🏽

@mateuszrzeszutek
Copy link
Member

@FranPregernik looks like your commits authored under a different id from your GitHub id, and the EasyCLA check can't verify if you've signed the CLA because of this. Can you set the author field to your GH id and rebase?

@mateuszrzeszutek mateuszrzeszutek linked an issue Feb 1, 2023 that may be closed by this pull request
JDBC autoinstrumentation uses a static OpenTelemetry instance and is not DI (Spring Boot) friendly #7677
Closed
@FranPregernik
Allow custom (non-global) OpenTelemetry instance propagation
8d2c676 
Propagates a custom OpenTelemetry instance through the datasource, connection and statement(s) wrappers.
The OpenTelemetryDriver uses an OpenTelemetry instance provided by the GlobalOpenTelemetry.get() static method.
mateuszrzeszutek
mateuszrzeszutek reviewed Feb 3, 2023
View reviewed changes
...ry/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/OpenTelemetryConnection.java Outdated Show resolved Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcInstrumenterFactory.java Outdated Show resolved Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcInstrumenterFactory.java Show resolved Hide resolved
...ary/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/OpenTelemetryStatement.java Outdated Show resolved Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/jdbc/datasource/OpenTelemetryDataSource.java Outdated Show resolved Hide resolved
...va/io/opentelemetry/javaagent/instrumentation/jdbc/datasource/DataSourceInstrumentation.java
@@ -46,7 +48,8 @@ public static void start(
return;
}

context = instrumenter().start(parentContext, ds);
Copy link
Member

Choose a reason for hiding this comment

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

Oh, it turns out the javaagent instrumentation was using the library singletons class. Can you add a dataSourceInstrumenter() method to the JdbcSingletons class? You should be able to reuse the DataSourceInstrumenterFactory class you've just added.

Copy link
Contributor Author

@FranPregernik FranPregernik Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure about JdbcSingletons? Because It has been renamed to JdbcInstrumenterFactory.
I added it for now to the DataSourceInstrumentation class but I can move it to the JdbcSingletons class.

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yep - the javaagent instrumentation has its own, separate JdbcSingletons class.
By convention, we always create static methods for each instrumenter - usually it's just instrumenter(), but in this case, since we have >1 instrumenter here it'd be dataSourceInstrumenter()

mateuszrzeszutek
mateuszrzeszutek reviewed Feb 7, 2023
View reviewed changes
.../javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcSingletons.java Outdated Show resolved Hide resolved
...dbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcSingletons.java
public static Instrumenter<DbRequest, Void> instrumenter() {
return INSTRUMENTER;
}
public static final Instrumenter<DbRequest, Void> STATEMENT_INSTRUMENTER =
Copy link
Member

Choose a reason for hiding this comment

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

The other comment about JdbcSingletons was about the javaagent JdbcSingletons class (yeah, we have multiple classes with the same name in this project 😅 ); this one should be safe to remove.

.../javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcSingletons.java Show resolved Hide resolved
...ary/src/test/groovy/io/opentelemetry/instrumentation/jdbc/OpenTelemetryConnectionTest.groovy Outdated Show resolved Hide resolved
mateuszrzeszutek
mateuszrzeszutek approved these changes Feb 8, 2023
View reviewed changes
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks for bearing with all my comments @FranPregernik !

@mateuszrzeszutek mateuszrzeszutek merged commit 1d3752f into open-telemetry:main Feb 8, 2023
@FranPregernik FranPregernik deleted the 7677 branch February 9, 2023 09:16
@FranPregernik FranPregernik mentioned this pull request Feb 9, 2023
Closed
@FranPregernik
Copy link
Contributor Author

Thank you for guiding me.

One tidbit of info... seems I am not the only one who needed this.
@jimbogithub over on spring-projects/spring-boot#34023 mentioned it as well.

https://stackoverflow.com/questions/75293292/spring-micrometer-opentelemetry-exporter-otlp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkwatson jkwatson jkwatson left review comments

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JDBC autoinstrumentation uses a static OpenTelemetry instance and is not DI (Spring Boot) friendly
3 participants
@FranPregernik @mateuszrzeszutek @jkwatson