From 81005157b84b552479bf06b10ef5ec37a3c3556a Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 19 Feb 2025 16:48:45 +0200 Subject: [PATCH 1/5] Don't cache sanitization results for large sql statements --- .../db/DbClientCommonAttributesGetter.java | 8 +- .../semconv/db/DbClientSpanNameExtractor.java | 9 +-- .../api/incubator/semconv/db/MultiQuery.java | 3 +- .../db/SqlClientAttributesExtractor.java | 6 +- .../semconv/db/SqlStatementSanitizer.java | 18 +++-- .../semconv/db/SqlStatementSanitizerUtil.java | 31 ++++++++ .../api/instrumenter/Instrumenter.java | 5 ++ .../api/internal/InstrumenterContext.java | 65 +++++++++++++++ .../api/internal/InstrumenterContextTest.java | 79 +++++++++++++++++++ 9 files changed, 206 insertions(+), 18 deletions(-) create mode 100644 instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java index bc9a335f4e76..1c5ece4a9191 100644 --- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java @@ -24,7 +24,9 @@ default String getDbSystem(REQUEST request) { @Deprecated @Nullable - String getUser(REQUEST request); + default String getUser(REQUEST request) { + return null; + } /** * @deprecated Use {@link #getDbNamespace(Object)} instead. @@ -43,5 +45,7 @@ default String getDbNamespace(REQUEST request) { @Deprecated @Nullable - String getConnectionString(REQUEST request); + default String getConnectionString(REQUEST request) { + return null; + } } diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java index 78f0f65927f6..089f81631047 100644 --- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java @@ -84,9 +84,6 @@ public String extract(REQUEST request) { private static final class SqlClientSpanNameExtractor extends DbClientSpanNameExtractor { - // a dedicated sanitizer just for extracting the operation and identifier name - private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true); - private final SqlClientAttributesGetter getter; private SqlClientSpanNameExtractor(SqlClientAttributesGetter getter) { @@ -106,13 +103,15 @@ public String extract(REQUEST request) { if (rawQueryTexts.size() > 1) { // for backcompat(?) return computeSpanName(namespace, null, null); } - SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryTexts.iterator().next()); + SqlStatementInfo sanitizedStatement = + SqlStatementSanitizerUtil.sanitize(rawQueryTexts.iterator().next()); return computeSpanName( namespace, sanitizedStatement.getOperation(), sanitizedStatement.getMainIdentifier()); } if (rawQueryTexts.size() == 1) { - SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryTexts.iterator().next()); + SqlStatementInfo sanitizedStatement = + SqlStatementSanitizerUtil.sanitize(rawQueryTexts.iterator().next()); String operation = sanitizedStatement.getOperation(); if (isBatch(request)) { operation = "BATCH " + operation; diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/MultiQuery.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/MultiQuery.java index 1dc939ebcb0d..bec0a7eb007d 100644 --- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/MultiQuery.java +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/MultiQuery.java @@ -10,7 +10,6 @@ import java.util.Set; class MultiQuery { - private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true); private final String mainIdentifier; private final String operation; @@ -28,7 +27,7 @@ static MultiQuery analyze( UniqueValue uniqueOperation = new UniqueValue(); Set uniqueStatements = new LinkedHashSet<>(); for (String rawQueryText : rawQueryTexts) { - SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryText); + SqlStatementInfo sanitizedStatement = SqlStatementSanitizerUtil.sanitize(rawQueryText); String mainIdentifier = sanitizedStatement.getMainIdentifier(); uniqueMainIdentifier.set(mainIdentifier); String operation = sanitizedStatement.getOperation(); diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java index 50f9bbdba0ff..7b32d879b390 100644 --- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java @@ -55,8 +55,6 @@ public static SqlClientAttributesExtractorBuilder oldSemconvTableAttribute; private final boolean statementSanitizationEnabled; @@ -83,7 +81,7 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST if (SemconvStability.emitOldDatabaseSemconv()) { if (rawQueryTexts.size() == 1) { // for backcompat(?) String rawQueryText = rawQueryTexts.iterator().next(); - SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryText); + SqlStatementInfo sanitizedStatement = SqlStatementSanitizerUtil.sanitize(rawQueryText); String operation = sanitizedStatement.getOperation(); internalSet( attributes, @@ -104,7 +102,7 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST } if (rawQueryTexts.size() == 1) { String rawQueryText = rawQueryTexts.iterator().next(); - SqlStatementInfo sanitizedStatement = sanitizer.sanitize(rawQueryText); + SqlStatementInfo sanitizedStatement = SqlStatementSanitizerUtil.sanitize(rawQueryText); String operation = sanitizedStatement.getOperation(); internalSet( attributes, diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java index e0ea15415ef9..3f1dd9c23aa1 100644 --- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java @@ -21,6 +21,7 @@ public final class SqlStatementSanitizer { private static final Cache sqlToStatementInfoCache = Cache.bounded(1000); + private static final int LARGE_STATEMENT_THRESHOLD = 10 * 1024; public static SqlStatementSanitizer create(boolean statementSanitizationEnabled) { return new SqlStatementSanitizer(statementSanitizationEnabled); @@ -40,12 +41,19 @@ public SqlStatementInfo sanitize(@Nullable String statement, SqlDialect dialect) if (!statementSanitizationEnabled || statement == null) { return SqlStatementInfo.create(statement, null, null); } + // sanitization result will not be cached for statements larger than the threshold to avoid + // cache growing too large + // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/13180 + if (statement.length() > LARGE_STATEMENT_THRESHOLD) { + return sanitizeImpl(statement, dialect); + } return sqlToStatementInfoCache.computeIfAbsent( - CacheKey.create(statement, dialect), - k -> { - supportability.incrementCounter(SQL_STATEMENT_SANITIZER_CACHE_MISS); - return AutoSqlSanitizer.sanitize(statement, dialect); - }); + CacheKey.create(statement, dialect), k -> sanitizeImpl(statement, dialect)); + } + + private static SqlStatementInfo sanitizeImpl(@Nullable String statement, SqlDialect dialect) { + supportability.incrementCounter(SQL_STATEMENT_SANITIZER_CACHE_MISS); + return AutoSqlSanitizer.sanitize(statement, dialect); } @AutoValue diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java new file mode 100644 index 000000000000..a7211897b2c4 --- /dev/null +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java @@ -0,0 +1,31 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.incubator.semconv.db; + +import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; +import io.opentelemetry.instrumentation.api.internal.InstrumenterContext; +import java.util.HashMap; +import java.util.Map; + +/** + * Helper class for sanitizing sql that keeps sanitization results in {@link InstrumenterContext} so + * that each statement would be sanitized only once for given {@link Instrumenter} call. + */ +class SqlStatementSanitizerUtil { + private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true); + + static SqlStatementInfo sanitize(String queryText) { + if (!InstrumenterContext.isActive()) { + return sanitizer.sanitize(queryText); + } + + Map map = + InstrumenterContext.computeIfAbsent("sanitized-sql-map", unused -> new HashMap<>()); + return map.computeIfAbsent(queryText, sanitizer::sanitize); + } + + private SqlStatementSanitizerUtil() {} +} diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java index a51990317067..cde4a38cbdfd 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java @@ -14,6 +14,7 @@ import io.opentelemetry.context.ContextKey; import io.opentelemetry.instrumentation.api.internal.HttpRouteState; import io.opentelemetry.instrumentation.api.internal.InstrumenterAccess; +import io.opentelemetry.instrumentation.api.internal.InstrumenterContext; import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil; import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; import java.time.Instant; @@ -164,6 +165,10 @@ Context startAndEnd( } private Context doStart(Context parentContext, REQUEST request, @Nullable Instant startTime) { + return InstrumenterContext.withContext(() -> doStartImpl(parentContext, request, startTime)); + } + + private Context doStartImpl(Context parentContext, REQUEST request, @Nullable Instant startTime) { SpanKind spanKind = spanKindExtractor.extract(request); SpanBuilder spanBuilder = tracer.spanBuilder(spanNameExtractor.extract(request)).setSpanKind(spanKind); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java new file mode 100644 index 000000000000..97d89cf18380 --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java @@ -0,0 +1,65 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.internal; + +import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; +import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; +import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Function; +import java.util.function.Supplier; + +/** + * Helper class for sharing computed values between different {@link AttributesExtractor}s and + * {@link SpanNameExtractor} called in the start phase of the {@link Instrumenter}. + * + *

This class is internal and is hence not for public use. Its APIs are unstable and can change + * at any time. + */ +public final class InstrumenterContext { + private static final ThreadLocal instrumenterContext = new ThreadLocal<>(); + + private final Map map = new HashMap<>(); + private int useCount; + + private InstrumenterContext() {} + + @SuppressWarnings("unchecked") + public static T computeIfAbsent(String key, Function function) { + InstrumenterContext context = instrumenterContext.get(); + if (context == null) { + return function.apply(key); + } + return (T) context.map.computeIfAbsent(key, function); + } + + // visible for testing + static Map get() { + return instrumenterContext.get().map; + } + + public static boolean isActive() { + return instrumenterContext.get() != null; + } + + public static T withContext(Supplier action) { + InstrumenterContext context = instrumenterContext.get(); + if (context == null) { + context = new InstrumenterContext(); + instrumenterContext.set(context); + } + context.useCount++; + try { + return action.get(); + } finally { + context.useCount--; + if (context.useCount == 0) { + instrumenterContext.remove(); + } + } + } +} diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java new file mode 100644 index 000000000000..19b986c80b98 --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java @@ -0,0 +1,79 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.internal; + +import static io.opentelemetry.instrumentation.testing.junit.db.SemconvStabilityUtil.maybeStable; +import static org.assertj.core.api.Assertions.assertThat; + +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.incubator.semconv.db.DbClientSpanNameExtractor; +import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlClientAttributesExtractor; +import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlClientAttributesGetter; +import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlStatementInfo; +import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; +import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; +import io.opentelemetry.semconv.incubating.DbIncubatingAttributes; +import java.util.Collection; +import java.util.Collections; +import java.util.Map; +import org.junit.jupiter.api.Test; + +class InstrumenterContextTest { + + @SuppressWarnings({"unchecked", "deprecation"}) // using deprecated DB_SQL_TABLE + @Test + void testSqlSanitizer() { + String testQuery = "SELECT name FROM test WHERE id = 1"; + SqlClientAttributesGetter getter = + new SqlClientAttributesGetter() { + + @Override + public Collection getRawQueryTexts(Object request) { + return Collections.singletonList(testQuery); + } + }; + SpanNameExtractor spanNameExtractor = DbClientSpanNameExtractor.create(getter); + AttributesExtractor attributesExtractor = + SqlClientAttributesExtractor.create(getter); + + assertThat(InstrumenterContext.isActive()).isFalse(); + InstrumenterContext.withContext( + () -> { + assertThat(InstrumenterContext.isActive()).isTrue(); + assertThat(InstrumenterContext.get()).isEmpty(); + assertThat(spanNameExtractor.extract(null)).isEqualTo("SELECT test"); + // verify that sanitized statement was cached, see SqlStatementSanitizerUtil + assertThat(InstrumenterContext.get()).containsKey("sanitized-sql-map"); + Map sanitizedMap = + (Map) InstrumenterContext.get().get("sanitized-sql-map"); + assertThat(sanitizedMap).containsKey(testQuery); + + // replace cached sanitization result to verify it is used + sanitizedMap.put( + testQuery, + SqlStatementInfo.create("SELECT name2 FROM test2 WHERE id = ?", "SELECT", "test2")); + { + AttributesBuilder builder = Attributes.builder(); + attributesExtractor.onStart(builder, Context.root(), null); + assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE))) + .isEqualTo("test2"); + } + + // clear cached value to see whether it gets recomputed correctly + sanitizedMap.clear(); + { + AttributesBuilder builder = Attributes.builder(); + attributesExtractor.onStart(builder, Context.root(), null); + assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE))) + .isEqualTo("test"); + } + + return null; + }); + } +} From c10cb1cbaa99272c315743cc43f81da3933e0a6d Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 21 Feb 2025 15:14:53 +0200 Subject: [PATCH 2/5] add test --- .../semconv/db/SqlStatementSanitizer.java | 5 +++++ .../semconv/db/SqlStatementSanitizerTest.java | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java index 3f1dd9c23aa1..58fd4c3cc7dc 100644 --- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizer.java @@ -56,6 +56,11 @@ private static SqlStatementInfo sanitizeImpl(@Nullable String statement, SqlDial return AutoSqlSanitizer.sanitize(statement, dialect); } + // visible for tests + static boolean isCached(String statement) { + return sqlToStatementInfoCache.get(CacheKey.create(statement, SqlDialect.DEFAULT)) != null; + } + @AutoValue abstract static class CacheKey { diff --git a/instrumentation-api-incubator/src/test/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerTest.java b/instrumentation-api-incubator/src/test/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerTest.java index c52ffa966c3f..3b2a6d5a87b2 100644 --- a/instrumentation-api-incubator/src/test/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerTest.java +++ b/instrumentation-api-incubator/src/test/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerTest.java @@ -137,6 +137,27 @@ public void longInStatementDoesntCauseStackOverflow() { assertThat(sanitized).isEqualTo("select col from table where col in (?)"); } + @Test + public void largeStatementCached() { + // test that short statement is cached + String shortStatement = "SELECT * FROM TABLE WHERE FIELD = 1234"; + String sanitizedShort = + SqlStatementSanitizer.create(true).sanitize(shortStatement).getFullStatement(); + assertThat(sanitizedShort).doesNotContain("1234"); + assertThat(SqlStatementSanitizer.isCached(shortStatement)).isTrue(); + + // test that large statement is not cached + StringBuffer s = new StringBuffer(); + for (int i = 0; i < 10000; i++) { + s.append("SELECT * FROM TABLE WHERE FIELD = 1234 AND "); + } + String largeStatement = s.toString(); + String sanitizedLarge = + SqlStatementSanitizer.create(true).sanitize(largeStatement).getFullStatement(); + assertThat(sanitizedLarge).doesNotContain("1234"); + assertThat(SqlStatementSanitizer.isCached(largeStatement)).isFalse(); + } + static class SqlArgs implements ArgumentsProvider { @Override From 095dd7cebe2f049d8cc7dc8e7a6f3850b2e1fc3e Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 12 Mar 2025 14:07:23 +0200 Subject: [PATCH 3/5] create thread local instrumentation context only when needed --- .../semconv/db/SqlStatementSanitizerUtil.java | 4 -- .../api/instrumenter/Instrumenter.java | 6 +- .../api/internal/InstrumenterContext.java | 32 ++-------- .../api/internal/InstrumenterContextTest.java | 61 +++++++++---------- 4 files changed, 40 insertions(+), 63 deletions(-) diff --git a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java index a7211897b2c4..fc82d4d0b485 100644 --- a/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java +++ b/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlStatementSanitizerUtil.java @@ -18,10 +18,6 @@ class SqlStatementSanitizerUtil { private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true); static SqlStatementInfo sanitize(String queryText) { - if (!InstrumenterContext.isActive()) { - return sanitizer.sanitize(queryText); - } - Map map = InstrumenterContext.computeIfAbsent("sanitized-sql-map", unused -> new HashMap<>()); return map.computeIfAbsent(queryText, sanitizer::sanitize); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java index cde4a38cbdfd..66f8cb577a49 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java @@ -165,7 +165,11 @@ Context startAndEnd( } private Context doStart(Context parentContext, REQUEST request, @Nullable Instant startTime) { - return InstrumenterContext.withContext(() -> doStartImpl(parentContext, request, startTime)); + try { + return doStartImpl(parentContext, request, startTime); + } finally { + InstrumenterContext.reset(); + } } private Context doStartImpl(Context parentContext, REQUEST request, @Nullable Instant startTime) { diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java index 97d89cf18380..877a52e546dc 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java @@ -11,7 +11,6 @@ import java.util.HashMap; import java.util.Map; import java.util.function.Function; -import java.util.function.Supplier; /** * Helper class for sharing computed values between different {@link AttributesExtractor}s and @@ -21,20 +20,16 @@ * at any time. */ public final class InstrumenterContext { - private static final ThreadLocal instrumenterContext = new ThreadLocal<>(); + private static final ThreadLocal instrumenterContext = + ThreadLocal.withInitial(InstrumenterContext::new); private final Map map = new HashMap<>(); - private int useCount; private InstrumenterContext() {} @SuppressWarnings("unchecked") public static T computeIfAbsent(String key, Function function) { - InstrumenterContext context = instrumenterContext.get(); - if (context == null) { - return function.apply(key); - } - return (T) context.map.computeIfAbsent(key, function); + return (T) get().computeIfAbsent(key, function); } // visible for testing @@ -42,24 +37,7 @@ static Map get() { return instrumenterContext.get().map; } - public static boolean isActive() { - return instrumenterContext.get() != null; - } - - public static T withContext(Supplier action) { - InstrumenterContext context = instrumenterContext.get(); - if (context == null) { - context = new InstrumenterContext(); - instrumenterContext.set(context); - } - context.useCount++; - try { - return action.get(); - } finally { - context.useCount--; - if (context.useCount == 0) { - instrumenterContext.remove(); - } - } + public static void reset() { + instrumenterContext.remove(); } } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java index 19b986c80b98..60dd45604067 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java @@ -17,13 +17,16 @@ import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlStatementInfo; import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; +import io.opentelemetry.instrumentation.testing.internal.AutoCleanupExtension; import io.opentelemetry.semconv.incubating.DbIncubatingAttributes; import java.util.Collection; import java.util.Collections; import java.util.Map; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; class InstrumenterContextTest { + @RegisterExtension static final AutoCleanupExtension cleanup = AutoCleanupExtension.create(); @SuppressWarnings({"unchecked", "deprecation"}) // using deprecated DB_SQL_TABLE @Test @@ -41,39 +44,35 @@ public Collection getRawQueryTexts(Object request) { AttributesExtractor attributesExtractor = SqlClientAttributesExtractor.create(getter); - assertThat(InstrumenterContext.isActive()).isFalse(); - InstrumenterContext.withContext( - () -> { - assertThat(InstrumenterContext.isActive()).isTrue(); - assertThat(InstrumenterContext.get()).isEmpty(); - assertThat(spanNameExtractor.extract(null)).isEqualTo("SELECT test"); - // verify that sanitized statement was cached, see SqlStatementSanitizerUtil - assertThat(InstrumenterContext.get()).containsKey("sanitized-sql-map"); - Map sanitizedMap = - (Map) InstrumenterContext.get().get("sanitized-sql-map"); - assertThat(sanitizedMap).containsKey(testQuery); + InstrumenterContext.reset(); + cleanup.deferCleanup(InstrumenterContext::reset); - // replace cached sanitization result to verify it is used - sanitizedMap.put( - testQuery, - SqlStatementInfo.create("SELECT name2 FROM test2 WHERE id = ?", "SELECT", "test2")); - { - AttributesBuilder builder = Attributes.builder(); - attributesExtractor.onStart(builder, Context.root(), null); - assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE))) - .isEqualTo("test2"); - } + assertThat(InstrumenterContext.get()).isEmpty(); + assertThat(spanNameExtractor.extract(null)).isEqualTo("SELECT test"); + // verify that sanitized statement was cached, see SqlStatementSanitizerUtil + assertThat(InstrumenterContext.get()).containsKey("sanitized-sql-map"); + Map sanitizedMap = + (Map) InstrumenterContext.get().get("sanitized-sql-map"); + assertThat(sanitizedMap).containsKey(testQuery); - // clear cached value to see whether it gets recomputed correctly - sanitizedMap.clear(); - { - AttributesBuilder builder = Attributes.builder(); - attributesExtractor.onStart(builder, Context.root(), null); - assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE))) - .isEqualTo("test"); - } + // replace cached sanitization result to verify it is used + sanitizedMap.put( + testQuery, + SqlStatementInfo.create("SELECT name2 FROM test2 WHERE id = ?", "SELECT", "test2")); + { + AttributesBuilder builder = Attributes.builder(); + attributesExtractor.onStart(builder, Context.root(), null); + assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE))) + .isEqualTo("test2"); + } - return null; - }); + // clear cached value to see whether it gets recomputed correctly + sanitizedMap.clear(); + { + AttributesBuilder builder = Attributes.builder(); + attributesExtractor.onStart(builder, Context.root(), null); + assertThat(builder.build().get(maybeStable(DbIncubatingAttributes.DB_SQL_TABLE))) + .isEqualTo("test"); + } } } From 1e772138e496b195c13a56eed0ffff85e7eb1919 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 12 Mar 2025 15:39:07 +0200 Subject: [PATCH 4/5] animalsniffer --- .../instrumentation/api/internal/InstrumenterContext.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java index 877a52e546dc..06c3f84b60a3 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContext.java @@ -21,7 +21,12 @@ */ public final class InstrumenterContext { private static final ThreadLocal instrumenterContext = - ThreadLocal.withInitial(InstrumenterContext::new); + new ThreadLocal() { + @Override + protected InstrumenterContext initialValue() { + return new InstrumenterContext(); + } + }; private final Map map = new HashMap<>(); From cbeaa217f31b11f28f65ded97cd5f82151f71a14 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 10 Apr 2025 19:35:47 +0300 Subject: [PATCH 5/5] fix merge --- .../instrumentation/api/internal/InstrumenterContextTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java index 60dd45604067..044935031cc2 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/InstrumenterContextTest.java @@ -32,8 +32,8 @@ class InstrumenterContextTest { @Test void testSqlSanitizer() { String testQuery = "SELECT name FROM test WHERE id = 1"; - SqlClientAttributesGetter getter = - new SqlClientAttributesGetter() { + SqlClientAttributesGetter getter = + new SqlClientAttributesGetter() { @Override public Collection getRawQueryTexts(Object request) {