Skip to content

Added instrumentation for transaction commit/rollback in jdbc #13709

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

Merged
merged 15 commits into from
May 14, 2025
Merged

Added instrumentation for transaction commit/rollback in jdbc #13709

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
83795a3
feat: contribute for #10381
stillya Apr 14, 2025
140583e
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
stillya Apr 14, 2025
54fee2e
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
stillya Apr 15, 2025
fb74aa2
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
stillya Apr 22, 2025
fb22764
Add new transaction instrumeter
stillya Apr 27, 2025
2de16b2
polish commit
stillya Apr 27, 2025
12b544b
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
stillya Apr 27, 2025
510c1b0
polish commit
stillya Apr 27, 2025
ac5ea54
review fixes
stillya Apr 28, 2025
114ccac
reduce the amount of code
laurit May 6, 2025
1336ab2
Merge branch 'main' into feat/jdbc-txn-instrumentation
laurit May 6, 2025
5a4a0d2
add db.operation attribute to transaction spans
laurit May 6, 2025
7bccecd
disable by default
laurit May 6, 2025
a928021
Merge branch 'main' into feat/jdbc-txn-instrumentation
laurit May 8, 2025
542f4b5
spring configuration, metadata
laurit May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions instrumentation-api-incubator/src/main/jflex/SqlSanitizer.jflex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,12 @@ WHITESPACE = [ \t\r\n]+
}
}

private class Commit extends Operation {
}

private class Rollback extends Operation {
}

private class Create extends DdlOperation {
}

Expand Down Expand Up @@ -485,6 +491,20 @@ WHITESPACE = [ \t\r\n]+
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}
"COMMIT" {
if (!insideComment) {
setOperation(new Commit());
}
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}
"ROLLBACK" {
if (!insideComment) {
setOperation(new Rollback());
}
appendCurrentFragment();
if (isOverLimit()) return YYEOF;
}

{COMMA} {
if (!insideComment && !extractionDone) {
Expand Down
28 changes: 28 additions & 0 deletions .../io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,17 @@ void checkDdlOperationStatementsAreOk(
assertThat(result.getMainIdentifier()).isEqualTo(expected.getMainIdentifier());
}

@ParameterizedTest
@ArgumentsSource(TransactionArgs.class)
void checkTransactionOperationStatementsAreOk(
String actual, Function<String, SqlStatementInfo> expectFunc) {
SqlStatementInfo result = SqlStatementSanitizer.create(true).sanitize(actual);
SqlStatementInfo expected = expectFunc.apply(actual);
assertThat(result.getFullStatement()).isEqualTo(expected.getFullStatement());
assertThat(result.getOperation()).isEqualTo(expected.getOperation());
assertThat(result.getMainIdentifier()).isEqualTo(expected.getMainIdentifier());
}

@Test
void lotsOfTicksDontCauseStackOverflowOrLongRuntimes() {
String s = "'";
Expand Down Expand Up @@ -427,4 +438,21 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
"CREATE PROCEDURE p AS SELECT * FROM table GO", expect("CREATE PROCEDURE", null)));
}
}

static class TransactionArgs implements ArgumentsProvider {
static Function<String, SqlStatementInfo> expect(String operation, String identifier) {
return sql -> SqlStatementInfo.create(sql, operation, identifier);
}

@Override
public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
return Stream.of(
Arguments.of("COMMIT", expect("COMMIT", null)),
Arguments.of("commit", expect("COMMIT", null)),
Copy link
Contributor

@laurit laurit Apr 16, 2025

Choose a reason for hiding this comment

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

might as well test some of
xa commit https://dev.mysql.com/doc/refman/8.4/en/xa-statements.html
commit work https://www.ibm.com/docs/en/informix-servers/14.10.0?topic=statements-commit-work-statement
commit tran https://learn.microsoft.com/en-us/sql/t-sql/language-elements/commit-transaction-transact-sql?view=sql-server-ver16
commit prepared https://www.postgresql.org/docs/current/sql-commit-prepared.html

Copy link
Contributor Author

@stillya stillya Apr 27, 2025

Choose a reason for hiding this comment

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

I dropped it - commit/rollback isn't part of the SQL statement parsing anymore.

Arguments.of("/*COMMIT*/", expect(null, null)),
Arguments.of("ROLLBACK", expect("ROLLBACK", null)),
Arguments.of("rollback", expect("ROLLBACK", null)),
Arguments.of("/*ROLLBACK*/", expect(null, null)));
}
}
}
7 changes: 4 additions & 3 deletions instrumentation/jdbc/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Settings for the JDBC instrumentation

| System property | Type | Default | Description |
|---------------------------------------------------------|---------|---------|----------------------------------------|
| `otel.instrumentation.jdbc.statement-sanitizer.enabled` | Boolean | `true` | Enables the DB statement sanitization. |
| System property | Type | Default | Description |
|---------------------------------------------------------|---------|---------|-------------------------------------------------------|
| `otel.instrumentation.jdbc.statement-sanitizer.enabled` | Boolean | `true` | Enables the DB statement sanitization. |
| `otel.instrumentation.jdbc.experimental.txn.enabled` | Boolean | `false` | Enables the experimental transaction instrumentation. |
Copy link
Contributor

@laurit laurit Apr 16, 2025

Choose a reason for hiding this comment

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

Replace txn with transaction. I think the description could be changed to indicate that this option enables creation of spans for COMMIT and ROLLBACK operations. The current description doesn't really reveal what this instrumentation does.

Copy link
Contributor Author

@stillya stillya Apr 27, 2025

Choose a reason for hiding this comment

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

Fixed.

6 changes: 6 additions & 0 deletions instrumentation/jdbc/javaagent/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,9 @@ tasks {
dependsOn(testSlickStableSemconv)
}
}

tasks {
withType<Test>().configureEach {
jvmArgs("-Dotel.instrumentation.jdbc.experimental.txn.enabled=true")
}
}
65 changes: 65 additions & 0 deletions .../main/java/io/opentelemetry/javaagent/instrumentation/jdbc/ConnectionInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,40 @@

package io.opentelemetry.javaagent.instrumentation.jdbc;

import static io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge.currentContext;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static io.opentelemetry.javaagent.instrumentation.jdbc.JdbcSingletons.statementInstrumenter;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
import static net.bytebuddy.matcher.ElementMatchers.returns;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
import static net.bytebuddy.matcher.ElementMatchers.takesNoArguments;

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.jdbc.internal.DbRequest;
import io.opentelemetry.instrumentation.jdbc.internal.JdbcData;
import io.opentelemetry.instrumentation.jdbc.internal.JdbcUtils;
import io.opentelemetry.javaagent.bootstrap.internal.AgentInstrumentationConfig;
import io.opentelemetry.javaagent.bootstrap.jdbc.DbInfo;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.util.Locale;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

public class ConnectionInstrumentation implements TypeInstrumentation {

private static final boolean TXN_ENABLED =
AgentInstrumentationConfig.get()
.getBoolean("otel.instrumentation.jdbc.experimental.txn.enabled", false);

@Override
public ElementMatcher<ClassLoader> classLoaderOptimization() {
return hasClassesNamed("java.sql.Connection");
Expand All @@ -40,6 +57,11 @@ public void transform(TypeTransformer transformer) {
// Also include CallableStatement, which is a sub type of PreparedStatement
.and(returns(implementsInterface(named("java.sql.PreparedStatement")))),
ConnectionInstrumentation.class.getName() + "$PrepareAdvice");
if (TXN_ENABLED) {
transformer.applyAdviceToMethod(
namedOneOf("commit", "rollback").and(takesNoArguments()).and(isPublic()),
ConnectionInstrumentation.class.getName() + "$TxnAdvice");
}
}

@SuppressWarnings("unused")
Expand All @@ -51,4 +73,47 @@ public static void addDbInfo(
JdbcData.preparedStatement.set(statement, sql);
}
}

@SuppressWarnings("unused")
public static class TxnAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static void onEnter(
@Advice.This Connection connection,
@Advice.Origin("#m") String methodName,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
Context parentContext = currentContext();
DbInfo dbInfo = null;
Connection realConnection = JdbcUtils.unwrapConnection(connection);
if (realConnection != null) {
dbInfo = JdbcUtils.extractDbInfo(realConnection);
}
if (dbInfo == null) {
return;
}
DbRequest request = DbRequest.create(dbInfo, methodName.toUpperCase(Locale.ROOT));

if (!statementInstrumenter().shouldStart(parentContext, request)) {
return;
}

request = DbRequest.create(request.getDbInfo(), methodName.toUpperCase(Locale.ROOT));
context = statementInstrumenter().start(parentContext, request);
scope = context.makeCurrent();
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void stopSpan(
@Advice.Thrown Throwable throwable,
@Advice.Local("otelRequest") DbRequest request,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
if (scope == null) {
return;
}
scope.close();
statementInstrumenter().end(context, request, null, throwable);
}
}
}
149 changes: 149 additions & 0 deletions ...st/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static io.opentelemetry.semconv.incubating.DbIncubatingAttributes.DB_OPERATION;
import static io.opentelemetry.semconv.incubating.DbIncubatingAttributes.DB_OPERATION_BATCH_SIZE;
import static io.opentelemetry.semconv.incubating.DbIncubatingAttributes.DB_OPERATION_NAME;
import static io.opentelemetry.semconv.incubating.DbIncubatingAttributes.DB_QUERY_TEXT;
import static io.opentelemetry.semconv.incubating.DbIncubatingAttributes.DB_SQL_TABLE;
import static io.opentelemetry.semconv.incubating.DbIncubatingAttributes.DB_STATEMENT;
import static io.opentelemetry.semconv.incubating.DbIncubatingAttributes.DB_SYSTEM;
Expand Down Expand Up @@ -1632,4 +1633,152 @@ void testPreparedBatch(String system, Connection connection, String username, St
DB_OPERATION_BATCH_SIZE,
emitStableDatabaseSemconv() ? 2L : null))));
}

@ParameterizedTest
@MethodSource("transactionOperationsStream")
void testCommitTransaction(String system, Connection connection, String username, String url)
throws SQLException {

String tableName = "TXN_COMMIT_TEST_" + system.toUpperCase(Locale.ROOT);
Statement createTable = connection.createStatement();
createTable.execute("CREATE TABLE " + tableName + " (id INTEGER not NULL, PRIMARY KEY ( id ))");
cleanup.deferCleanup(createTable);

boolean originalAutoCommit = connection.getAutoCommit();
connection.setAutoCommit(false);

testing.waitForTraces(1);
testing.clearData();

try {
Statement insertStatement = connection.createStatement();
cleanup.deferCleanup(insertStatement);

testing.runWithSpan(
"parent",
() -> {
insertStatement.executeUpdate("INSERT INTO " + tableName + " VALUES(1)");
connection.commit();
});

testing.waitAndAssertTraces(
trace ->
trace.hasSpansSatisfyingExactly(
span -> span.hasName("parent").hasKind(SpanKind.INTERNAL).hasNoParent(),
span ->
span.hasName("INSERT jdbcunittest." + tableName)
.hasKind(SpanKind.CLIENT)
.hasParent(trace.getSpan(0))
.hasAttributesSatisfyingExactly(
equalTo(maybeStable(DB_SYSTEM), maybeStableDbSystemName(system)),
equalTo(maybeStable(DB_NAME), dbNameLower),
equalTo(DB_USER, emitStableDatabaseSemconv() ? null : username),
equalTo(
DB_CONNECTION_STRING, emitStableDatabaseSemconv() ? null : url),
equalTo(
maybeStable(DB_STATEMENT),
"INSERT INTO " + tableName + " VALUES(?)"),
equalTo(maybeStable(DB_OPERATION), "INSERT"),
equalTo(maybeStable(DB_SQL_TABLE), tableName)),
span ->
span.hasName("COMMIT " + dbNameLower)
.hasKind(SpanKind.CLIENT)
.hasParent(trace.getSpan(0))
.hasAttributesSatisfyingExactly(
equalTo(maybeStable(DB_SYSTEM), maybeStableDbSystemName(system)),
equalTo(maybeStable(DB_NAME), dbNameLower),
equalTo(DB_USER, emitStableDatabaseSemconv() ? null : username),
equalTo(DB_STATEMENT, emitStableDatabaseSemconv() ? null : "COMMIT"),
equalTo(DB_QUERY_TEXT, emitStableDatabaseSemconv() ? "COMMIT" : null),
Copy link
Contributor

@laurit laurit Apr 16, 2025

Choose a reason for hiding this comment

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

imo you shouldn't fill the query, you don't really know what the jdbc driver executes for commit operation

Copy link
Contributor Author

@stillya stillya Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I considered it but there's an issue: the span name extractor uses the query text to compute the span name. I see three possible solutions:

  1. Add a new separate field to the request struct for the operation name.
  2. Add a special check in the JDBC attributes getter to specifically handle commit and rollback operations.
  3. Do nothing. Commit semantics differ across drivers, but since we add an attribute with the system information and consumers know which driver they’re using, this might be acceptable.

WDYT?

P.S. Also we could create new instrumenter but it's too much I think

Copy link
Contributor

@laurit laurit Apr 18, 2025

Choose a reason for hiding this comment

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

The description for db.query.text only mentions The database query being executed and some things about sanitization. As far as I can tell it does not allow for substituting the original query with another one.

Also we could create new instrumenter but it's too much I think

We sometimes do this for instrumentations that are not enabled by default. Then we can use setEnabled on the instrumenter to control whether it is enabled or not.

Copy link
Contributor Author

@stillya stillya Apr 27, 2025

Choose a reason for hiding this comment

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

Added a new transaction instrumenter. A bit more code, but it's consistent with the docs now. WDYT?

equalTo(
DB_CONNECTION_STRING, emitStableDatabaseSemconv() ? null : url),
equalTo(maybeStable(DB_OPERATION), "COMMIT"))));
} finally {
connection.setAutoCommit(originalAutoCommit);
}
}

@ParameterizedTest
@MethodSource("transactionOperationsStream")
void testRollbackTransaction(String system, Connection connection, String username, String url)
throws SQLException {

String tableName = "TXN_ROLLBACK_TEST_" + system.toUpperCase(Locale.ROOT);
Statement createTable = connection.createStatement();
createTable.execute("CREATE TABLE " + tableName + " (id INTEGER not NULL, PRIMARY KEY ( id ))");
cleanup.deferCleanup(createTable);

boolean originalAutoCommit = connection.getAutoCommit();
connection.setAutoCommit(false);

testing.waitForTraces(1);
testing.clearData();

try {
Statement insertStatement = connection.createStatement();
cleanup.deferCleanup(insertStatement);

testing.runWithSpan(
"parent",
() -> {
insertStatement.executeUpdate("INSERT INTO " + tableName + " VALUES(1)");
connection.rollback();
});

testing.waitAndAssertTraces(
trace ->
trace.hasSpansSatisfyingExactly(
span -> span.hasName("parent").hasKind(SpanKind.INTERNAL).hasNoParent(),
span ->
span.hasName("INSERT jdbcunittest." + tableName)
.hasKind(SpanKind.CLIENT)
.hasParent(trace.getSpan(0))
.hasAttributesSatisfyingExactly(
equalTo(maybeStable(DB_SYSTEM), maybeStableDbSystemName(system)),
equalTo(maybeStable(DB_NAME), dbNameLower),
equalTo(DB_USER, emitStableDatabaseSemconv() ? null : username),
equalTo(
DB_CONNECTION_STRING, emitStableDatabaseSemconv() ? null : url),
equalTo(
maybeStable(DB_STATEMENT),
"INSERT INTO " + tableName + " VALUES(?)"),
equalTo(maybeStable(DB_OPERATION), "INSERT"),
equalTo(maybeStable(DB_SQL_TABLE), tableName)),
span ->
span.hasName("ROLLBACK " + dbNameLower)
.hasKind(SpanKind.CLIENT)
.hasParent(trace.getSpan(0))
.hasAttributesSatisfyingExactly(
equalTo(maybeStable(DB_SYSTEM), maybeStableDbSystemName(system)),
equalTo(maybeStable(DB_NAME), dbNameLower),
equalTo(DB_USER, emitStableDatabaseSemconv() ? null : username),
equalTo(
DB_STATEMENT, emitStableDatabaseSemconv() ? null : "ROLLBACK"),
equalTo(
DB_QUERY_TEXT, emitStableDatabaseSemconv() ? "ROLLBACK" : null),
equalTo(
DB_CONNECTION_STRING, emitStableDatabaseSemconv() ? null : url),
equalTo(maybeStable(DB_OPERATION), "ROLLBACK"))));

Statement selectStatement = connection.createStatement();
cleanup.deferCleanup(selectStatement);
ResultSet resultSet = selectStatement.executeQuery("SELECT COUNT(*) FROM " + tableName);
resultSet.next();
assertThat(resultSet.getInt(1)).isEqualTo(0);
Copy link
Contributor

@laurit laurit Apr 28, 2025

Choose a reason for hiding this comment

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

I suspect that this could cause test flakiness. executeQuery creates a span but since it is not asserted there is a possibility that the span arrives after the test has completed and exported data is reset before the next test. If that happens the next test will fail because there is an unexpected span. You could work around this by calling testing.clearData(); before this code (to clear the existing trace) and testing.waitForTraces(1); after this code to ensure that trace data gets cleared before the next test.

Copy link
Contributor Author

@stillya stillya Apr 28, 2025

Choose a reason for hiding this comment

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

I dropped these asserts since they're irrelevant to the test (we don't check rollback functionality).

} finally {
connection.setAutoCommit(originalAutoCommit);
}
}

static Stream<Arguments> transactionOperationsStream() throws SQLException {
return Stream.of(
Arguments.of("h2", new org.h2.Driver().connect(jdbcUrls.get("h2"), null), null, "h2:mem:"),
Arguments.of(
"derby",
new EmbeddedDriver().connect(jdbcUrls.get("derby"), null),
"APP",
"derby:memory:"),
Arguments.of(
"hsqldb", new JDBCDriver().connect(jdbcUrls.get("hsqldb"), null), "SA", "hsqldb:mem:"));
}
}
6 changes: 6 additions & 0 deletions instrumentation/jdbc/library/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,9 @@ tasks {
dependsOn(testStableSemconv)
}
}

tasks {
withType<Test>().configureEach {
jvmArgs("-Dotel.instrumentation.jdbc.experimental.txn.enabled=true")
}
}
Loading
Loading