Add sqlcommenter support for jdbc and r2dbc #13714
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.instrumentation.api.incubator.semconv.db.internal; | ||
|
|
||
| import io.opentelemetry.api.trace.Span; | ||
| import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator; | ||
| import io.opentelemetry.context.Context; | ||
| import java.io.UnsupportedEncodingException; | ||
| import java.net.URLEncoder; | ||
|
|
||
| /** | ||
| * This class is internal and experimental. Its APIs are unstable and can change at any time. Its | ||
| * APIs (or a version of them) may be promoted to the public stable API in the future, but no | ||
| * guarantees are made. | ||
| */ | ||
| public final class SqlCommenterUtil { | ||
|
|
||
| /** | ||
| * Append comment containing tracing information at the end of the query. See <a | ||
| * href="https://google.github.io/sqlcommenter/spec/">sqlcommenter</a> for the description of the | ||
| * algorithm. | ||
| */ | ||
| public static String processQuery(String query) { | ||
| if (!Span.current().getSpanContext().isValid()) { | ||
| return query; | ||
| } | ||
| // skip queries that contain comments | ||
| if (containsSqlComment(query)) { | ||
| return query; | ||
| } | ||
|
|
||
| class State { | ||
| String traceparent; | ||
| String tracestate; | ||
| } | ||
|
|
||
| State state = new State(); | ||
|
|
||
| W3CTraceContextPropagator.getInstance() | ||
| .inject( | ||
| Context.current(), | ||
| state, | ||
| (carrier, key, value) -> { | ||
| if ("traceparent".equals(key)) { | ||
| carrier.traceparent = value; | ||
| } else if ("tracestate".equals(key)) { | ||
| carrier.tracestate = value; | ||
| } | ||
| }); | ||
| try { | ||
| // we know that the traceparent doesn't contain anything that needs to be encoded | ||
| query += " /*traceparent='" + state.traceparent + "'"; | ||
| if (state.tracestate != null) { | ||
| query += ", tracestate=" + serialize(state.tracestate); | ||
| } | ||
| query += "*/"; | ||
| } catch (UnsupportedEncodingException exception) { | ||
| // this exception should never happen as UTF-8 encoding is always available | ||
| } | ||
| return query; | ||
| } | ||
|
|
||
| private static boolean containsSqlComment(String query) { | ||
| return query.contains("--") || query.contains("/*"); | ||
| } | ||
|
|
||
| private static String serialize(String value) throws UnsupportedEncodingException { | ||
| // specification requires percent encoding, here we use the java build in url encoder that | ||
| // encodes space as '+' instead of '%20' as required | ||
| String result = URLEncoder.encode(value, "UTF-8").replace("+", "%20"); | ||
|
Contributor
Author
There was a problem hiding this comment. Since the |
||
| // specification requires escaping ' with a backslash, we skip this because URLEncoder already | ||
| // encodes the ' | ||
| return "'" + result + "'"; | ||
| } | ||
|
|
||
| private SqlCommenterUtil() {} | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.instrumentation.api.incubator.semconv.db.internal; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import io.opentelemetry.api.trace.Span; | ||
| import io.opentelemetry.api.trace.SpanContext; | ||
| import io.opentelemetry.api.trace.TraceFlags; | ||
| import io.opentelemetry.api.trace.TraceState; | ||
| import io.opentelemetry.context.Context; | ||
| import io.opentelemetry.context.Scope; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.ValueSource; | ||
|
|
||
| class SqlCommenterUtilTest { | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(strings = {"SELECT /**/ 1", "SELECT 1 --", "SELECT '/*'"}) | ||
| void skipQueriesWithComments(String query) { | ||
| Context parent = | ||
| Context.root() | ||
| .with( | ||
| Span.wrap( | ||
| SpanContext.create( | ||
| "ff01020304050600ff0a0b0c0d0e0f00", | ||
| "090a0b0c0d0e0f00", | ||
| TraceFlags.getSampled(), | ||
| TraceState.getDefault()))); | ||
|
|
||
| try (Scope ignore = parent.makeCurrent()) { | ||
| assertThat(SqlCommenterUtil.processQuery(query)).isEqualTo(query); | ||
| } | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(booleans = {true, false}) | ||
| void sqlCommenter(boolean hasTraceState) { | ||
| TraceState state = | ||
| hasTraceState ? TraceState.builder().put("test", "test'").build() : TraceState.getDefault(); | ||
| Context parent = | ||
| Context.root() | ||
| .with( | ||
| Span.wrap( | ||
| SpanContext.create( | ||
| "ff01020304050600ff0a0b0c0d0e0f00", | ||
| "090a0b0c0d0e0f00", | ||
| TraceFlags.getSampled(), | ||
| state))); | ||
|
|
||
| try (Scope ignore = parent.makeCurrent()) { | ||
| assertThat(SqlCommenterUtil.processQuery("SELECT 1")) | ||
| .isEqualTo( | ||
| hasTraceState | ||
| ? "SELECT 1 /*traceparent='00-ff01020304050600ff0a0b0c0d0e0f00-090a0b0c0d0e0f00-01', tracestate='test%3Dtest%27'*/" | ||
| : "SELECT 1 /*traceparent='00-ff01020304050600ff0a0b0c0d0e0f00-090a0b0c0d0e0f00-01'*/"); | ||
| } | ||
| } | ||
| } |
| 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. | | ||
| | `otel.instrumentation.jdbc.sqlcommenter.enabled` | Boolean | `false` | Enables augmenting queries with a comment containing the tracing information. See [sqlcommenter](https://google.github.io/sqlcommenter/) for more info. WARNING: augmenting queries with tracing context will make query texts unique, which may have adverse impact on database performance. Consult with database experts before enabling. | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ public static class StatementAdvice { | |
|
|
||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void onEnter( | ||
| @Advice.Argument(value = 0, readOnly = false) String sql, | ||
| @Advice.This Statement statement, | ||
| @Advice.Local("otelCallDepth") CallDepth callDepth, | ||
| @Advice.Local("otelRequest") DbRequest request, | ||
|
|
@@ -83,6 +83,8 @@ public static void onEnter( | |
| Context parentContext = currentContext(); | ||
| request = DbRequest.create(statement, sql); | ||
|
|
||
| sql = JdbcSingletons.processSql(sql); | ||
|
Contributor
Author
There was a problem hiding this comment. Here we are doing the processing before the database span is started. The consequence is that the comment with the tracing info that is appended to the query will point to the parent span of the database span which really isn't ideal (we should link to the db span not its parent). I chose to do it this way for consistency with the prepared statement handling. In prepared statement handling we process the query when
Member
There was a problem hiding this comment. oh no, I hadn't thought about this, isn't injecting the span id into the prepared statement going to invalidate the benefit of using a prepared statement?
Contributor
Author
There was a problem hiding this comment.
If you mean that it can mess with query plan caching since the full query text always changes then I guess so, depends on the db. My understanding is that It is not an universal solution that works equally well on all databases. For example supposedly it does not work on oracle because oracle strips the comment from the logged query text. Users will need to consult the documentation for the database of their choice and their database experts to determine whether this feature is usable for them.
Member
There was a problem hiding this comment. I'm worried that most JDBC drivers cache prepared statements based on exact query string (without any parsing), e.g. I looked up Oracle JDBC driver and found
I recall (from days prior to prepared statement caching being the default), that enabling prepared statement caching in c3p0 made a huge difference in application performance.
Contributor
Author
There was a problem hiding this comment.
For now I think we don't need to bother too much with the perf implications that arise from implementing the sqlcommenter as specified, we can't really do anything about it anyway. Since this feature is disabled by default users can determine themself whether it is suitable for their needs. The website for sqlcommenter lists postgres, mariadb, mysql, sqlite and google cloudsql as supported databases. For other databases propagating context to the db may require a different approach.
Contributor
Author
There was a problem hiding this comment. @trask I added a warning about possible impact on database performance.
Member
There was a problem hiding this comment. The proposed solution works on databases without a SQL cache, like Postgres and MySQL, but not with prepared statements. Prepared statements are executed only once, so it isn't possible to inject new comments with subsequent executions. On databases with a SQL cache, like Oracle and SQL Server, this approach is detrimental to performance by bloating and fragmenting the cache — each execution creates a new cached statement — and adds overhead from repeated (very expensive!) compilation and optimization.
For Oracle and SQL Server, there are alternative methods of instrumenting sessions with We can exploit application_name to inject trace context into Postgres to handle prepared statements.
That's not correct. For example, all comments are visible in V$SQL.SQL_TEXT.
Contributor
Author
There was a problem hiding this comment.
We instrument
Sorry if that was incorrect. Anyway I only wished to illustrate that the sqlcommenter comments are not going to be useful on all databases.
Member
There was a problem hiding this comment.
Which means that we add
Static comments, i.e. those that don't have to change with each execution, such as |
||
|
|
||
| if (request == null || !statementInstrumenter().shouldStart(parentContext, request)) { | ||
| return; | ||
| } | ||
|
|
@@ -112,12 +114,15 @@ public static void stopSpan( | |
| @SuppressWarnings("unused") | ||
| public static class AddBatchAdvice { | ||
|
|
||
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void addBatch( | ||
| @Advice.This Statement statement, | ||
| @Advice.Argument(value = 0, readOnly = false) String sql) { | ||
| if (statement instanceof PreparedStatement) { | ||
| return; | ||
| } | ||
| JdbcData.addStatementBatch(statement, sql); | ||
| sql = JdbcSingletons.processSql(sql); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to use
sqlcommenterinstead ofsql-commenterbecause https://google.github.io/sqlcommenter/ also spells it as one word.