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 JdbcTelemetry and JdbcTelemetryBuilder #9685

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

hannahchan
Copy link
Contributor

We're currently using the JDBC library instrumentation in our application and unlike the agent instrumentation, it doesn't have an option to turn off the noisy getConnection spans. In the agent there is an option to disable jdbc-datasource and leave on jdbc.

This PR attempts to solve this by introducing an OpenTelemetryDataSourceBuilder in the library instrumentation so that consumers can through code, disable these spans.

@hannahchan hannahchan requested a review from a team as a code owner October 15, 2023 22:31
@laurit
Copy link
Contributor

laurit commented Oct 16, 2023

@hannahchan Have a look at a few of our existing library instrumentations, for example https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/cassandra/cassandra-4.4/library/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_4 Most of them follow the same style, there is a class called XxxTelemetry that have methods create, that produces an instance with default configuration, and builder that returns XxxTelemetryBuilder. The telemetry class has a method wrap (different instrumentations use different names for this method) that allows wrapping the existing instance to create a telemetry producing instance. WDYT about following the same style? As for the existing constructors in OpenTelemetryDataSource you can mark them as deprecated.

@hannahchan hannahchan force-pushed the OpenTelemetryDataSourceBuilder branch from 02c1bab to dd44b96 Compare October 16, 2023 12:44
@hannahchan
Copy link
Contributor Author

@laurit

Thanks for pointing out the examples. I've refactored my PR to what I think is following the pattern you pointed out. The only thing I'm not sure about is the name of the class I've chosen and am happy to take suggestions here. They are currently name JdbcTelemetry and JdbcTelemetryBuilder.

I'm ready for any feedback.

@hannahchan hannahchan force-pushed the OpenTelemetryDataSourceBuilder branch from dd44b96 to 1fb452d Compare October 16, 2023 12:54
@hannahchan
Copy link
Contributor Author

It looks like the build it broken and I don't know why. Is someone able to advise?

@laurit
Copy link
Contributor

laurit commented Oct 17, 2023

@hannahchan

It looks like the build it broken and I don't know why. Is someone able to advise?

We run tests agains latest version of instrumented libraries. When there is a new version sometimes our tests break, these failures don't prevent merging of the pull requests.

@hannahchan hannahchan changed the title Add OpenTelemetryDataSourceBuilder Add JdbcTelemetry and JdbcTelemetryBuilder Oct 17, 2023
@trask trask merged commit 2a554bd into open-telemetry:main Oct 17, 2023
46 of 47 checks passed
@hannahchan hannahchan deleted the OpenTelemetryDataSourceBuilder branch October 17, 2023 20:16
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

4 participants