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

Enable span suppression by SpanKey by default #5779

Merged
merged 15 commits into from
Apr 19, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.internal.SpanKey;
import io.opentelemetry.instrumentation.api.internal.SpanKeyProvider;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import java.util.List;
import java.util.function.Function;
Expand All @@ -29,7 +31,8 @@
*/
public final class HttpServerAttributesExtractor<REQUEST, RESPONSE>
extends HttpCommonAttributesExtractor<
REQUEST, RESPONSE, HttpServerAttributesGetter<REQUEST, RESPONSE>> {
REQUEST, RESPONSE, HttpServerAttributesGetter<REQUEST, RESPONSE>>
implements SpanKeyProvider {

/** Creates the HTTP server attributes extractor with default configuration. */
public static <REQUEST, RESPONSE> HttpServerAttributesExtractor<REQUEST, RESPONSE> create(
Expand Down Expand Up @@ -136,4 +139,13 @@ private String clientIp(REQUEST request) {

return null;
}

/**
* This method is internal and is hence not for public use. Its API is unstable and can change at
* any time.
*/
@Override
public SpanKey internalGetSpanKey() {
return SpanKey.HTTP_SERVER;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

package io.opentelemetry.instrumentation.api.instrumenter.rpc;

import io.opentelemetry.instrumentation.api.internal.SpanKey;
import io.opentelemetry.instrumentation.api.internal.SpanKeyProvider;

/**
* Extractor of <a
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/rpc.md">RPC
Expand All @@ -14,7 +17,7 @@
* extraction from request/response objects.
*/
public final class RpcServerAttributesExtractor<REQUEST, RESPONSE>
extends RpcCommonAttributesExtractor<REQUEST, RESPONSE> {
extends RpcCommonAttributesExtractor<REQUEST, RESPONSE> implements SpanKeyProvider {

/** Creates the RPC server attributes extractor. */
public static <REQUEST, RESPONSE> RpcServerAttributesExtractor<REQUEST, RESPONSE> create(
Expand All @@ -25,4 +28,13 @@ public static <REQUEST, RESPONSE> RpcServerAttributesExtractor<REQUEST, RESPONSE
private RpcServerAttributesExtractor(RpcAttributesGetter<REQUEST> getter) {
super(getter);
}

/**
* This method is internal and is hence not for public use. Its API is unstable and can change at
* any time.
*/
@Override
public SpanKey internalGetSpanKey() {
return SpanKey.RPC_SERVER;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,13 @@ public final class ServerSpan {
*/
@Nullable
public static Span fromContextOrNull(Context context) {
return SpanKey.SERVER.fromContextOrNull(context);
// try the HTTP semconv span first
Span span = SpanKey.HTTP_SERVER.fromContextOrNull(context);
// if it's absent then try the SERVER one - perhaps suppression by span kind is enabled
if (span == null) {
span = SpanKey.KIND_SERVER.fromContextOrNull(context);
}
return span;
trask marked this conversation as resolved.
Show resolved Hide resolved
}

private ServerSpan() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ void shouldSetRouteEvenIfSpanIsNotSampled() {

Context context = Context.root();
context = context.with(span);
context = SpanKey.SERVER.storeInContext(context, span);
context = SpanKey.HTTP_SERVER.storeInContext(context, span);
context = HttpRouteHolder.get().start(context, null, Attributes.empty());

assertNull(HttpRouteHolder.getRoute(context));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Properties;
import javax.annotation.Nullable;

/** A builder of a {@link Config}. */
public final class ConfigBuilder {
Expand All @@ -19,8 +20,10 @@ public final class ConfigBuilder {
ConfigBuilder() {}

/** Adds a single property named to the config. */
public ConfigBuilder addProperty(String name, String value) {
allProperties.put(NamingConvention.DOT.normalize(name), value);
public ConfigBuilder addProperty(String name, @Nullable String value) {
if (value != null) {
allProperties.put(NamingConvention.DOT.normalize(name), value);
}
return this;
}

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> builder
private final ErrorCauseExtractor errorCauseExtractor;
@Nullable private final TimeExtractor<REQUEST, RESPONSE> timeExtractor;
private final boolean enabled;
private final SpanSuppressionStrategy spanSuppressionStrategy;
private final SpanSuppressor spanSuppressor;

Instrumenter(InstrumenterBuilder<REQUEST, RESPONSE> builder) {
this.instrumentationName = builder.instrumentationName;
Expand All @@ -91,7 +91,7 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> builder
this.errorCauseExtractor = builder.errorCauseExtractor;
this.timeExtractor = builder.timeExtractor;
this.enabled = builder.enabled;
this.spanSuppressionStrategy = builder.buildSpanSuppressionStrategy();
this.spanSuppressor = builder.buildSpanSuppressor();
}

/**
Expand All @@ -105,7 +105,7 @@ public boolean shouldStart(Context parentContext, REQUEST request) {
return false;
}
SpanKind spanKind = spanKindExtractor.extract(request);
boolean suppressed = spanSuppressionStrategy.shouldSuppress(parentContext, spanKind);
boolean suppressed = spanSuppressor.shouldSuppress(parentContext, spanKind);

if (suppressed) {
supportability.recordSuppressedSpan(spanKind, instrumentationName);
Expand Down Expand Up @@ -162,7 +162,7 @@ public Context start(Context parentContext, REQUEST request) {
}
}

return spanSuppressionStrategy.storeInContext(context, spanKind, span);
return spanSuppressor.storeInContext(context, spanKind, span);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.api.metrics.MeterBuilder;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.api.trace.TracerBuilder;
Expand All @@ -36,10 +35,8 @@
*/
public final class InstrumenterBuilder<REQUEST, RESPONSE> {

/** Instrumentation type suppression configuration property key. */
private static final boolean ENABLE_SPAN_SUPPRESSION_BY_TYPE =
Config.get()
.getBoolean("otel.instrumentation.experimental.outgoing-span-suppression-by-type", false);
private static final SpanSuppressionStrategy spanSuppressionStrategy =
SpanSuppressionStrategy.fromConfig(Config.get());

final OpenTelemetry openTelemetry;
final String instrumentationName;
Expand All @@ -61,8 +58,6 @@ public final class InstrumenterBuilder<REQUEST, RESPONSE> {
@Nullable TimeExtractor<REQUEST, RESPONSE> timeExtractor = null;
boolean enabled = true;

private boolean enableSpanSuppressionByType = ENABLE_SPAN_SUPPRESSION_BY_TYPE;

InstrumenterBuilder(
OpenTelemetry openTelemetry,
String instrumentationName,
Expand Down Expand Up @@ -192,42 +187,6 @@ public InstrumenterBuilder<REQUEST, RESPONSE> setEnabled(boolean enabled) {
return this;
}

// visible for tests
/**
* Enables CLIENT nested span suppression based on the instrumentation type.
*
* <p><strong>When enabled:</strong>.
*
* <ul>
* <li>CLIENT nested spans are suppressed depending on their type: {@code
* HttpClientAttributesExtractor HTTP}, {@code RpcClientAttributesExtractor RPC} or {@code
* DbClientAttributesExtractor database} clients. If a span with the same type is present in
* the parent context object, new span of the same type will not be started.
* </ul>
*
* <p><strong>When disabled:</strong>
*
* <ul>
* <li>CLIENT nested spans are always suppressed.
* </ul>
*
* <p>For all other {@linkplain SpanKind span kinds} the suppression rules are as follows:
*
* <ul>
* <li>SERVER nested spans are always suppressed. If a SERVER span is present in the parent
* context object, new SERVER span will not be started.
* <li>Messaging (PRODUCER and CONSUMER) nested spans are suppressed depending on their {@code
* MessageOperation operation}. If a span with the same operation is present in the parent
* context object, new span with the same operation will not be started.
* <li>INTERNAL spans are never suppressed.
* </ul>
*/
InstrumenterBuilder<REQUEST, RESPONSE> enableInstrumentationTypeSuppression(
boolean enableInstrumentationType) {
this.enableSpanSuppressionByType = enableInstrumentationType;
return this;
}

/**
* Returns a new {@link Instrumenter} which will create client spans and inject context into
* requests.
Expand Down Expand Up @@ -327,13 +286,8 @@ List<RequestListener> buildRequestListeners() {
return listeners;
}

SpanSuppressionStrategy buildSpanSuppressionStrategy() {
Set<SpanKey> spanKeys = getSpanKeysFromAttributesExtractors();
if (enableSpanSuppressionByType) {
return SpanSuppressionStrategy.from(spanKeys);
}
// if not enabled, preserve current behavior, not distinguishing CLIENT instrumentation types
return SpanSuppressionStrategy.suppressNestedClients(spanKeys);
SpanSuppressor buildSpanSuppressor() {
return spanSuppressionStrategy.create(getSpanKeysFromAttributesExtractors());
}

private Set<SpanKey> getSpanKeysFromAttributesExtractors() {
Expand Down

This file was deleted.

Loading