Add JDBI 3 plugin #1460

reftel · 2025-04-10T18:01:19Z

Adds a plugin for JDBI 3 (https://jdbi.org/ ) that sends spans via Brave for all SQL commands run. Tag names are chosen to match the old P6Spy instrumentation from the 5.x series.
Including both a Mocktio test and an integration test that uses H2. Not sure both are needed, as they test pretty much the same thing.

instrumentation/jdbi3/pom.xml

instrumentation/jdbi3/README.md

reftel · 2025-04-28T06:28:03Z

Rebased, bumped parent version, bumped jdbi version.

reta · 2025-04-29T12:29:39Z

@codefromthecrypt any concerns from you? thanks!

codefromthecrypt

I like the work, but I think we need to study a bit more other modules (or ask your favorite coding assistant to). Please use same conventions in coding and tests for the more recent similar code. you've made a good start though!

instrumentation/jdbi3/pom.xml

codefromthecrypt · 2025-04-30T04:20:54Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+            .addContextListener(new StatementContextListener() {
+                @Override
+                public void contextCreated(final StatementContext ctx) {
+                    String spanName = "jdbi." + ctx.describeJdbiStatementType();


hmm try to match the data policy with mysql. I don't think we want to put jdbi in the span name.. https://github.com/openzipkin/brave/blob/master/instrumentation/mysql8/src/main/java/brave/mysql8/TracingQueryInterceptor.java

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

codefromthecrypt · 2025-04-30T04:22:20Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+            this(tracing, false);
+    }
+
+    public Jdbi3BravePlugin(Tracing tracing, boolean includeBindVariables) {


if you look elsewhere you'll see types that have builders. Typically we don't add public ctors with options as it is constant maintenance. maybe find one to copy/paste find/replace

codefromthecrypt · 2025-04-30T04:23:48Z

instrumentation/jdbi3/src/test/java/brave/jdbi3/Jdbi3BravePluginIT.java

+import brave.handler.SpanHandler;
+import brave.propagation.TraceContext;
+
+class Jdbi3BravePluginIT {


try for naming conventions and other conventions in the project. IT prefix e.g. https://github.com/openzipkin/brave/blob/master/instrumentation/mysql8/src/test/java/brave/mysql8/ITTracingQueryInterceptor.java and also IT implies real DB. This might be a unit test? ack jdbc:h2:mem is grey area.

It´s the use of an actual (though in-process and in-memory) database, along with using the third-party libraries as-is (as opposed to stubbing them, or parts of them, out) that made me call it an IT, but if you disagree, I´ll gladly change it.

Ah well you can probably use the mysql container we use in the other but agree it is a minor point. Just make sure the tests are similar as possible. Cheers

I´ve changed the tests over to more-or-less a copy of the tests from the mysql8 instrumentation. I´ll make a stab at getting the tests run with both H2 and MySQL, as a way to show that the instrumentation is not tied to any particular database.

Made the test abstract, with concrete implementations that test against H2 in-memory, MySQL configured as in the mysql8 tests, Postgres via the Zonky.io extension, and Sqlite via the xerial.org library.
In Junit 5.13, there will be a cleaner way of doing this, but it´s not released yet.

reftel · 2025-04-30T07:00:11Z

I like the work, but I think we need to study a bit more other modules (or ask your favorite coding assistant to). Please use same conventions in coding and tests for the more recent similar code. you've made a good start though!

Thank you for your comments! I believe I´ve addressed them all in 17321f1

instrumentation/jdbi3/pom.xml

codefromthecrypt

last feedback!

Unless there is a main code difference, please test only one DB as an IT. We should save test time for things that improve coverage.

Finally, it is easy to miss, but brave-bom needs an update both version and also to include this module https://github.com/openzipkin/brave/blob/master/brave-bom/pom.xml#L12

reftel · 2025-05-02T11:27:09Z

Thank you for your patience and prompt reviews! I hope all should be in order now.

Unless there is a main code difference, please test only one DB as an IT. We should save test time for things that improve coverage.

OK, dropped everything but the MySQL one.

Finally, it is easy to miss, but brave-bom needs an update both version and also to include this module https://github.com/openzipkin/brave/blob/master/brave-bom/pom.xml#L12

Done.

codefromthecrypt

I wanted to click approve, but really there are a lot of little things to clean up. clean up what you can. If you are out of time, then let's leave this PR open until it can be.

codefromthecrypt · 2025-05-02T22:19:52Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+        private boolean includeBindVariables = false;
+
+        public Builder(Tracing tracing) {
+            this.tracing = tracing;


Suggested change

this.tracing = tracing;

if (tracing == null) throw new NullPointerException("tracing == null");

this.tracing = tracing;

codefromthecrypt · 2025-05-02T22:20:01Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+        return null;
+    }
+
+    public static class Builder {


Suggested change

public static class Builder {

public static final class Builder {

Not sure the builder is still needed now that includeBindVariables is not there any longer, though.

codefromthecrypt · 2025-05-02T22:22:42Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+        }
+        URI uri;
+        try {
+            uri = new URI(connection.getMetaData().getURL().replaceAll("^jdbc:", ""));


normally I would challenge you to make this similar to mysql code which is more efficient than regex. However, you've had a lot of back and forth so we can leave that for someone else to refine. Generally it is better to look at how we do things as often there is some performance mania going on, though not always.

brave/instrumentation/mysql/src/main/java/brave/mysql/TracingStatementInterceptor.java

Lines 76 to 77 in ff40062

URI url = URI.create(connection.getMetaData().getURL().substring(5)); // strip "jdbc:"

String remoteServiceName = connection.getProperties().getProperty("zipkinServiceName");

Also added a simple cache, since a listener usually only ever sees one URL, and the remote service name for a given URL will always be the same.

codefromthecrypt · 2025-05-02T22:23:57Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+ * JDBI plugin that sends a trace span for every SQL command executed by Jdbi.
+ * Install by calling {@code jdbi.installPlugin(new Jdbi3BravePlugin(tracing));}
+ */
+public class Jdbi3BravePlugin extends JdbiPlugin.Singleton {


Suggested change

public class Jdbi3BravePlugin extends JdbiPlugin.Singleton {

public final class Jdbi3BravePlugin extends JdbiPlugin.Singleton {

codefromthecrypt · 2025-05-02T22:24:44Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+            return this;
+        }
+
+        public Jdbi3BravePlugin build() {


always return the most narrow type you can that works. Either plugin or plugin.singleton depending on what you think here. This is likely a place where since mysql plugin was made so long ago we didn't make do this.

Suggested change

public Jdbi3BravePlugin build() {

public JdbiPlugin.Singleton build() {

codefromthecrypt · 2025-05-02T22:32:33Z

instrumentation/jdbi3/src/test/java/brave/jdbi3/ITJdbi3BravePlugin.java

+
+        assertThat(spans)
+            .extracting(MutableSpan::name)
+            .containsExactly("Query");


I think you've misunderstood me. We shouldn't model differently than mysql. Please match your tests to what is here, as the span name shouldn't be "Query" that's not a SQL operator

brave/instrumentation/mysql8/src/test/java/brave/mysql8/ITTracingQueryInterceptor.java

Line 109 in ff40062

.containsExactly("select");

That´s the reason I had the jdbi. prefix originally - the name is the Jdbi description of the statement type, not the first word of the statement. Extracting the first word of the statement seems very brittle (e.g. it fails when using comment prefixes, which is generally is good practice), and since the full SQL is in the sql.query tag, it is also redundant. Using the Jdbi description of the type of statement (e.g. Query, Update, or Call) seems to me to be a better way.
I can change it if you insist, but I do think it would result in a worse implementation.

codefromthecrypt · 2025-05-02T22:33:11Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+
+    @Override
+    public void customizeJdbi(Jdbi jdbi) {
+        if (tracing == null) {


remove tracing == null here to reduce branching, as it is a bug to have null here and we can guard against it.

codefromthecrypt · 2025-05-02T22:34:10Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+ * Instrumentation for JDBI 3.
+ * <p>
+ * JDBI plugin that sends a trace span for every SQL command executed by Jdbi.
+ * Install by calling {@code jdbi.installPlugin(new Jdbi3BravePlugin(tracing));}


this drifted, also check your README

codefromthecrypt · 2025-05-02T22:37:03Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+/**
+ * Instrumentation for JDBI 3.
+ * <p>
+ * JDBI plugin that sends a trace span for every SQL command executed by Jdbi.


This sounds redundant. A lot of code here has had a lot of review, so you can avoid back and forth by copying it first, then revising. For example, this says what (a couple times) not why. e.g. if you look at the reference I mentioned, the javadoc says this A MySQL query interceptor that will report to Zipkin how long each query takes So the why is about the query. Also, we don't need to get specific to the version as that's already in the package name.

https://github.com/openzipkin/brave/blob/master/instrumentation/mysql8/src/main/java/brave/mysql8/TracingQueryInterceptor.java#L23-L31

codefromthecrypt · 2025-05-02T22:39:31Z

instrumentation/jdbi3/pom.xml

+
+    <main.basedir>${project.basedir}/../..</main.basedir>
+
+    <embedded-postgres.version>2.1.0</embedded-postgres.version>


I think there are a number of unused versions here. Also make sure you aren't declaring a version already declared elsewhere. I think you might be on mysql-connector-java.version. If that version isn't in the top-level pom due to lack of re-use, I would extract the one from mysql8 up to the top-level pom, then use the same variable name here.

When we don't take care to match other instrumentation, it leaves little bombs for maintainers later. For example, @reta and I have been dealing with versions for many years on code that sometimes was only polished once by the initial author (the original PR). This is why it is really ideal to look at things mentioned for review and look closely, as others are likely going to maintain it for you, and this is a perpetual task. Since it is, going the same way as other code lightens the load.

codefromthecrypt · 2025-05-02T22:48:57Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/Jdbi3BravePlugin.java

+            return;
+        }
+        jdbi.getConfig(SqlStatements.class)
+            .addContextListener(new StatementContextListener() {


one last thing is that this code would be easier to read and more conventional extracted to a package-private type (TracingStatementContextListener) and then used here. Another way to reduce ceremony is to switch the whole type to TracingStatementContextListener similar to this https://github.com/openzipkin/brave/blob/master/instrumentation/okhttp3/src/main/java/brave/okhttp3/TracingCallFactory.java (keeping the builder if you decide to keep the toggle).

This allows people to configure JDBI however they want including via a plugin and keeps the complexity of this code to a minimal. In other words, the actual code is the context listener, plugin is just one way to configure it. By limiting to plugin we add more harder to test code here, to save someone from doing jdbi.getConfig(SqlStatements.class) .addContextListener(our_listener) despite that being how all the other instrumentation work (direct integration where possible)

Making this comment as on the mind, but like I said you can feel free to stop refactoring if you have run out of time. If I were being strict like years ago I would have insisted on it.

instrumentation/jdbi3/src/main/java/brave/jdbi3/BraveStatementContextListener.java

instrumentation/jdbi3/src/test/java/brave/jdbi3/BraveStatementContextListenerTest.java

reta · 2025-05-08T21:33:26Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/RemoteServiceNameResolver.java

+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class RemoteServiceNameResolver {


Super minor, I think we shouldn't expose this class publicly:

Suggested change

public class RemoteServiceNameResolver {

final class RemoteServiceNameResolver {

reta · 2025-05-08T21:34:00Z

instrumentation/jdbi3/src/main/java/brave/jdbi3/BraveStatementContextListener.java

+        });
+    }
+
+    String getRemoteServiceName(StatementContext ctx) {


👍 thank you

reta · 2025-05-08T21:35:47Z

@codefromthecrypt mind taking another look? thank you!

codefromthecrypt

sure I'll do a small amount of polishing post merge. good work!

@reftel

This converts jdbi3 instrumentation to be more like MongoDB in implementation: configured by instance, not static like mysql connection driver. It also pares back the data policy to be the same as MySQL: this allows us a base case to move forward with. Finally, it switches implementation to a StatementLogger, so that exceptions are visible. Infrastructure wise, this re-uses the more recent style from instrumentation here, including docker for the IT. This is a follow-up to #1460 before we release a version, so API breaks are not an issue, yet. Thanks to @reftel for the initial work on this, and @reta on the review of that. I did my best in RATIONALE.md here to explain why the changes are important. --------- Signed-off-by: Adrian Cole <[email protected]>

reftel force-pushed the feature/jdbi_instrumentation branch from f756d98 to 969af12 Compare April 11, 2025 06:56

reta reviewed Apr 27, 2025

View reviewed changes

instrumentation/jdbi3/pom.xml Outdated Show resolved Hide resolved

reta reviewed Apr 27, 2025

View reviewed changes

instrumentation/jdbi3/README.md Outdated Show resolved Hide resolved

reftel force-pushed the feature/jdbi_instrumentation branch from 969af12 to 646c901 Compare April 28, 2025 06:26

reftel force-pushed the feature/jdbi_instrumentation branch from 646c901 to ccc66d9 Compare April 29, 2025 06:11

reta approved these changes Apr 29, 2025

View reviewed changes

reta requested a review from codefromthecrypt April 29, 2025 12:29

codefromthecrypt reviewed Apr 30, 2025

View reviewed changes

reftel force-pushed the feature/jdbi_instrumentation branch from ccc66d9 to 17321f1 Compare April 30, 2025 06:59

reta reviewed Apr 30, 2025

View reviewed changes

instrumentation/jdbi3/pom.xml Outdated Show resolved Hide resolved

reftel force-pushed the feature/jdbi_instrumentation branch from 17321f1 to ae0492c Compare May 2, 2025 08:21

codefromthecrypt reviewed May 2, 2025

View reviewed changes

6.3.0

24e5d41

reftel force-pushed the feature/jdbi_instrumentation branch from ae0492c to f5fd953 Compare May 2, 2025 11:17

codefromthecrypt reviewed May 2, 2025

View reviewed changes

reftel force-pushed the feature/jdbi_instrumentation branch from f5fd953 to 65f563f Compare May 5, 2025 07:05

reta reviewed May 6, 2025

View reviewed changes

instrumentation/jdbi3/src/main/java/brave/jdbi3/BraveStatementContextListener.java Outdated Show resolved Hide resolved

reftel force-pushed the feature/jdbi_instrumentation branch from 65f563f to 2ecc9d4 Compare May 6, 2025 06:38

reta reviewed May 7, 2025

View reviewed changes

instrumentation/jdbi3/src/main/java/brave/jdbi3/BraveStatementContextListener.java Outdated Show resolved Hide resolved

reta reviewed May 7, 2025

View reviewed changes

instrumentation/jdbi3/src/main/java/brave/jdbi3/BraveStatementContextListener.java Outdated Show resolved Hide resolved

reta reviewed May 7, 2025

View reviewed changes

instrumentation/jdbi3/src/main/java/brave/jdbi3/BraveStatementContextListener.java Outdated Show resolved Hide resolved

reftel force-pushed the feature/jdbi_instrumentation branch from 2ecc9d4 to f803bb1 Compare May 8, 2025 06:17

reta reviewed May 8, 2025

View reviewed changes

instrumentation/jdbi3/src/test/java/brave/jdbi3/BraveStatementContextListenerTest.java Outdated Show resolved Hide resolved

reftel force-pushed the feature/jdbi_instrumentation branch from f803bb1 to 9f9842b Compare May 8, 2025 11:47

Add JDBI 3 plugin

cb6d68f

reftel force-pushed the feature/jdbi_instrumentation branch from 9f9842b to cb6d68f Compare May 8, 2025 12:00

reta reviewed May 8, 2025

View reviewed changes

codefromthecrypt approved these changes May 27, 2025

View reviewed changes

codefromthecrypt merged commit 0ad3f6d into openzipkin:master May 27, 2025
4 checks passed

codefromthecrypt mentioned this pull request May 28, 2025

jdbi3: converts to StatementLogger and follows existing style #1464

Merged

	this.tracing = tracing;
	if (tracing == null) throw new NullPointerException("tracing == null");
	this.tracing = tracing;

	public static class Builder {
	public static final class Builder {

	URI url = URI.create(connection.getMetaData().getURL().substring(5)); // strip "jdbc:"
	String remoteServiceName = connection.getProperties().getProperty("zipkinServiceName");

	public class Jdbi3BravePlugin extends JdbiPlugin.Singleton {
	public final class Jdbi3BravePlugin extends JdbiPlugin.Singleton {

	public Jdbi3BravePlugin build() {
	public JdbiPlugin.Singleton build() {


		<main.basedir>${project.basedir}/../..</main.basedir>

		<embedded-postgres.version>2.1.0</embedded-postgres.version>

	public class RemoteServiceNameResolver {
	final class RemoteServiceNameResolver {

Add JDBI 3 plugin #1460

Add JDBI 3 plugin #1460

Uh oh!

Conversation

reftel commented Apr 10, 2025

Uh oh!

Uh oh!

Uh oh!

reftel commented Apr 28, 2025

Uh oh!

reta commented Apr 29, 2025

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reftel May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reftel commented Apr 30, 2025

Uh oh!

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

reftel commented May 2, 2025

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reftel May 2, 2025 •

edited

Loading