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

use semantic convention APIs for opentelemetry #2662

Merged
merged 7 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ jacksonVersion=2.14.3
openTracingVersion=0.33.0
zipkinReporterVersion=2.16.4
opentelemetryVersion=1.28.0
opentelemetryInstrumentationVersion=1.28.0-alpha

# gRPC
protobufGradlePluginVersion=0.9.4
Expand All @@ -73,7 +74,6 @@ assertJCoreVersion=3.24.2
hamcrestVersion=2.2
mockitoCoreVersion=4.11.0
spotbugsPluginVersion=5.0.13
opentelemetryInstrumentationVersion=1.9.2-alpha

apacheDirectoryServerVersion=1.5.7
commonsLangVersion=2.6
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,24 @@ public String name() {
return httpVersion;
}

/**
* Resolves and return the http version number as a String. e.g. : 1.1 or 2.0.
* @return the http version number string.
*/
public String fullVersion() {
if (major == 1) {
idelpivnitskiy marked this conversation as resolved.
Show resolved Hide resolved
if (minor == 1) {
return "1.1";
}
if (minor == 0) {
return "1.0";
}
} else if (major == 2 && minor == 0) {
return "2.0";
}
return major + "." + minor;
}

/**
* Determine if the protocol version is {@link #major()} is {@code 1} and trailers are supported.
* @param version The version to check.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class HttpProtocolVersionTest {
void testHttp11Constant() {
assertEquals(1, HTTP_1_1.major());
assertEquals(1, HTTP_1_1.minor());
assertEquals("1.1", HTTP_1_1.fullVersion());
assertEquals("HTTP/1.1", HTTP_1_1.toString());
assertWriteToBuffer("HTTP/1.1", HTTP_1_1);
}
Expand All @@ -47,6 +48,7 @@ void testHttp11Constant() {
void testHttp10Constant() {
assertEquals(1, HTTP_1_0.major());
assertEquals(0, HTTP_1_0.minor());
assertEquals("1.0", HTTP_1_0.fullVersion());
assertEquals("HTTP/1.0", HTTP_1_0.toString());
assertWriteToBuffer("HTTP/1.0", HTTP_1_0);
}
Expand All @@ -63,6 +65,7 @@ void testCreateNewProtocolVersionFromMajorAndMinor() {
assertEquals(9, version98.major());
assertEquals(8, version98.minor());
assertEquals("HTTP/9.8", version98.toString());
assertEquals("9.8", version98.fullVersion());
assertWriteToBuffer("HTTP/9.8", version98);
}

Expand Down
5 changes: 4 additions & 1 deletion servicetalk-opentelemetry-http/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ dependencies {

api project(":servicetalk-http-api")
api "io.opentelemetry:opentelemetry-api:$opentelemetryVersion"
implementation("io.opentelemetry.instrumentation:opentelemetry-instrumentation-api-semconv:" +
"$opentelemetryInstrumentationVersion")

implementation project(":servicetalk-annotations")
implementation project(":servicetalk-http-utils")
Expand All @@ -36,7 +38,8 @@ dependencies {
testImplementation project(":servicetalk-http-netty")
testImplementation project(":servicetalk-test-resources")
testImplementation "io.opentelemetry:opentelemetry-sdk-testing:$opentelemetryVersion"
testImplementation "io.opentelemetry.instrumentation:opentelemetry-log4j-2.13.2:$opentelemetryInstrumentationVersion"
testRuntimeOnly("io.opentelemetry.instrumentation:opentelemetry-log4j-context-data-2.17-autoconfigure:" +
"$opentelemetryInstrumentationVersion")
testImplementation "org.junit.jupiter:junit-jupiter-api"
testImplementation "org.assertj:assertj-core:$assertJCoreVersion"
testImplementation "org.mockito:mockito-core:$mockitoCoreVersion"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,31 @@
import io.servicetalk.concurrent.api.Single;
import io.servicetalk.http.api.FilterableStreamingHttpClient;
import io.servicetalk.http.api.FilterableStreamingHttpConnection;
import io.servicetalk.http.api.HttpRequestMetaData;
import io.servicetalk.http.api.HttpResponseMetaData;
import io.servicetalk.http.api.StreamingHttpClientFilter;
import io.servicetalk.http.api.StreamingHttpClientFilterFactory;
import io.servicetalk.http.api.StreamingHttpConnectionFilter;
import io.servicetalk.http.api.StreamingHttpConnectionFilterFactory;
import io.servicetalk.http.api.StreamingHttpRequest;
import io.servicetalk.http.api.StreamingHttpRequester;
import io.servicetalk.http.api.StreamingHttpResponse;
import io.servicetalk.transport.api.HostAndPort;

import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder;
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientMetrics;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;

import java.util.function.UnaryOperator;

Expand All @@ -54,17 +63,42 @@
public final class OpenTelemetryHttpRequestFilter extends AbstractOpenTelemetryFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should rename this to OpenTelemetryHttpClientFilter to have proper alignment with the service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's too late. We cannot break the contract with clients, renaming will cause a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR, but in a follow up it will be nice to rename it to OpenTelemetryHttpRequesterFilter to be consistent with all other filters. We use "requester" on the client-side because all clients and connections implement HttpRequester interface.

For backward compatibility, we can deprecate current OpenTelemetryHttpRequestFilter and link to OpenTelemetryHttpRequesterFilter as a replacement.

implements StreamingHttpClientFilterFactory, StreamingHttpConnectionFilterFactory {

private final String componentName;
private final Instrumenter<HttpRequestMetaData, HttpResponseMetaData> instrumenter;

/**
* Create a new instance.
*
* @param openTelemetry the {@link OpenTelemetry}.
* @param componentName The component name used during building new spans.
* @param openTelemetry the {@link OpenTelemetry}.
* @param componentName The component name used during building new spans.
* @param opentelemetryOptions extra options to create the opentelemetry filter.
*/
public OpenTelemetryHttpRequestFilter(final OpenTelemetry openTelemetry, String componentName) {
public OpenTelemetryHttpRequestFilter(final OpenTelemetry openTelemetry, String componentName,
OpentelemetryOptions opentelemetryOptions) {
super(openTelemetry);
this.componentName = componentName.trim();
SpanNameExtractor<HttpRequestMetaData> serverSpanNameExtractor =
HttpSpanNameExtractor.create(ServicetalkHttpClientAttributesGetter.INSTANCE);
InstrumenterBuilder<HttpRequestMetaData, HttpResponseMetaData> clientInstrumenterBuilder =
Instrumenter.builder(openTelemetry, INSTRUMENTATION_SCOPE_NAME, serverSpanNameExtractor);
clientInstrumenterBuilder.setSpanStatusExtractor(ServicetalkSpanStatusExtractor.INSTANCE);

clientInstrumenterBuilder
.addAttributesExtractor(HttpClientAttributesExtractor
.builder(ServicetalkHttpClientAttributesGetter.INSTANCE,
ServicetalkNetClientAttributesGetter.INSTANCE)
.setCapturedRequestHeaders(opentelemetryOptions.captureRequestHeaders())
.setCapturedResponseHeaders(opentelemetryOptions.captureResponseHeaders())
.build())
.addAttributesExtractor(
NetClientAttributesExtractor.create(ServicetalkNetClientAttributesGetter.INSTANCE));
if (opentelemetryOptions.enableMetrics()) {
clientInstrumenterBuilder.addOperationMetrics(HttpClientMetrics.get());
}
if (!componentName.trim().isEmpty()) {
clientInstrumenterBuilder.addAttributesExtractor(
AttributesExtractor.constant(SemanticAttributes.PEER_SERVICE, componentName));
}
instrumenter =
clientInstrumenterBuilder.buildClientInstrumenter(RequestHeadersPropagatorSetter.INSTANCE);
}

/**
Expand All @@ -73,15 +107,35 @@ public OpenTelemetryHttpRequestFilter(final OpenTelemetry openTelemetry, String
* @param componentName The component name used during building new spans.
*/
public OpenTelemetryHttpRequestFilter(String componentName) {
this(GlobalOpenTelemetry.get(), componentName);
this(GlobalOpenTelemetry.get(), componentName, OpentelemetryOptions.newBuilder().build());
idelpivnitskiy marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Create a new instance, searching for any instance of an opentelemetry available.
*
* @param openTelemetry the {@link OpenTelemetry}.
* @param componentName The component name used during building new spans.
*/
public OpenTelemetryHttpRequestFilter(final OpenTelemetry openTelemetry, String componentName) {
idelpivnitskiy marked this conversation as resolved.
Show resolved Hide resolved
this(openTelemetry, componentName, OpentelemetryOptions.newBuilder().build());
idelpivnitskiy marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Create a new instance, searching for any instance of an opentelemetry available.
*
* @param componentName The component name used during building new spans.
* @param opentelemetryOptions extra options to create the opentelemetry filter
*/
public OpenTelemetryHttpRequestFilter(String componentName, OpentelemetryOptions opentelemetryOptions) {
this(GlobalOpenTelemetry.get(), componentName, opentelemetryOptions);
}

/**
* Create a new instance, searching for any instance of an opentelemetry available,
* using the hostname as the component name.
*/
public OpenTelemetryHttpRequestFilter() {
this(GlobalOpenTelemetry.get(), "");
this(GlobalOpenTelemetry.get(), "", OpentelemetryOptions.newBuilder().build());
idelpivnitskiy marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand All @@ -108,34 +162,18 @@ public Single<StreamingHttpResponse> request(final StreamingHttpRequest request)

private Single<StreamingHttpResponse> trackRequest(final StreamingHttpRequester delegate,
final StreamingHttpRequest request) {
Context context = Context.current();
final Span span = RequestTagExtractor.reportTagsAndStart(tracer
.spanBuilder(getSpanName(request))
.setParent(context)
.setSpanKind(SpanKind.CLIENT), request);

final Scope scope = span.makeCurrent();
final ScopeTracker tracker = new ScopeTracker(scope, span);
final Context parentContext = Context.current();
Context context = instrumenter.start(parentContext, request);

final Scope scope = context.makeCurrent();
final ScopeTracker tracker = new ScopeTracker(scope, context, request, instrumenter);
Single<StreamingHttpResponse> response;
try {
propagators.getTextMapPropagator().inject(Context.current(), request.headers(),
HeadersPropagatorSetter.INSTANCE);
response = delegate.request(request);
} catch (Throwable t) {
tracker.onError(t);
return Single.failed(t);
}
return tracker.track(response);
}

private String getSpanName(StreamingHttpRequest request) {
if (!componentName.isEmpty()) {
return componentName;
}
HostAndPort hostAndPort = request.effectiveHostAndPort();
if (hostAndPort != null) {
return hostAndPort.hostName();
}
return request.requestTarget();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.servicetalk.concurrent.api.Publisher;
import io.servicetalk.concurrent.api.Single;
import io.servicetalk.http.api.HttpRequestMetaData;
import io.servicetalk.http.api.HttpResponseMetaData;
import io.servicetalk.http.api.HttpServiceContext;
import io.servicetalk.http.api.StreamingHttpRequest;
import io.servicetalk.http.api.StreamingHttpResponse;
Expand All @@ -30,10 +31,16 @@
import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder;
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerMetrics;
import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesExtractor;

import java.util.function.UnaryOperator;

Expand All @@ -52,21 +59,62 @@
*/
public final class OpenTelemetryHttpServerFilter extends AbstractOpenTelemetryFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should rename this to OpenTelemetryHttpServiceFilter to have proper alignment with the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming will break clients who are already using it.

Copy link
Member

Choose a reason for hiding this comment

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

We can introduce new names and deprecate all names (not in this PR, in a follow-up)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do in a follow up PR

implements StreamingHttpServiceFilterFactory {
private final Instrumenter<HttpRequestMetaData, HttpResponseMetaData> instrumenter;

/**
* Create a new instance.
*
* @param openTelemetry the {@link OpenTelemetry}.
* @param openTelemetry the {@link OpenTelemetry}.
* @param opentelemetryOptions extra options to create the opentelemetry filter.
*/
public OpenTelemetryHttpServerFilter(final OpenTelemetry openTelemetry) {
public OpenTelemetryHttpServerFilter(final OpenTelemetry openTelemetry, OpentelemetryOptions opentelemetryOptions) {
super(openTelemetry);
SpanNameExtractor<HttpRequestMetaData> serverSpanNameExtractor =
HttpSpanNameExtractor.create(ServicetalkHttpServerAttributesGetter.INSTANCE);
InstrumenterBuilder<HttpRequestMetaData, HttpResponseMetaData> serverInstrumenterBuilder =
Instrumenter.builder(openTelemetry, INSTRUMENTATION_SCOPE_NAME, serverSpanNameExtractor);
serverInstrumenterBuilder.setSpanStatusExtractor(ServicetalkSpanStatusExtractor.INSTANCE);

serverInstrumenterBuilder
.addAttributesExtractor(HttpServerAttributesExtractor
.builder(ServicetalkHttpServerAttributesGetter.INSTANCE,
ServicetalkNetServerAttributesGetter.INSTANCE)
.setCapturedRequestHeaders(opentelemetryOptions.captureRequestHeaders())
.setCapturedResponseHeaders(opentelemetryOptions.captureResponseHeaders())
.build())
.addAttributesExtractor(
NetServerAttributesExtractor.create(ServicetalkNetServerAttributesGetter.INSTANCE));
if (opentelemetryOptions.enableMetrics()) {
serverInstrumenterBuilder.addOperationMetrics(HttpServerMetrics.get());
}

instrumenter =
serverInstrumenterBuilder.buildServerInstrumenter(RequestHeadersPropagatorGetter.INSTANCE);
}

/**
* Create a new Instance, searching for any instance of an opentelemetry available.
*/
public OpenTelemetryHttpServerFilter() {
this(GlobalOpenTelemetry.get());
this(GlobalOpenTelemetry.get(), OpentelemetryOptions.newBuilder().build());
idelpivnitskiy marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Create a new instance.
*
* @param opentelemetryOptions extra options to create the opentelemetry filter
*/
public OpenTelemetryHttpServerFilter(OpentelemetryOptions opentelemetryOptions) {
idelpivnitskiy marked this conversation as resolved.
Show resolved Hide resolved
this(GlobalOpenTelemetry.get(), opentelemetryOptions);
}

/**
* Create a new instance.
*
* @param openTelemetry the {@link OpenTelemetry}.
*/
public OpenTelemetryHttpServerFilter(final OpenTelemetry openTelemetry) {
this(openTelemetry, OpentelemetryOptions.newBuilder().build());
}

@Override
Expand All @@ -85,26 +133,15 @@ private Single<StreamingHttpResponse> trackRequest(final StreamingHttpService de
final HttpServiceContext ctx,
final StreamingHttpRequest request,
final StreamingHttpResponseFactory responseFactory) {
final Context context = Context.root();
io.opentelemetry.context.Context tracingContext =
propagators.getTextMapPropagator().extract(context, request.headers(), HeadersPropagatorGetter.INSTANCE);

final Span span = RequestTagExtractor.reportTagsAndStart(tracer
.spanBuilder(getOperationName(request))
.setParent(tracingContext)
.setSpanKind(SpanKind.SERVER), request);
final Context parentContext = Context.current();
if (!instrumenter.shouldStart(parentContext, request)) {
return delegate.handle(ctx, request, responseFactory);
}
Context context = instrumenter.start(parentContext, request);

final Scope scope = span.makeCurrent();
final ScopeTracker tracker = new ScopeTracker(scope, span) {
@Override
protected void tagStatusCode() {
super.tagStatusCode();
if (metaData != null) {
propagators.getTextMapPropagator().inject(Context.current(), metaData.headers(),
HeadersPropagatorSetter.INSTANCE);
}
}
};
final Scope scope = context.makeCurrent();
final ScopeTracker tracker = new ScopeTracker(scope, context, request, instrumenter);
Single<StreamingHttpResponse> response;
try {
response = delegate.handle(ctx, request, responseFactory);
Expand All @@ -114,14 +151,4 @@ protected void tagStatusCode() {
}
return tracker.track(response);
}

/**
* Get the operation name to build the span with.
*
* @param metaData The {@link HttpRequestMetaData}.
* @return the operation name to build the span with.
*/
private static String getOperationName(HttpRequestMetaData metaData) {
return metaData.method().name() + ' ' + metaData.path();
}
}
Loading