Skip to content

Commit 8100515

Browse files
lauritlaurit
committed
Don't cache sanitization results for large sql statements
1 parent cd72da2 commit 8100515

File tree

9 files changed

+206
-18
lines changed

9 files changed

+206
-18
lines changed

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java

+6-2
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@ default String getDbSystem(REQUEST request) {
2424

2525
@Deprecated
2626
@Nullable
27-
String getUser(REQUEST request);
27+
default String getUser(REQUEST request) {
28+
return null;
29+
}
2830

2931
/**
3032
* @deprecated Use {@link #getDbNamespace(Object)} instead.
@@ -43,5 +45,7 @@ default String getDbNamespace(REQUEST request) {
4345

4446
@Deprecated
4547
@Nullable
46-
String getConnectionString(REQUEST request);
48+
default String getConnectionString(REQUEST request) {
49+
return null;
50+
}
4751
}

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java

+4-5
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,6 @@ public String extract(REQUEST request) {
8484
private static final class SqlClientSpanNameExtractor<REQUEST>
8585
extends DbClientSpanNameExtractor<REQUEST> {
8686

87-
// a dedicated sanitizer just for extracting the operation and identifier name
88-
private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true);
89-
9087
private final SqlClientAttributesGetter<REQUEST> getter;
9188

9289
private SqlClientSpanNameExtractor(SqlClientAttributesGetter<REQUEST> getter) {
@@ -106,13 +103,15 @@ public String extract(REQUEST request) {
106103
if (rawQueryTexts.size() > 1) { // for backcompat(?)
107104
return computeSpanName(namespace, null, null);
108105
}
109-
SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryTexts.iterator().next());
106+
SqlStatementInfo sanitizedStatement =
107+
SqlStatementSanitizerUtil.sanitize(rawQueryTexts.iterator().next());
110108
return computeSpanName(
111109
namespace, sanitizedStatement.getOperation(), sanitizedStatement.getMainIdentifier());
112110
}
113111

114112
if (rawQueryTexts.size() == 1) {
115-
SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryTexts.iterator().next());
113+
SqlStatementInfo sanitizedStatement =
114+
SqlStatementSanitizerUtil.sanitize(rawQueryTexts.iterator().next());
116115
String operation = sanitizedStatement.getOperation();
117116
if (isBatch(request)) {
118117
operation = "BATCH " + operation;

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/MultiQuery.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import java.util.Set;
1111

1212
class MultiQuery {
13-
private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true);
1413

1514
private final String mainIdentifier;
1615
private final String operation;
@@ -28,7 +27,7 @@ static MultiQuery analyze(
2827
UniqueValue uniqueOperation = new UniqueValue();
2928
Set<String> uniqueStatements = new LinkedHashSet<>();
3029
for (String rawQueryText : rawQueryTexts) {
31-
SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryText);
30+
SqlStatementInfo sanitizedStatement = SqlStatementSanitizerUtil.sanitize(rawQueryText);
3231
String mainIdentifier = sanitizedStatement.getMainIdentifier();
3332
uniqueMainIdentifier.set(mainIdentifier);
3433
String operation = sanitizedStatement.getOperation();

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java

+2-4
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,6 @@ public static <REQUEST, RESPONSE> SqlClientAttributesExtractorBuilder<REQUEST, R
5555
}
5656

5757
private static final String SQL_CALL = "CALL";
58-
// sanitizer is also used to extract operation and table name, so we have it always enabled here
59-
private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true);
6058

6159
private final AttributeKey<String> oldSemconvTableAttribute;
6260
private final boolean statementSanitizationEnabled;
@@ -83,7 +81,7 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
8381
if (SemconvStability.emitOldDatabaseSemconv()) {
8482
if (rawQueryTexts.size() == 1) { // for backcompat(?)
8583
String rawQueryText = rawQueryTexts.iterator().next();
86-
SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryText);
84+
SqlStatementInfo sanitizedStatement = SqlStatementSanitizerUtil.sanitize(rawQueryText);
8785
String operation = sanitizedStatement.getOperation();
8886
internalSet(
8987
attributes,
@@ -104,7 +102,7 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
104102
}
105103
if (rawQueryTexts.size() == 1) {
106104
String rawQueryText = rawQueryTexts.iterator().next();
107-
SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryText);
105+
SqlStatementInfo sanitizedStatement = SqlStatementSanitizerUtil.sanitize(rawQueryText);
108106
String operation = sanitizedStatement.getOperation();
109107
internalSet(
110108
attributes,

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java

+13-5
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ public final class SqlStatementSanitizer {
2121

2222
private static final Cache<CacheKey, SqlStatementInfo> sqlToStatementInfoCache =
2323
Cache.bounded(1000);
24+
private static final int LARGE_STATEMENT_THRESHOLD = 10 * 1024;
2425

2526
public static SqlStatementSanitizer create(boolean statementSanitizationEnabled) {
2627
return new SqlStatementSanitizer(statementSanitizationEnabled);
@@ -40,12 +41,19 @@ public SqlStatementInfo sanitize(@Nullable String statement, SqlDialect dialect)
4041
if (!statementSanitizationEnabled || statement == null) {
4142
return SqlStatementInfo.create(statement, null, null);
4243
}
44+
// sanitization result will not be cached for statements larger than the threshold to avoid
45+
// cache growing too large
46+
// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/13180
47+
if (statement.length() > LARGE_STATEMENT_THRESHOLD) {
48+
return sanitizeImpl(statement, dialect);
49+
}
4350
return sqlToStatementInfoCache.computeIfAbsent(
44-
CacheKey.create(statement, dialect),
45-
k -> {
46-
supportability.incrementCounter(SQL_STATEMENT_SANITIZER_CACHE_MISS);
47-
return AutoSqlSanitizer.sanitize(statement, dialect);
48-
});
51+
CacheKey.create(statement, dialect), k -> sanitizeImpl(statement, dialect));
52+
}
53+
54+
private static SqlStatementInfo sanitizeImpl(@Nullable String statement, SqlDialect dialect) {
55+
supportability.incrementCounter(SQL_STATEMENT_SANITIZER_CACHE_MISS);
56+
return AutoSqlSanitizer.sanitize(statement, dialect);
4957
}
5058

5159
@AutoValue

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.api.incubator.semconv.db;
7+
8+
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
9+
import io.opentelemetry.instrumentation.api.internal.InstrumenterContext;
10+
import java.util.HashMap;
11+
import java.util.Map;
12+
13+
/**
14+
* Helper class for sanitizing sql that keeps sanitization results in {@link InstrumenterContext} so
15+
* that each statement would be sanitized only once for given {@link Instrumenter} call.
16+
*/
17+
class SqlStatementSanitizerUtil {
18+
private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true);
19+
20+
static SqlStatementInfo sanitize(String queryText) {
21+
if (!InstrumenterContext.isActive()) {
22+
return sanitizer.sanitize(queryText);
23+
}
24+
25+
Map<String, SqlStatementInfo> map =
26+
InstrumenterContext.computeIfAbsent("sanitized-sql-map", unused -> new HashMap<>());
27+
return map.computeIfAbsent(queryText, sanitizer::sanitize);
28+
}
29+
30+
private SqlStatementSanitizerUtil() {}
31+
}

‎instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

+5
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import io.opentelemetry.context.ContextKey;
1515
import io.opentelemetry.instrumentation.api.internal.HttpRouteState;
1616
import io.opentelemetry.instrumentation.api.internal.InstrumenterAccess;
17+
import io.opentelemetry.instrumentation.api.internal.InstrumenterContext;
1718
import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil;
1819
import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics;
1920
import java.time.Instant;
@@ -164,6 +165,10 @@ Context startAndEnd(
164165
}
165166

166167
private Context doStart(Context parentContext, REQUEST request, @Nullable Instant startTime) {
168+
return InstrumenterContext.withContext(() -> doStartImpl(parentContext, request, startTime));
169+
}
170+
171+
private Context doStartImpl(Context parentContext, REQUEST request, @Nullable Instant startTime) {
167172
SpanKind spanKind = spanKindExtractor.extract(request);
168173
SpanBuilder spanBuilder =
169174
tracer.spanBuilder(spanNameExtractor.extract(request)).setSpanKind(spanKind);

‎instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java

+65
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.api.internal;
7+
8+
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
9+
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
10+
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
11+
import java.util.HashMap;
12+
import java.util.Map;
13+
import java.util.function.Function;
14+
import java.util.function.Supplier;
15+
16+
/**
17+
* Helper class for sharing computed values between different {@link AttributesExtractor}s and
18+
* {@link SpanNameExtractor} called in the start phase of the {@link Instrumenter}.
19+
*
20+
* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
21+
* at any time.
22+
*/
23+
public final class InstrumenterContext {
24+
private static final ThreadLocal<InstrumenterContext> instrumenterContext = new ThreadLocal<>();
25+
26+
private final Map<String, Object> map = new HashMap<>();
27+
private int useCount;
28+
29+
private InstrumenterContext() {}
30+
31+
@SuppressWarnings("unchecked")
32+
public static <T> T computeIfAbsent(String key, Function<String, T> function) {
33+
InstrumenterContext context = instrumenterContext.get();
34+
if (context == null) {
35+
return function.apply(key);
36+
}
37+
return (T) context.map.computeIfAbsent(key, function);
38+
}
39+
40+
// visible for testing
41+
static Map<String, Object> get() {
42+
return instrumenterContext.get().map;
43+
}
44+
45+
public static boolean isActive() {
46+
return instrumenterContext.get() != null;
47+
}
48+
49+
public static <T> T withContext(Supplier<T> action) {
50+
InstrumenterContext context = instrumenterContext.get();
51+
if (context == null) {
52+
context = new InstrumenterContext();
53+
instrumenterContext.set(context);
54+
}
55+
context.useCount++;
56+
try {
57+
return action.get();
58+
} finally {
59+
context.useCount--;
60+
if (context.useCount == 0) {
61+
instrumenterContext.remove();
62+
}
63+
}
64+
}
65+
}

‎instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java

+79
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.api.internal;
7+
8+
import static io.opentelemetry.instrumentation.testing.junit.db.SemconvStabilityUtil.maybeStable;
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
import io.opentelemetry.api.common.Attributes;
12+
import io.opentelemetry.api.common.AttributesBuilder;
13+
import io.opentelemetry.context.Context;
14+
import io.opentelemetry.instrumentation.api.incubator.semconv.db.DbClientSpanNameExtractor;
15+
import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlClientAttributesExtractor;
16+
import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlClientAttributesGetter;
17+
import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlStatementInfo;
18+
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
19+
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
20+
import io.opentelemetry.semconv.incubating.DbIncubatingAttributes;
21+
import java.util.Collection;
22+
import java.util.Collections;
23+
import java.util.Map;
24+
import org.junit.jupiter.api.Test;
25+
26+
class InstrumenterContextTest {
27+
28+
@SuppressWarnings({"unchecked", "deprecation"}) // using deprecated DB_SQL_TABLE
29+
@Test
30+
void testSqlSanitizer() {
31+
String testQuery = "SELECT name FROM test WHERE id = 1";
32+
SqlClientAttributesGetter<Object> getter =
33+
new SqlClientAttributesGetter<Object>() {
34+
35+
@Override
36+
public Collection<String> getRawQueryTexts(Object request) {
37+
return Collections.singletonList(testQuery);
38+
}
39+
};
40+
SpanNameExtractor<Object> spanNameExtractor = DbClientSpanNameExtractor.create(getter);
41+
AttributesExtractor<Object, Void> attributesExtractor =
42+
SqlClientAttributesExtractor.create(getter);
43+
44+
assertThat(InstrumenterContext.isActive()).isFalse();
45+
InstrumenterContext.withContext(
46+
() -> {
47+
assertThat(InstrumenterContext.isActive()).isTrue();
48+
assertThat(InstrumenterContext.get()).isEmpty();
49+
assertThat(spanNameExtractor.extract(null)).isEqualTo("SELECT test");
50+
// verify that sanitized statement was cached, see SqlStatementSanitizerUtil
51+
assertThat(InstrumenterContext.get()).containsKey("sanitized-sql-map");
52+
Map<String, SqlStatementInfo> sanitizedMap =
53+
(Map<String, SqlStatementInfo>) InstrumenterContext.get().get("sanitized-sql-map");
54+
assertThat(sanitizedMap).containsKey(testQuery);
55+
56+
// replace cached sanitization result to verify it is used
57+
sanitizedMap.put(
58+
testQuery,
59+
SqlStatementInfo.create("SELECT name2 FROM test2 WHERE id = ?", "SELECT", "test2"));
60+
{
61+
AttributesBuilder builder = Attributes.builder();
62+
attributesExtractor.onStart(builder, Context.root(), null);
63+
assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE)))
64+
.isEqualTo("test2");
65+
}
66+
67+
// clear cached value to see whether it gets recomputed correctly
68+
sanitizedMap.clear();
69+
{
70+
AttributesBuilder builder = Attributes.builder();
71+
attributesExtractor.onStart(builder, Context.root(), null);
72+
assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE)))
73+
.isEqualTo("test");
74+
}
75+
76+
return null;
77+
});
78+
}
79+
}

0 commit comments

Comments
 (0)