Added instrumentation for transaction commit/rollback in jdbc #13709

stillya · 2025-04-14T11:40:04Z

Contribute for #10381

To enable trace when transaction commit or rollback in jdbc under experimental flag.

linux-foundation-easycla · 2025-04-14T11:40:11Z

The committers listed above are authorized under a signed CLA.

✅ login: stillya / name: Starchenko Ilya (ac5ea54, 140583e, 83795a3, 510c1b0, fb74aa2, fb22764, 54fee2e, 2de16b2, 12b544b)
✅ login: laurit / name: Lauri Tulmin (1336ab2, 5a4a0d2, a928021, 542f4b5, 114ccac, 7bccecd)

…nstrumentation into feat/jdbc-txn-instrumentation

laurit · 2025-04-16T10:40:20Z

...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerTest.java

+    public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
+      return Stream.of(
+          Arguments.of("COMMIT", expect("COMMIT", null)),
+          Arguments.of("commit", expect("COMMIT", null)),


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

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

laurit · 2025-04-16T10:43:47Z

instrumentation/jdbc/README.md

+| 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. |


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.

...src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/ConnectionInstrumentation.java

laurit · 2025-04-16T10:47:59Z

.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java

+                              equalTo(DB_STATEMENT, emitStableDatabaseSemconv() ? null : "COMMIT"),
+                              equalTo(DB_QUERY_TEXT, emitStableDatabaseSemconv() ? "COMMIT" : null),


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

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:

Add a new separate field to the request struct for the operation name.

Add a special check in the JDBC attributes getter to specifically handle commit and rollback operations.

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

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.

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

laurit · 2025-04-16T10:50:43Z

...ry/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/OpenTelemetryConnection.java

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


should be configured in JdbcTelemetryBuilder using a setter and also in OpenTelemetryDriver

…nstrumentation into feat/jdbc-txn-instrumentation

- fix review comments - add seperate transaction instrumenter

…nstrumentation into feat/jdbc-txn-instrumentation

...ary/src/main/java/io/opentelemetry/instrumentation/jdbc/datasource/JdbcTelemetryBuilder.java

...src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/ConnectionInstrumentation.java

.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java

laurit · 2025-04-28T12:29:40Z

.../test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/JdbcInstrumentationTest.java

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


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.

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

laurit · 2025-04-28T12:40:59Z

.../javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcSingletons.java

+                PeerServiceAttributesExtractor.create(
+                    transactionNetAttributesGetter,
+                    AgentCommonConfig.get().getPeerServiceResolver()))
+            .addOperationMetrics(DbClientMetrics.get())


addOperationMetrics(DbClientMetrics.get()) is responsible for creating metrics, you probably should not add it to the transaction isntrumenter

laurit · 2025-04-28T12:44:13Z

.../javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcSingletons.java

            .addOperationMetrics(DbClientMetrics.get())
            .buildInstrumenter(SpanKindExtractor.alwaysClient());
+
+    TRANSACTION_INSTRUMENTER =
+        Instrumenter.<TransactionRequest, Void>builder(


could try to reuse the code in JdbcInstrumenterFactory

I don't think so, because we use PeerServiceAttributesExtractor, which is agent-specific.

stillya · 2025-05-05T14:23:55Z

Hello @laurit,
Mind taking another look? I think I’ve fixed all the comments above.

feat: contribute for open-telemetry#10381

83795a3

To enable trace when transaction commit or rollback in jdbc under experimental flag.

stillya requested a review from a team as a code owner April 14, 2025 11:40

github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Apr 14, 2025

stillya mentioned this pull request Apr 14, 2025

Instrumentation for JDBC transaction commits #10381

Open

stillya added 2 commits April 14, 2025 16:51

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

140583e

…nstrumentation into feat/jdbc-txn-instrumentation

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

54fee2e

…nstrumentation into feat/jdbc-txn-instrumentation

laurit reviewed Apr 16, 2025

View reviewed changes

stillya added 2 commits April 23, 2025 01:37

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

fb74aa2

…nstrumentation into feat/jdbc-txn-instrumentation

Add new transaction instrumeter

fb22764

- fix review comments - add seperate transaction instrumenter

stillya force-pushed the feat/jdbc-txn-instrumentation branch from ad17c0e to fb22764 Compare April 27, 2025 18:49

stillya added 3 commits April 28, 2025 00:04

polish commit

2de16b2

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

12b544b

…nstrumentation into feat/jdbc-txn-instrumentation

polish commit

510c1b0

stillya requested a review from laurit April 27, 2025 20:04

laurit reviewed Apr 28, 2025

View reviewed changes

review fixes

ac5ea54

stillya requested a review from laurit April 28, 2025 15:20

laurit added 6 commits May 6, 2025 14:32

reduce the amount of code

114ccac

Merge branch 'main' into feat/jdbc-txn-instrumentation

1336ab2

add db.operation attribute to transaction spans

5a4a0d2

disable by default

7bccecd

Merge branch 'main' into feat/jdbc-txn-instrumentation

a928021

spring configuration, metadata

542f4b5

laurit approved these changes May 8, 2025

View reviewed changes

laurit added this to the v2.16.0 milestone May 9, 2025

laurit merged commit 97e5260 into open-telemetry:main May 14, 2025
87 checks passed

		equalTo(DB_STATEMENT, emitStableDatabaseSemconv() ? null : "COMMIT"),
		equalTo(DB_QUERY_TEXT, emitStableDatabaseSemconv() ? "COMMIT" : null),

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

Added instrumentation for transaction commit/rollback in jdbc #13709

Added instrumentation for transaction commit/rollback in jdbc #13709

Uh oh!

Conversation

stillya commented Apr 14, 2025

Uh oh!

linux-foundation-easycla bot commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stillya Apr 16, 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!

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!

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!

stillya commented May 5, 2025

Uh oh!

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Apr 14, 2025 •

edited

Loading

stillya Apr 16, 2025 •

edited

Loading