From 738c6d5e2c7d5b8fc748fa22c25cd36dcaa6d86b Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 7 Apr 2021 13:56:34 +0200 Subject: [PATCH 1/3] Record internal metric for SQL cache misses And use `SupportabilityMetrics` in `Instrumenter` --- .../api/db/SqlStatementSanitizer.java | 7 ++--- .../api/instrumenter/ClientInstrumenter.java | 2 ++ .../api/instrumenter/Instrumenter.java | 26 ++++++++++++----- .../api/instrumenter/InstrumenterBuilder.java | 20 +++++++++++-- .../api/instrumenter/ServerInstrumenter.java | 2 ++ .../SupportabilityMetrics.java | 29 +++++++++++++++---- .../api/tracer/BaseTracer.java | 6 ++-- .../SupportabilityMetricsTest.java | 22 ++++++++++---- 8 files changed, 86 insertions(+), 28 deletions(-) rename instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/{tracer => internal}/SupportabilityMetrics.java (79%) rename instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/{tracer => internal}/SupportabilityMetricsTest.java (77%) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java index a2f2966a048f..89065620c19e 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java @@ -8,15 +8,14 @@ import static io.opentelemetry.instrumentation.api.db.StatementSanitizationConfig.isStatementSanitizationEnabled; import io.opentelemetry.instrumentation.api.caching.Cache; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; /** * This class is responsible for masking potentially sensitive parameters in SQL (and SQL-like) * statements and queries. */ public final class SqlStatementSanitizer { - private static final Logger log = LoggerFactory.getLogger(SqlStatementSanitizer.class); + private static final SupportabilityMetrics supportability = SupportabilityMetrics.instance(); private static final Cache sqlToStatementInfoCache = Cache.newBuilder().setMaximumSize(1000).build(); @@ -28,7 +27,7 @@ public static SqlStatementInfo sanitize(String statement) { return sqlToStatementInfoCache.computeIfAbsent( statement, k -> { - log.trace("SQL statement cache miss"); + supportability.incrementCounter("SqlStatementSanitizer cache miss"); return AutoSqlSanitizer.sanitize(statement); }); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java index 815bca1800a2..22520eda3875 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java @@ -17,6 +17,7 @@ final class ClientInstrumenter extends Instrumenter setter; ClientInstrumenter( + String instrumentationName, Tracer tracer, SpanNameExtractor spanNameExtractor, SpanKindExtractor spanKindExtractor, @@ -26,6 +27,7 @@ final class ClientInstrumenter extends Instrumenter setter) { super( + instrumentationName, tracer, spanNameExtractor, spanKindExtractor, 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 d696fdf94e51..a06606c37569 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.api.trace.SpanKind; import io.opentelemetry.api.trace.Tracer; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; import io.opentelemetry.instrumentation.api.tracer.ClientSpan; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import java.util.List; @@ -46,6 +47,9 @@ public static InstrumenterBuilder newBuil return new InstrumenterBuilder<>(openTelemetry, instrumentationName, spanNameExtractor); } + private static final SupportabilityMetrics supportability = SupportabilityMetrics.instance(); + + private final String instrumentationName; private final Tracer tracer; private final SpanNameExtractor spanNameExtractor; private final SpanKindExtractor spanKindExtractor; @@ -54,12 +58,14 @@ public static InstrumenterBuilder newBuil private final ErrorCauseExtractor errorCauseExtractor; Instrumenter( + String instrumentationName, Tracer tracer, SpanNameExtractor spanNameExtractor, SpanKindExtractor spanKindExtractor, SpanStatusExtractor spanStatusExtractor, List> extractors, ErrorCauseExtractor errorCauseExtractor) { + this.instrumentationName = instrumentationName; this.tracer = tracer; this.spanNameExtractor = spanNameExtractor; this.spanKindExtractor = spanKindExtractor; @@ -70,21 +76,27 @@ public static InstrumenterBuilder newBuil /** * Returns whether instrumentation should be applied for the {@link REQUEST}. If {@code true}, - * call {@link #start(Context, Object)} and {@link #end(Context, Object, Object, Throwable)} arond - * the operation being instrumented, or if {@code false} execute the operation directly without - * calling those methods. + * call {@link #start(Context, Object)} and {@link #end(Context, Object, Object, Throwable)} + * around the operation being instrumented, or if {@code false} execute the operation directly + * without calling those methods. */ public boolean shouldStart(Context parentContext, REQUEST request) { + boolean suppressed = false; SpanKind spanKind = spanKindExtractor.extract(request); switch (spanKind) { case SERVER: - return ServerSpan.fromContextOrNull(parentContext) == null; + suppressed = ServerSpan.fromContextOrNull(parentContext) == null; + break; case CLIENT: - return ClientSpan.fromContextOrNull(parentContext) == null; + suppressed = ClientSpan.fromContextOrNull(parentContext) == null; + break; default: - // fallthrough + break; + } + if (suppressed) { + supportability.recordSuppressedSpan(spanKind, instrumentationName); } - return true; + return !suppressed; } /** diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java index 52508c79f83b..eacd635a510d 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java @@ -114,6 +114,7 @@ private Instrumenter newInstrumenter( InstrumenterConstructor constructor, SpanKindExtractor spanKindExtractor) { return constructor.create( + instrumentationName, openTelemetry.getTracer(instrumentationName, InstrumentationVersion.VERSION), spanNameExtractor, spanKindExtractor, @@ -124,6 +125,7 @@ private Instrumenter newInstrumenter( private interface InstrumenterConstructor { Instrumenter create( + String instrumentationName, Tracer tracer, SpanNameExtractor spanNameExtractor, SpanKindExtractor spanKindExtractor, @@ -137,8 +139,15 @@ static InstrumenterConstructor internal() { static InstrumenterConstructor propagatingToDownstream( ContextPropagators propagators, TextMapSetter setter) { - return (tracer, spanName, spanKind, spanStatus, attributes, errorCauseExtractor) -> + return (instrumentationName, + tracer, + spanName, + spanKind, + spanStatus, + attributes, + errorCauseExtractor) -> new ClientInstrumenter<>( + instrumentationName, tracer, spanName, spanKind, @@ -151,8 +160,15 @@ static InstrumenterConstructor propagatingToDownstream( static InstrumenterConstructor propagatingFromUpstream( ContextPropagators propagators, TextMapGetter getter) { - return (tracer, spanName, spanKind, spanStatus, attributes, errorCauseExtractor) -> + return (instrumentationName, + tracer, + spanName, + spanKind, + spanStatus, + attributes, + errorCauseExtractor) -> new ServerInstrumenter<>( + instrumentationName, tracer, spanName, spanKind, diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java index 21d134c3f902..afdebaa31c59 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java @@ -17,6 +17,7 @@ final class ServerInstrumenter extends Instrumenter getter; ServerInstrumenter( + String instrumentationName, Tracer tracer, SpanNameExtractor spanNameExtractor, SpanKindExtractor spanKindExtractor, @@ -26,6 +27,7 @@ final class ServerInstrumenter extends Instrumenter getter) { super( + instrumentationName, tracer, spanNameExtractor, spanKindExtractor, diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/SupportabilityMetrics.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java similarity index 79% rename from instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/SupportabilityMetrics.java rename to instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java index d95c4640ca56..901e023de071 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/SupportabilityMetrics.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.instrumentation.api.tracer; +package io.opentelemetry.instrumentation.api.internal; import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.instrumentation.api.config.Config; @@ -16,15 +16,19 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -class SupportabilityMetrics { +public final class SupportabilityMetrics { private static final Logger log = LoggerFactory.getLogger(SupportabilityMetrics.class); private final boolean agentDebugEnabled; private final Consumer reporter; private final ConcurrentMap suppressionCounters = new ConcurrentHashMap<>(); + private final ConcurrentMap counters = new ConcurrentHashMap<>(); - public SupportabilityMetrics(Config config) { - this(config, log::debug); + private static final SupportabilityMetrics INSTANCE = + new SupportabilityMetrics(Config.get(), log::debug).start(); + + public static SupportabilityMetrics instance() { + return INSTANCE; } // visible for testing @@ -33,7 +37,7 @@ public SupportabilityMetrics(Config config) { this.reporter = reporter; } - void recordSuppressedSpan(SpanKind kind, String instrumentationName) { + public void recordSuppressedSpan(SpanKind kind, String instrumentationName) { if (!agentDebugEnabled) { return; } @@ -43,6 +47,14 @@ void recordSuppressedSpan(SpanKind kind, String instrumentationName) { .increment(kind); } + public void incrementCounter(String counterName) { + if (!agentDebugEnabled) { + return; + } + + counters.computeIfAbsent(counterName, k -> new LongAdder()).increment(); + } + // visible for testing void report() { suppressionCounters.forEach( @@ -55,6 +67,13 @@ void report() { } } }); + counters.forEach( + (counterName, counter) -> { + long value = counter.sumThenReset(); + if (value > 0) { + reporter.accept("Counter '" + counterName + "' : " + value); + } + }); } SupportabilityMetrics start() { diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java index 7b16550c5182..edf1012fa30f 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java @@ -18,8 +18,8 @@ import io.opentelemetry.context.propagation.TextMapPropagator; import io.opentelemetry.context.propagation.TextMapSetter; import io.opentelemetry.instrumentation.api.InstrumentationVersion; -import io.opentelemetry.instrumentation.api.config.Config; import io.opentelemetry.instrumentation.api.context.ContextPropagationDebug; +import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.UndeclaredThrowableException; @@ -48,9 +48,7 @@ * are able to see those attributes in the {@code onStart()} method and can freely read/modify them. */ public abstract class BaseTracer { - // should we make this injectable? - private static final SupportabilityMetrics supportability = - new SupportabilityMetrics(Config.get()).start(); + private static final SupportabilityMetrics supportability = SupportabilityMetrics.instance(); private final Tracer tracer; private final ContextPropagators propagators; diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/SupportabilityMetricsTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java similarity index 77% rename from instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/SupportabilityMetricsTest.java rename to instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java index 3f94c1a11b7d..adf70d9ec350 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/SupportabilityMetricsTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java @@ -3,9 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.instrumentation.api.tracer; +package io.opentelemetry.instrumentation.api.internal; -import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; import io.opentelemetry.api.trace.SpanKind; @@ -28,6 +27,9 @@ void disabled() { metrics.recordSuppressedSpan(SpanKind.SERVER, "favoriteInstrumentation"); metrics.recordSuppressedSpan(SpanKind.CLIENT, "favoriteInstrumentation"); metrics.recordSuppressedSpan(SpanKind.INTERNAL, "otherInstrumentation"); + metrics.incrementCounter("some counter"); + metrics.incrementCounter("another counter"); + metrics.incrementCounter("some counter"); metrics.report(); @@ -45,17 +47,22 @@ void reportsMetrics() { metrics.recordSuppressedSpan(SpanKind.SERVER, "favoriteInstrumentation"); metrics.recordSuppressedSpan(SpanKind.CLIENT, "favoriteInstrumentation"); metrics.recordSuppressedSpan(SpanKind.INTERNAL, "otherInstrumentation"); + metrics.incrementCounter("some counter"); + metrics.incrementCounter("another counter"); + metrics.incrementCounter("some counter"); metrics.report(); assertThat(reports) .isNotEmpty() - .hasSize(3) + .hasSize(5) .hasSameElementsAs( Arrays.asList( "Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 2", "Suppressed Spans by 'favoriteInstrumentation' (SERVER) : 1", - "Suppressed Spans by 'otherInstrumentation' (INTERNAL) : 1")); + "Suppressed Spans by 'otherInstrumentation' (INTERNAL) : 1", + "Counter 'some counter' : 2", + "Counter 'another counter' : 1")); } @Test @@ -66,14 +73,17 @@ void resetsCountsEachReport() { Config.create(Collections.singletonMap("otel.javaagent.debug", "true")), reports::add); metrics.recordSuppressedSpan(SpanKind.CLIENT, "favoriteInstrumentation"); + metrics.incrementCounter("some counter"); metrics.report(); metrics.report(); assertThat(reports) .isNotEmpty() - .hasSize(1) + .hasSize(2) .hasSameElementsAs( - singletonList("Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 1")); + Arrays.asList( + "Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 1", + "Counter 'some counter' : 1")); } } From daf6f4e39604981b4fb3e8f52e9f6113b0b9f81a Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 7 Apr 2021 14:50:16 +0200 Subject: [PATCH 2/3] Fix broken shouldStart() logic --- .../instrumentation/api/instrumenter/Instrumenter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 a06606c37569..78551eb5a739 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 @@ -85,10 +85,10 @@ public boolean shouldStart(Context parentContext, REQUEST request) { SpanKind spanKind = spanKindExtractor.extract(request); switch (spanKind) { case SERVER: - suppressed = ServerSpan.fromContextOrNull(parentContext) == null; + suppressed = ServerSpan.fromContextOrNull(parentContext) != null; break; case CLIENT: - suppressed = ClientSpan.fromContextOrNull(parentContext) == null; + suppressed = ClientSpan.fromContextOrNull(parentContext) != null; break; default: break; From d0a403b3a2c4a5c4de20f1add8e6ebba8f8a68f1 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 8 Apr 2021 10:51:36 +0200 Subject: [PATCH 3/3] Code review comments --- .../api/db/SqlStatementSanitizer.java | 3 ++- .../api/internal/SupportabilityMetrics.java | 7 ++++++ .../internal/SupportabilityMetricsTest.java | 25 +++++++------------ 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java index 89065620c19e..5d553d1e0afd 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizer.java @@ -6,6 +6,7 @@ package io.opentelemetry.instrumentation.api.db; import static io.opentelemetry.instrumentation.api.db.StatementSanitizationConfig.isStatementSanitizationEnabled; +import static io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics.CounterNames.SQL_STATEMENT_SANITIZER_CACHE_MISS; import io.opentelemetry.instrumentation.api.caching.Cache; import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; @@ -27,7 +28,7 @@ public static SqlStatementInfo sanitize(String statement) { return sqlToStatementInfoCache.computeIfAbsent( statement, k -> { - supportability.incrementCounter("SqlStatementSanitizer cache miss"); + supportability.incrementCounter(SQL_STATEMENT_SANITIZER_CACHE_MISS); return AutoSqlSanitizer.sanitize(statement); }); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java index 901e023de071..8b84a190a2b1 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java @@ -91,6 +91,13 @@ SupportabilityMetrics start() { return this; } + public static final class CounterNames { + public static final String SQL_STATEMENT_SANITIZER_CACHE_MISS = + "SqlStatementSanitizer cache miss"; + + private CounterNames() {} + } + // this class is threadsafe. private static class KindCounters { private final LongAdder server = new LongAdder(); diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java index adf70d9ec350..62dadc314768 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java @@ -10,7 +10,6 @@ import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.instrumentation.api.config.Config; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; import org.junit.jupiter.api.Test; @@ -54,15 +53,12 @@ void reportsMetrics() { metrics.report(); assertThat(reports) - .isNotEmpty() - .hasSize(5) - .hasSameElementsAs( - Arrays.asList( - "Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 2", - "Suppressed Spans by 'favoriteInstrumentation' (SERVER) : 1", - "Suppressed Spans by 'otherInstrumentation' (INTERNAL) : 1", - "Counter 'some counter' : 2", - "Counter 'another counter' : 1")); + .containsExactlyInAnyOrder( + "Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 2", + "Suppressed Spans by 'favoriteInstrumentation' (SERVER) : 1", + "Suppressed Spans by 'otherInstrumentation' (INTERNAL) : 1", + "Counter 'some counter' : 2", + "Counter 'another counter' : 1"); } @Test @@ -79,11 +75,8 @@ void resetsCountsEachReport() { metrics.report(); assertThat(reports) - .isNotEmpty() - .hasSize(2) - .hasSameElementsAs( - Arrays.asList( - "Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 1", - "Counter 'some counter' : 1")); + .containsExactlyInAnyOrder( + "Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 1", + "Counter 'some counter' : 1"); } }