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

New Instrumenter API #2596

Merged
merged 26 commits into from
Apr 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
8027c55
Instrumenter
Mar 10, 2021
b7e66a6
Spotless
Mar 10, 2021
6af20de
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 15, 2021
c1e19af
More instrumenter
Mar 16, 2021
46a9e1c
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 17, 2021
2e6db8f
More
Mar 17, 2021
be06cdb
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 18, 2021
a91ad8e
Instrumenter API
Mar 18, 2021
6c5e576
New Instrumenter API - InstrumenterBuilder (#20)
Mar 23, 2021
297f336
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 23, 2021
fbee4b0
Start doccing
Mar 23, 2021
2cd5e68
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 29, 2021
fb3038a
Docs
Mar 29, 2021
fbbe2d5
Most tests
Mar 29, 2021
cdd8078
Last test
Mar 29, 2021
fb33d75
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 29, 2021
54c3d76
Stick with current hostName lookup for now
Mar 29, 2021
243ba5b
Update testing-common/src/main/groovy/io/opentelemetry/instrumentatio…
Mar 30, 2021
5babac7
New Instrumenter API - code review comments (#21)
Mar 31, 2021
cdf7bca
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 31, 2021
b32c9a0
Doc
Mar 31, 2021
36f5fa6
Merge branch 'instrumentation-extractor' of github.com:anuraaga/opent…
Mar 31, 2021
0d5ec54
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 31, 2021
0bdd63f
Checkstyle
Apr 1, 2021
28c7d6c
Cleanups
Apr 1, 2021
cd185f6
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Apr 5, 2021
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
3 changes: 2 additions & 1 deletion gradle/enforcement/checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,8 @@
value="Method type name ''{0}'' must match pattern ''{1}''."/>
</module>
<module name="InterfaceTypeParameterName">
<property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
<!-- modified from default Google Java Style -->
<property name="format" value="^[A-Z]+[0-9]?$"/>
<message key="name.invalidPattern"
value="Interface type name ''{0}'' must match pattern ''{1}''."/>
</module>
Expand Down
1 change: 1 addition & 0 deletions instrumentation-api/instrumentation-api.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,5 @@ dependencies {
testImplementation deps.mockito
testImplementation deps.assertj
testImplementation deps.awaitility
testImplementation deps.opentelemetrySdkTesting
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.AttributesBuilder;

/**
* Extractor of {@link io.opentelemetry.api.common.Attributes} for a given request and response.
* Will be called {@linkplain #onStart(AttributesBuilder, Object) on start} with just the {@link
* REQUEST} and again {@linkplain #onEnd(AttributesBuilder, Object, Object) on end} with both {@link
* REQUEST} and {@link RESPONSE} to allow populating attributes at each stage of a request's
* lifecycle. It is best to populate as much as possible in {@link #onStart(AttributesBuilder,
* Object)} to have it available during sampling.
*
* @see HttpAttributesExtractor
* @see NetAttributesExtractor
*/
public abstract class AttributesExtractor<REQUEST, RESPONSE> {
/**
* Extracts attributes from the {@link REQUEST} into the {@link AttributesBuilder} at the
* beginning of a request.
*/
protected abstract void onStart(AttributesBuilder attributes, REQUEST request);

/**
* Extracts attributes from the {@link REQUEST} and {@link RESPONSE} into the {@link
* AttributesBuilder} at the end of a request.
*/
protected abstract void onEnd(AttributesBuilder attributes, REQUEST request, RESPONSE response);

/**
* Sets the {@code value} with the given {@code key} to the {@link AttributesBuilder} if {@code
* value} is not {@code null}.
*/
protected static <T> void set(AttributesBuilder attributes, AttributeKey<T> key, T value) {
if (value != null) {
attributes.put(key, value);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.context.propagation.TextMapSetter;
import java.util.List;

final class ClientInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST, RESPONSE> {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably rename ClientInstrumenter/ServerInstrumenter to DownstreamPropagatingInstrumenter/UpstreamPropagatingInstrumenter (also see anuraaga#20 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah - I understood the comment, I am still on the fence on it. Considering how much more common server/client is than the other forms I wonder if it's not so bad to have server / client words even when it's actually consumer / producer. Was thinking of addressing this in a follow-up when using from messaging.

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify: we can still have newServerInstrumenter(TextMapGetter), new(Upstream|Downstream)Instrumenter() would be just for the situation when you have to pass a custom SpanKindExtractor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's a tradeoff between introducing new concepts vs having precise terminology. If server and producer are close enough to be two ducks (i.e, if it quacks it's a duck) then consolidating might be ok. I'm not too sure on it yet.


private final ContextPropagators propagators;
private final TextMapSetter<REQUEST> setter;

ClientInstrumenter(
Tracer tracer,
SpanNameExtractor<? super REQUEST> spanNameExtractor,
SpanKindExtractor<? super REQUEST> spanKindExtractor,
SpanStatusExtractor<? super REQUEST, ? super RESPONSE> spanStatusExtractor,
List<? extends AttributesExtractor<? super REQUEST, ? super RESPONSE>> attributesExtractors,
ErrorCauseExtractor errorCauseExtractor,
ContextPropagators propagators,
TextMapSetter<REQUEST> setter) {
super(
tracer,
spanNameExtractor,
spanKindExtractor,
spanStatusExtractor,
attributesExtractors,
errorCauseExtractor);
this.propagators = propagators;
this.setter = setter;
}

@Override
public Context start(Context parentContext, REQUEST request) {
Context newContext = super.start(parentContext, request);
propagators.getTextMapPropagator().inject(newContext, request, setter);
return newContext;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

import io.opentelemetry.api.trace.StatusCode;
import org.checkerframework.checker.nullness.qual.Nullable;

final class DefaultSpanStatusExtractor<REQUEST, RESPONSE>
implements SpanStatusExtractor<REQUEST, RESPONSE> {

static final SpanStatusExtractor<Object, Object> INSTANCE = new DefaultSpanStatusExtractor<>();

@Override
public StatusCode extract(REQUEST request, RESPONSE response, @Nullable Throwable error) {
if (error != null) {
return StatusCode.ERROR;
}
return StatusCode.UNSET;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

/**
* Extractor of the root cause of a {@link Throwable}. When instrumenting a library which wraps user
* exceptions with a framework exception, generally for propagating checked exceptions across
* unchecked boundaries, it is recommended to override this to unwrap back to the user exception.
*/
public interface ErrorCauseExtractor {
Throwable extractCause(Throwable error);

/**
* Returns a {@link ErrorCauseExtractor} which unwraps common standard library wrapping
* exceptions.
*/
static ErrorCauseExtractor jdk() {
return JdkErrorCauseExtractor.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

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
* 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.
*/
public abstract class HttpAttributesExtractor<REQUEST, RESPONSE>
Copy link
Member

Choose a reason for hiding this comment

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

Should we put HTTP-related classes in a ...instrumenter.http subpackage? Same for DB/messaging/RPC attribute/span name extractors (in the future). The base package would contain just the base classes/interfaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm - I think it means we wouldn't have SpanStatusExtractor.http() (not in same package anymore) and just use HttpStatusExtractor.create() or something. I'm wondering about this vs extracting an attributes package with HttpAttributesExtractor, NetAttributesExtractor, etc, that does keep the public API together with some separation.

Copy link
Member

Choose a reason for hiding this comment

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

Or HttpSemanticConventions.spanName()/HttpSemanticConventions.status() in the http package

extends AttributesExtractor<REQUEST, RESPONSE> {

@Override
protected final 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));
Comment on lines +25 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

From http semantic convention:

more than the required attributes can be supplied, but this is recommended only if they cannot be inferred from the sent ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this API, we just set what was provided indeed, I don't think we can model something like that at this layer since we don't know what can be inferred, or if we do then it needs to be more explicit in the spec I guess, that spec language is way too vague to be actually implementable I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

WDYM, vague? Spec gives several options for required attribute combinations and then warns agains duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well these attributes are defined by the spec so it could say exactly which attributes should be filled. If it's the three sets of attributes and says attributes SHOULD NOT be filled from one set if another is filled, it would be clearer. I don't know either if this is a ordered list by priority. So vague ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

E.g, in that text server name is mentioned descriptively but it could just be said it should always be filled when available.

}

@Override
protected final void onEnd(AttributesBuilder attributes, REQUEST request, RESPONSE response) {
set(
attributes,
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
requestContentLength(request, response));
set(
attributes,
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED,
requestContentLengthUncompressed(request, response));
set(attributes, SemanticAttributes.HTTP_STATUS_CODE, statusCode(request, response));
set(attributes, SemanticAttributes.HTTP_FLAVOR, flavor(request, response));
set(
attributes,
SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH,
responseContentLength(request, response));
set(
attributes,
SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED,
responseContentLengthUncompressed(request, response));
set(attributes, SemanticAttributes.HTTP_SERVER_NAME, serverName(request, response));
set(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp(request, response));
}

// Attributes that always exist in a request

@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);

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

@Nullable
protected abstract Long requestContentLength(REQUEST request, RESPONSE response);

@Nullable
protected abstract Long requestContentLengthUncompressed(REQUEST request, RESPONSE response);

@Nullable
protected abstract Long statusCode(REQUEST request, RESPONSE response);

@Nullable
protected abstract String flavor(REQUEST request, RESPONSE response);

@Nullable
protected abstract Long responseContentLength(REQUEST request, RESPONSE response);

@Nullable
protected abstract Long responseContentLengthUncompressed(REQUEST request, RESPONSE response);

@Nullable
protected abstract String serverName(REQUEST request, RESPONSE response);

@Nullable
protected abstract String clientIp(REQUEST request, RESPONSE response);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

final class HttpSpanNameExtractor<REQUEST> implements SpanNameExtractor<REQUEST> {

private final HttpAttributesExtractor<REQUEST, ?> attributesExtractor;

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

@Override
public String extract(REQUEST request) {
String route = attributesExtractor.route(request);
if (route != null) {
return route;
}
String method = attributesExtractor.method(request);
if (method != null) {
return "HTTP " + method;
}
return "HTTP request";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.instrumentation.api.tracer.HttpStatusConverter;

final class HttpSpanStatusExtractor<REQUEST, RESPONSE>
implements SpanStatusExtractor<REQUEST, RESPONSE> {

private final HttpAttributesExtractor<REQUEST, RESPONSE> attributesExtractor;

protected HttpSpanStatusExtractor(
HttpAttributesExtractor<REQUEST, RESPONSE> attributesExtractor) {
this.attributesExtractor = attributesExtractor;
}

@Override
public StatusCode extract(REQUEST request, RESPONSE response, Throwable error) {
Long statusCode = attributesExtractor.statusCode(request, response);
if (statusCode != null) {
return HttpStatusConverter.statusFromHttpStatus((int) (long) statusCode);
}
return SpanStatusExtractor.getDefault().extract(request, response, error);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter;

import java.net.InetAddress;
import java.net.InetSocketAddress;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* Extractor of <a
* href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/span-general.md#general-network-connection-attributes">Network
* attributes</a> from a {@link InetSocketAddress}. Most network libraries will provide access to a
* {@link InetSocketAddress} so this is a convenient alternative to {@link NetAttributesExtractor}.
* There is no meaning to implement both in the same instrumentation.
*/
public abstract class InetSocketAddressNetAttributesExtractor<REQUEST, RESPONSE>
extends NetAttributesExtractor<REQUEST, RESPONSE> {

@Nullable
protected abstract InetSocketAddress getAddress(REQUEST request, RESPONSE response);

@Override
@Nullable
protected final String peerName(REQUEST request, RESPONSE response) {
InetSocketAddress address = getAddress(request, response);
if (address == null) {
return null;
}
if (address.getAddress() != null) {
return address.getAddress().getHostName();
}
return address.getHostString();
}

@Override
@Nullable
protected final Long peerPort(REQUEST request, RESPONSE response) {
InetSocketAddress address = getAddress(request, response);
if (address == null) {
return null;
}
return (long) address.getPort();
}

@Override
@Nullable
protected final String peerIp(REQUEST request, RESPONSE response) {
InetSocketAddress address = getAddress(request, response);
if (address == null) {
return null;
}
InetAddress remoteAddress = address.getAddress();
if (remoteAddress != null) {
return remoteAddress.getHostAddress();
}
return null;
}
}
Loading