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

Separate HTTP client/server AttributesExtractors #4195

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 @@ -8,7 +8,7 @@
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.InetSocketAddressNetAttributesExtractor;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
Expand Down Expand Up @@ -54,8 +54,8 @@ public Context startEnd() {
return context;
}

static class ConstantHttpAttributesExtractor extends HttpAttributesExtractor<Void, Void> {
static final HttpAttributesExtractor<Void, Void> INSTANCE =
static class ConstantHttpAttributesExtractor extends HttpClientAttributesExtractor<Void, Void> {
static final HttpClientAttributesExtractor<Void, Void> INSTANCE =
new ConstantHttpAttributesExtractor();

@Override
Expand All @@ -78,11 +78,6 @@ static class ConstantHttpAttributesExtractor extends HttpAttributesExtractor<Voi
return "opentelemetry.io";
}

@Override
protected @Nullable String route(Void unused) {
return "/benchmark";
}

@Override
protected @Nullable String scheme(Void unused) {
return "https";
Expand All @@ -108,11 +103,6 @@ static class ConstantHttpAttributesExtractor extends HttpAttributesExtractor<Voi
return SemanticAttributes.HttpFlavorValues.HTTP_2_0;
}

@Override
protected @Nullable String serverName(Void unused, @Nullable Void unused2) {
return null;
}

@Override
protected @Nullable Integer statusCode(Void unused, Void unused2) {
return 200;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.instrumentation.api.instrumenter.db.DbAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetAttributesExtractor;
import org.checkerframework.checker.nullness.qual.Nullable;

Expand All @@ -21,7 +21,7 @@
* #onStart(AttributesBuilder, Object)} to have it available during sampling.
*
* @see DbAttributesExtractor
* @see HttpAttributesExtractor
* @see HttpClientAttributesExtractor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @see HttpClientAttributesExtractor
* @see HttpClientAttributesExtractor
* @see HttpServerAttributesExtractor

* @see NetAttributesExtractor
*/
public abstract class AttributesExtractor<REQUEST, RESPONSE> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import io.opentelemetry.instrumentation.api.annotations.UnstableApi;
import io.opentelemetry.instrumentation.api.config.Config;
import io.opentelemetry.instrumentation.api.instrumenter.db.DbAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessagingAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.rpc.RpcAttributesExtractor;
import java.util.ArrayList;
Expand Down Expand Up @@ -161,9 +161,9 @@ public InstrumenterBuilder<REQUEST, RESPONSE> setDisabled(boolean disabled) {
*
* <ul>
* <li>CLIENT nested spans are suppressed depending on their type: {@linkplain
* HttpAttributesExtractor HTTP}, {@linkplain RpcAttributesExtractor RPC} or {@linkplain
* DbAttributesExtractor 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.
* HttpClientAttributesExtractor HTTP}, {@linkplain RpcAttributesExtractor RPC} or
* {@linkplain DbAttributesExtractor 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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.context.propagation.TextMapGetter;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerAttributesExtractor;
import io.opentelemetry.instrumentation.api.internal.ContextPropagationDebug;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import org.checkerframework.checker.nullness.qual.Nullable;
Expand All @@ -36,11 +36,11 @@ public Context start(Context parentContext, REQUEST request) {

private static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> addClientIpExtractor(
InstrumenterBuilder<REQUEST, RESPONSE> builder, TextMapGetter<REQUEST> getter) {
HttpAttributesExtractor<REQUEST, RESPONSE> httpAttributesExtractor = null;
HttpServerAttributesExtractor<REQUEST, RESPONSE> httpAttributesExtractor = null;
for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor :
builder.attributesExtractors) {
if (extractor instanceof HttpAttributesExtractor) {
httpAttributesExtractor = (HttpAttributesExtractor<REQUEST, RESPONSE>) extractor;
if (extractor instanceof HttpServerAttributesExtractor) {
httpAttributesExtractor = (HttpServerAttributesExtractor<REQUEST, RESPONSE>) extractor;
}
}
if (httpAttributesExtractor == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.api.instrumenter;

import io.opentelemetry.instrumentation.api.instrumenter.db.DbAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessagingAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.rpc.RpcAttributesExtractor;
import java.util.HashSet;
Expand All @@ -23,7 +23,7 @@ static Set<SpanKey> determineSpanKeys(
List<? extends AttributesExtractor<?, ?>> attributesExtractors) {
Set<SpanKey> spanKeys = new HashSet<>();
for (AttributesExtractor<?, ?> attributeExtractor : attributesExtractors) {
if (attributeExtractor instanceof HttpAttributesExtractor) {
if (attributeExtractor instanceof HttpClientAttributesExtractor) {
spanKeys.add(SpanKey.HTTP_CLIENT);
} else if (attributeExtractor instanceof RpcAttributesExtractor) {
spanKeys.add(SpanKey.RPC_CLIENT);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

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

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* Extractor of <a
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-client">HTTP
* client attributes</a>. Instrumentation of HTTP client frameworks should extend this class,
* defining {@link REQUEST} and {@link RESPONSE} with the actual request / response types of the
* instrumented library. If an attribute is not available in this library, it is appropriate to
* return {@code null} from the protected attribute methods, but implement as many as possible for
* best compliance with the OpenTelemetry specification.
*/
public abstract class HttpClientAttributesExtractor<REQUEST, RESPONSE>
extends HttpCommonAttributesExtractor<REQUEST, RESPONSE> {

@Override
protected final void onStart(AttributesBuilder attributes, REQUEST request) {
super.onStart(attributes, request);
set(attributes, SemanticAttributes.HTTP_URL, url(request));

// TODO: these are specific to servers, should we remove those?
set(attributes, SemanticAttributes.HTTP_TARGET, target(request));
set(attributes, SemanticAttributes.HTTP_HOST, host(request));
set(attributes, SemanticAttributes.HTTP_SCHEME, scheme(request));
Comment on lines +28 to +32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for removing these. We can always add them later if there's really a compelling reason.

cc: @lmolkova

}

@Override
protected final void onEnd(
AttributesBuilder attributes,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {
super.onEnd(attributes, request, response, error);
}

// Attributes that always exist in a request

@Nullable
protected abstract String url(REQUEST request);

// TODO: this is specific to servers, should we remove this?
@Nullable
protected abstract String target(REQUEST request);

// TODO: this is specific to servers, should we remove this?
@Nullable
protected abstract String host(REQUEST request);

// TODO: this is specific to servers, should we remove this?
@Nullable
protected abstract String scheme(REQUEST request);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,28 @@

/**
* Extractor of <a
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md">HTTP
* attributes</a>. Instrumentation of HTTP server or client frameworks should extend this class,
* defining {@link REQUEST} and {@link RESPONSE} with the actual request / response types of the
* instrumented library. If an attribute is not available in this library, it is appropriate to
* return {@code null} from the protected attribute methods, but implement as many as possible for
* best compliance with the OpenTelemetry specification.
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#common-attributes">HTTP
* attributes</a> that are common to client and server instrumentations.
*/
public abstract class HttpAttributesExtractor<REQUEST, RESPONSE>
public abstract class HttpCommonAttributesExtractor<REQUEST, RESPONSE>
extends AttributesExtractor<REQUEST, RESPONSE> {

// directly extending this class should not be possible outside of this package
HttpCommonAttributesExtractor() {}

@Override
protected final void onStart(AttributesBuilder attributes, REQUEST request) {
protected void onStart(AttributesBuilder attributes, REQUEST request) {
set(attributes, SemanticAttributes.HTTP_METHOD, method(request));
set(attributes, SemanticAttributes.HTTP_URL, url(request));
set(attributes, SemanticAttributes.HTTP_TARGET, target(request));
set(attributes, SemanticAttributes.HTTP_HOST, host(request));
set(attributes, SemanticAttributes.HTTP_ROUTE, route(request));
set(attributes, SemanticAttributes.HTTP_SCHEME, scheme(request));
set(attributes, SemanticAttributes.HTTP_USER_AGENT, userAgent(request));
}

@Override
protected final void onEnd(
protected void onEnd(
AttributesBuilder attributes,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {

set(
attributes,
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
Expand All @@ -50,7 +45,6 @@ protected final void onEnd(
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED,
requestContentLengthUncompressed(request, response));
set(attributes, SemanticAttributes.HTTP_FLAVOR, flavor(request, response));
set(attributes, SemanticAttributes.HTTP_SERVER_NAME, serverName(request, response));
if (response != null) {
Integer statusCode = statusCode(request, response);
if (statusCode != null) {
Expand All @@ -72,21 +66,6 @@ protected final void onEnd(
@Nullable
protected abstract String method(REQUEST request);

@Nullable
protected abstract String url(REQUEST request);

@Nullable
protected abstract String target(REQUEST request);

@Nullable
protected abstract String host(REQUEST request);

@Nullable
protected abstract String route(REQUEST request);

@Nullable
protected abstract String scheme(REQUEST request);

@Nullable
protected abstract String userAgent(REQUEST request);

Expand Down Expand Up @@ -120,15 +99,6 @@ protected abstract Long requestContentLengthUncompressed(
@Nullable
protected abstract String flavor(REQUEST request, @Nullable RESPONSE response);

/**
* Extracts the {@code http.server_name} span attribute.
*
* <p>This is called from {@link Instrumenter#end(Context, Object, Object, Throwable)}, whether
* {@code response} is {@code null} or not.
*/
@Nullable
protected abstract String serverName(REQUEST request, @Nullable RESPONSE response);

/**
* Extracts the {@code http.status_code} span attribute.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

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

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* Extractor of <a
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-server">HTTP
* server attributes</a>. Instrumentation of HTTP server frameworks should extend this class,
* defining {@link REQUEST} and {@link RESPONSE} with the actual request / response types of the
* instrumented library. If an attribute is not available in this library, it is appropriate to
* return {@code null} from the protected attribute methods, but implement as many as possible for
* best compliance with the OpenTelemetry specification.
*/
public abstract class HttpServerAttributesExtractor<REQUEST, RESPONSE>
extends HttpCommonAttributesExtractor<REQUEST, RESPONSE> {

@Override
protected final void onStart(AttributesBuilder attributes, REQUEST request) {
super.onStart(attributes, request);

set(attributes, SemanticAttributes.HTTP_SCHEME, scheme(request));
set(attributes, SemanticAttributes.HTTP_HOST, host(request));
set(attributes, SemanticAttributes.HTTP_TARGET, target(request));
set(attributes, SemanticAttributes.HTTP_ROUTE, route(request));

// TODO: this is specific to clients, should we remove this?
set(attributes, SemanticAttributes.HTTP_URL, url(request));
Comment on lines +34 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, +1 for removing this and we can always add it later if there's really a compelling reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that we can remove this. I arbitrary took a couple of server instrumentation's extractor, and they all returned null for attributes above

}

@Override
protected final void onEnd(
AttributesBuilder attributes,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {

super.onEnd(attributes, request, response, error);
set(attributes, SemanticAttributes.HTTP_SERVER_NAME, serverName(request, response));
}

// Attributes that always exist in a request

// TODO: this is specific to clients, should we remove this?
@Nullable
protected abstract String url(REQUEST request);

@Nullable
protected abstract String target(REQUEST request);

@Nullable
protected abstract String host(REQUEST request);

@Nullable
protected abstract String route(REQUEST request);

@Nullable
protected abstract String scheme(REQUEST request);

// Attributes which are not always available when the request is ready.

/**
* Extracts the {@code http.server_name} span attribute.
*
* <p>This is called from {@link Instrumenter#end(Context, Object, Object, Throwable)}, whether
* {@code response} is {@code null} or not.
*/
@Nullable
protected abstract String serverName(REQUEST request, @Nullable RESPONSE response);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.instrumentation.api.instrumenter.http;

import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* Extractor of the <a
Expand All @@ -20,19 +21,19 @@ public final class HttpSpanNameExtractor<REQUEST> implements SpanNameExtractor<R
* will be examined to determine the name of the span.
*/
public static <REQUEST> SpanNameExtractor<REQUEST> create(
HttpAttributesExtractor<REQUEST, ?> attributesExtractor) {
HttpCommonAttributesExtractor<REQUEST, ?> attributesExtractor) {
return new HttpSpanNameExtractor<>(attributesExtractor);
}

private final HttpAttributesExtractor<REQUEST, ?> attributesExtractor;
private final HttpCommonAttributesExtractor<REQUEST, ?> attributesExtractor;

private HttpSpanNameExtractor(HttpAttributesExtractor<REQUEST, ?> attributesExtractor) {
private HttpSpanNameExtractor(HttpCommonAttributesExtractor<REQUEST, ?> attributesExtractor) {
this.attributesExtractor = attributesExtractor;
}

@Override
public String extract(REQUEST request) {
String route = attributesExtractor.route(request);
String route = extractRoute(request);
if (route != null) {
return route;
}
Expand All @@ -42,4 +43,12 @@ public String extract(REQUEST request) {
}
return "HTTP request";
}

@Nullable
private String extractRoute(REQUEST request) {
if (attributesExtractor instanceof HttpServerAttributesExtractor) {
return ((HttpServerAttributesExtractor<REQUEST, ?>) attributesExtractor).route(request);
}
return null;
}
Comment on lines +48 to +53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth HttpClientSpanNameExtractor and HttpServerSpanNameExtractor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe we could have different factory methods in HttpSpanNameExtractor? One accepting the client extractor, the other the server one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instanceof seems OK too from what I can tell

}
Loading