Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record internal metric for SQL cache misses #2747

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,17 @@
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 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<String, SqlStatementInfo> sqlToStatementInfoCache =
Cache.newBuilder().setMaximumSize(1000).build();
Expand All @@ -28,7 +28,7 @@ public static SqlStatementInfo sanitize(String statement) {
return sqlToStatementInfoCache.computeIfAbsent(
statement,
k -> {
log.trace("SQL statement cache miss");
supportability.incrementCounter(SQL_STATEMENT_SANITIZER_CACHE_MISS);
return AutoSqlSanitizer.sanitize(statement);
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ final class ClientInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
private final TextMapSetter<REQUEST> setter;

ClientInstrumenter(
String instrumentationName,
Tracer tracer,
SpanNameExtractor<? super REQUEST> spanNameExtractor,
SpanKindExtractor<? super REQUEST> spanKindExtractor,
Expand All @@ -26,6 +27,7 @@ final class ClientInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
ContextPropagators propagators,
TextMapSetter<REQUEST> setter) {
super(
instrumentationName,
tracer,
spanNameExtractor,
spanKindExtractor,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -46,6 +47,9 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> 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<? super REQUEST> spanNameExtractor;
private final SpanKindExtractor<? super REQUEST> spanKindExtractor;
Expand All @@ -54,12 +58,14 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> newBuil
private final ErrorCauseExtractor errorCauseExtractor;

Instrumenter(
String instrumentationName,
Tracer tracer,
SpanNameExtractor<? super REQUEST> spanNameExtractor,
SpanKindExtractor<? super REQUEST> spanKindExtractor,
SpanStatusExtractor<? super REQUEST, ? super RESPONSE> spanStatusExtractor,
List<? extends AttributesExtractor<? super REQUEST, ? super RESPONSE>> extractors,
ErrorCauseExtractor errorCauseExtractor) {
this.instrumentationName = instrumentationName;
this.tracer = tracer;
this.spanNameExtractor = spanNameExtractor;
this.spanKindExtractor = spanKindExtractor;
Expand All @@ -70,21 +76,27 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> 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;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ private Instrumenter<REQUEST, RESPONSE> newInstrumenter(
InstrumenterConstructor<REQUEST, RESPONSE> constructor,
SpanKindExtractor<? super REQUEST> spanKindExtractor) {
return constructor.create(
instrumentationName,
openTelemetry.getTracer(instrumentationName, InstrumentationVersion.VERSION),
spanNameExtractor,
spanKindExtractor,
Expand All @@ -124,6 +125,7 @@ private Instrumenter<REQUEST, RESPONSE> newInstrumenter(

private interface InstrumenterConstructor<RQ, RS> {
Instrumenter<RQ, RS> create(
String instrumentationName,
Tracer tracer,
SpanNameExtractor<? super RQ> spanNameExtractor,
SpanKindExtractor<? super RQ> spanKindExtractor,
Expand All @@ -137,8 +139,15 @@ static <RQ, RS> InstrumenterConstructor<RQ, RS> internal() {

static <RQ, RS> InstrumenterConstructor<RQ, RS> propagatingToDownstream(
ContextPropagators propagators, TextMapSetter<RQ> setter) {
return (tracer, spanName, spanKind, spanStatus, attributes, errorCauseExtractor) ->
return (instrumentationName,
tracer,
spanName,
spanKind,
spanStatus,
attributes,
errorCauseExtractor) ->
new ClientInstrumenter<>(
instrumentationName,
tracer,
spanName,
spanKind,
Expand All @@ -151,8 +160,15 @@ static <RQ, RS> InstrumenterConstructor<RQ, RS> propagatingToDownstream(

static <RQ, RS> InstrumenterConstructor<RQ, RS> propagatingFromUpstream(
ContextPropagators propagators, TextMapGetter<RQ> getter) {
return (tracer, spanName, spanKind, spanStatus, attributes, errorCauseExtractor) ->
return (instrumentationName,
tracer,
spanName,
spanKind,
spanStatus,
attributes,
errorCauseExtractor) ->
new ServerInstrumenter<>(
instrumentationName,
tracer,
spanName,
spanKind,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ final class ServerInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
private final TextMapGetter<REQUEST> getter;

ServerInstrumenter(
String instrumentationName,
Tracer tracer,
SpanNameExtractor<? super REQUEST> spanNameExtractor,
SpanKindExtractor<? super REQUEST> spanKindExtractor,
Expand All @@ -26,6 +27,7 @@ final class ServerInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST,
ContextPropagators propagators,
TextMapGetter<REQUEST> getter) {
super(
instrumentationName,
tracer,
spanNameExtractor,
spanKindExtractor,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String> reporter;

private final ConcurrentMap<String, KindCounters> suppressionCounters = new ConcurrentHashMap<>();
private final ConcurrentMap<String, LongAdder> 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
Expand All @@ -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;
}
Expand All @@ -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(
Expand All @@ -55,6 +67,13 @@ void report() {
}
}
});
counters.forEach(
(counterName, counter) -> {
long value = counter.sumThenReset();
if (value > 0) {
reporter.accept("Counter '" + counterName + "' : " + value);
}
});
}

SupportabilityMetrics start() {
Expand All @@ -72,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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@
* 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;
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;
Expand All @@ -28,6 +26,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();

Expand All @@ -45,17 +46,19 @@ 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)
.hasSameElementsAs(
Arrays.asList(
"Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 2",
"Suppressed Spans by 'favoriteInstrumentation' (SERVER) : 1",
"Suppressed Spans by 'otherInstrumentation' (INTERNAL) : 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
Expand All @@ -66,14 +69,14 @@ 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)
.hasSameElementsAs(
singletonList("Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 1"));
.containsExactlyInAnyOrder(
"Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 1",
"Counter 'some counter' : 1");
}
}