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.

Sign up for GitHub

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

Add error parameter to EndTimeExtractor and AttributesExtractor#onEnd() #3988

Merged
merged 4 commits into from
Sep 8, 2021
Merged

Add error parameter to EndTimeExtractor and AttributesExtractor#onEnd() #3988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b98bc6e
Add error parameter to EndTimeExtractor and AttributesExtractor#onEnd()
Aug 27, 2021
3bb8807
fix compilation failure
Aug 31, 2021
4ac1ea2
spotless
Aug 31, 2021
757a92a
fixed merge conflict
Sep 8, 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
5 changes: 4 additions & 1 deletion ...o/opentelemetry/instrumentation/api/annotation/support/MethodSpanAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ protected void onStart(AttributesBuilder attributes, REQUEST request) {

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

/**
* Creates a binding of the parameters of the traced method to span attributes.
Expand Down
17 changes: 10 additions & 7 deletions .../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/AttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
/**
* 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.
* REQUEST} and again {@linkplain #onEnd(AttributesBuilder, Object, Object, Throwable) 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 DbAttributesExtractor
* @see HttpAttributesExtractor
Expand All @@ -32,11 +32,14 @@ public abstract class AttributesExtractor<REQUEST, RESPONSE> {
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.
* Extracts attributes from the {@link REQUEST} and either {@link RESPONSE} or {@code error} into
* the {@link AttributesBuilder} at the end of a request.
*/
protected abstract void onEnd(
AttributesBuilder attributes, REQUEST request, @Nullable RESPONSE response);
AttributesBuilder attributes,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error);

/**
* Sets the {@code value} with the given {@code key} to the {@link AttributesBuilder} if {@code
Expand Down
2 changes: 1 addition & 1 deletion ...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@
public interface EndTimeExtractor<REQUEST, RESPONSE> {

/** Returns the timestamp marking the end of the response processing. */
Instant extract(REQUEST request, @Nullable RESPONSE response);
Instant extract(REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error);
mateuszrzeszutek marked this conversation as resolved.
Show resolved Hide resolved
}
17 changes: 8 additions & 9 deletions ...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,30 +165,29 @@ public void end(
Context context, REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error) {
Span span = Span.fromContext(context);

if (error != null) {
error = errorCauseExtractor.extractCause(error);
span.recordException(error);
}

UnsafeAttributes attributesBuilder = new UnsafeAttributes();
for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor : attributesExtractors) {
extractor.onEnd(attributesBuilder, request, response);
extractor.onEnd(attributesBuilder, request, response, error);
Comment on lines +168 to +175
Copy link
Member

Choose a reason for hiding this comment

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

wondering a little bit about passing original error vs passing the extracted error here.

do you think ErrorCauseExtractor might be used/useful for suppressing "expected" exceptions from being recorded? (and maybe still wanting to add a tag that there was an error, just wanting to suppress the big stack trace from getting recorded?)

or maybe there's a better mechanism for this use case

Copy link
Member Author

Choose a reason for hiding this comment

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

do you think ErrorCauseExtractor might be used/useful for suppressing "expected" exceptions from being recorded?

I think that'd be a misuse of the API. It was originally meant to strip out those "meaningless" exceptions that wrap over the "real" ones, like ExecutionException.

The stacktrace issue seems to be a real one though, we have #431 in our backlog. Perhaps we could think about an additional configuration knob on the InstrumenterBuilder for that.

Copy link
Member

Choose a reason for hiding this comment

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

ya, that makes sense to me 👍

}
Attributes attributes = attributesBuilder;
span.setAllAttributes(attributes);

for (RequestListener requestListener : requestListeners) {
requestListener.end(context, attributes);
}

span.setAllAttributes(attributes);

if (error != null) {
error = errorCauseExtractor.extractCause(error);
span.recordException(error);
}

StatusCode statusCode = spanStatusExtractor.extract(request, response, error);
if (statusCode != StatusCode.UNSET) {
span.setStatus(statusCode);
}

if (endTimeExtractor != null) {
span.end(endTimeExtractor.extract(request, response));
span.end(endTimeExtractor.extract(request, response, error));
} else {
span.end();
}
Expand Down
4 changes: 2 additions & 2 deletions ...-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/RequestListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ public interface RequestListener {
* the start and end of the request, e.g., an in-progress span, it should be added to the passed
* in {@link Context} and returned.
*/
Context start(Context context, Attributes requestAttributes);
Context start(Context context, Attributes startAttributes);

/** Listener method that is called at the end of a request. */
void end(Context context, Attributes responseAttributes);
void end(Context context, Attributes endAttributes);
}
5 changes: 4 additions & 1 deletion ...i/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,10 @@ protected void onStart(AttributesBuilder attributes, REQUEST request) {}

@Override
protected void onEnd(
AttributesBuilder attributes, REQUEST request, @Nullable RESPONSE response) {
AttributesBuilder attributes,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {
String clientIp = getForwardedClientIp(request);
if (clientIp == null && netAttributesExtractor != null) {
clientIp = netAttributesExtractor.peerIp(request, response);
Expand Down
5 changes: 4 additions & 1 deletion .../java/io/opentelemetry/instrumentation/api/instrumenter/code/CodeAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) {

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

@Nullable
protected abstract Class<?> codeClass(REQUEST request);
Expand Down
5 changes: 4 additions & 1 deletion ...main/java/io/opentelemetry/instrumentation/api/instrumenter/db/DbAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ protected void onStart(AttributesBuilder attributes, REQUEST request) {

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

@Nullable
protected abstract String system(REQUEST request);
Expand Down
5 changes: 4 additions & 1 deletion .../java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) {

@Override
protected final void onEnd(
AttributesBuilder attributes, REQUEST request, @Nullable RESPONSE response) {
AttributesBuilder attributes,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {
set(
attributes,
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
Expand Down
6 changes: 3 additions & 3 deletions ...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,16 @@ private HttpClientMetrics(Meter meter) {
}

@Override
public Context start(Context context, Attributes requestAttributes) {
public Context start(Context context, Attributes startAttributes) {
long startTimeNanos = System.nanoTime();

return context.with(
HTTP_CLIENT_REQUEST_METRICS_STATE,
new AutoValue_HttpClientMetrics_State(requestAttributes, startTimeNanos));
new AutoValue_HttpClientMetrics_State(startAttributes, startTimeNanos));
}

@Override
public void end(Context context, Attributes responseAttributes) {
public void end(Context context, Attributes endAttributes) {
State state = context.get(HTTP_CLIENT_REQUEST_METRICS_STATE);
if (state == null) {
logger.debug(
Expand Down
8 changes: 4 additions & 4 deletions ...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,17 @@ private HttpServerMetrics(Meter meter) {
}

@Override
public Context start(Context context, Attributes requestAttributes) {
public Context start(Context context, Attributes startAttributes) {
long startTimeNanos = System.nanoTime();
activeRequests.add(1, applyActiveRequestsView(requestAttributes));
activeRequests.add(1, applyActiveRequestsView(startAttributes));

return context.with(
HTTP_SERVER_REQUEST_METRICS_STATE,
new AutoValue_HttpServerMetrics_State(requestAttributes, startTimeNanos));
new AutoValue_HttpServerMetrics_State(startAttributes, startTimeNanos));
}

@Override
public void end(Context context, Attributes responseAttributes) {
public void end(Context context, Attributes endAttributes) {
State state = context.get(HTTP_SERVER_REQUEST_METRICS_STATE);
if (state == null) {
logger.debug(
Expand Down
5 changes: 4 additions & 1 deletion ...pentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) {

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

Expand Down
5 changes: 4 additions & 1 deletion ...in/java/io/opentelemetry/instrumentation/api/instrumenter/net/NetAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) {

@Override
protected final void onEnd(
AttributesBuilder attributes, REQUEST request, @Nullable RESPONSE response) {
AttributesBuilder attributes,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {
set(attributes, SemanticAttributes.NET_PEER_IP, peerIp(request, response));
set(attributes, SemanticAttributes.NET_PEER_NAME, peerName(request, response));
Integer peerPort = peerPort(request, response);
Expand Down
5 changes: 4 additions & 1 deletion ...in/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) {

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

Expand Down
27 changes: 22 additions & 5 deletions .../test/java/io/opentelemetry/instrumentation/api/instrumenter/AttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import io.opentelemetry.api.common.AttributesBuilder;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

class AttributesExtractorTest {
Expand All @@ -28,23 +29,39 @@ protected void onStart(AttributesBuilder attributes, Map<String, String> request

@Override
protected void onEnd(
AttributesBuilder attributes, Map<String, String> request, Map<String, String> response) {
set(attributes, AttributeKey.stringKey("food"), response.get("food"));
set(attributes, AttributeKey.stringKey("number"), request.get("number"));
AttributesBuilder attributes,
Map<String, String> request,
@Nullable Map<String, String> response,
@Nullable Throwable error) {
if (response != null) {
set(attributes, AttributeKey.stringKey("food"), response.get("food"));
set(attributes, AttributeKey.stringKey("number"), request.get("number"));
}
if (error != null) {
set(attributes, AttributeKey.stringKey("full_error_class"), error.getClass().getName());
}
}
}

@Test
void normal() {
TestAttributesExtractor extractor = new TestAttributesExtractor();

Map<String, String> request = new HashMap<>();
request.put("animal", "cat");
Map<String, String> response = new HashMap<>();
response.put("food", "pizza");
Exception error = new RuntimeException();

AttributesBuilder attributesBuilder = Attributes.builder();
extractor.onStart(attributesBuilder, request);
extractor.onEnd(attributesBuilder, request, response);
extractor.onEnd(attributesBuilder, request, response, null);
extractor.onEnd(attributesBuilder, request, null, error);

assertThat(attributesBuilder.build())
.containsOnly(attributeEntry("animal", "cat"), attributeEntry("food", "pizza"));
.containsOnly(
attributeEntry("animal", "cat"),
attributeEntry("food", "pizza"),
attributeEntry("full_error_class", "java.lang.RuntimeException"));
}
}
13 changes: 10 additions & 3 deletions ...api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.RegisterExtension;
Expand Down Expand Up @@ -80,7 +81,10 @@ protected void onStart(AttributesBuilder attributes, Map<String, String> request

@Override
protected void onEnd(
AttributesBuilder attributes, Map<String, String> request, Map<String, String> response) {
AttributesBuilder attributes,
Map<String, String> request,
Map<String, String> response,
@Nullable Throwable error) {
attributes.put("resp1", response.get("resp1"));
attributes.put("resp2", response.get("resp2"));
}
Expand All @@ -97,7 +101,10 @@ protected void onStart(AttributesBuilder attributes, Map<String, String> request

@Override
protected void onEnd(
AttributesBuilder attributes, Map<String, String> request, Map<String, String> response) {
AttributesBuilder attributes,
Map<String, String> request,
Map<String, String> response,
@Nullable Throwable error) {
attributes.put("resp3", response.get("resp3"));
attributes.put("resp2", response.get("resp2_2"));
}
Expand Down Expand Up @@ -490,7 +497,7 @@ void shouldStartSpanWithGivenStartTime() {
Instrumenter<Instant, Instant> instrumenter =
Instrumenter.<Instant, Instant>newBuilder(
otelTesting.getOpenTelemetry(), "test", request -> "test span")
.setTimeExtractors(request -> request, (request, response) -> response)
.setTimeExtractors(request -> request, (request, response, error) -> response)
.newInstrumenter();

Instant startTime = Instant.ofEpochSecond(100);
Expand Down
2 changes: 1 addition & 1 deletion ...a/io/opentelemetry/instrumentation/api/instrumenter/code/CodeAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void shouldExtractAllAttributes() {
underTest.onStart(startAttributes, request);

AttributesBuilder endAttributes = Attributes.builder();
underTest.onEnd(endAttributes, request, null);
underTest.onEnd(endAttributes, request, null, null);

// then
assertThat(startAttributes.build())
Expand Down
2 changes: 1 addition & 1 deletion .../java/io/opentelemetry/instrumentation/api/instrumenter/db/DbAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void shouldExtractAllAvailableAttributes() {
underTest.onStart(startAttributes, request);

AttributesBuilder endAttributes = Attributes.builder();
underTest.onEnd(endAttributes, request, null);
underTest.onEnd(endAttributes, request, null, null);

// then
assertThat(startAttributes.build())
Expand Down
2 changes: 1 addition & 1 deletion ...java/io/opentelemetry/instrumentation/api/instrumenter/db/SqlAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ void shouldExtractAllAttributes() {
underTest.onStart(startAttributes, request);

AttributesBuilder endAttributes = Attributes.builder();
underTest.onEnd(endAttributes, request, null);
underTest.onEnd(endAttributes, request, null, null);

// then
assertThat(startAttributes.build())
Expand Down
2 changes: 1 addition & 1 deletion ...a/io/opentelemetry/instrumentation/api/instrumenter/http/HttpAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ void normal() {
entry(SemanticAttributes.HTTP_SCHEME, "https"),
entry(SemanticAttributes.HTTP_USER_AGENT, "okhttp 3.x"));

extractor.onEnd(attributes, request, response);
extractor.onEnd(attributes, request, response, null);
assertThat(attributes.build())
.containsOnly(
entry(SemanticAttributes.HTTP_METHOD, "POST"),
Expand Down
4 changes: 2 additions & 2 deletions ...elemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ void shouldExtractAllAvailableAttributes(
underTest.onStart(startAttributes, request);

AttributesBuilder endAttributes = Attributes.builder();
underTest.onEnd(endAttributes, request, "42");
underTest.onEnd(endAttributes, request, "42", null);

// then
List<MapEntry<AttributeKey<?>, Object>> expectedEntries = new ArrayList<>();
Expand Down Expand Up @@ -168,7 +168,7 @@ void shouldExtractNoAttributesIfNoneAreAvailable() {
underTest.onStart(startAttributes, Collections.emptyMap());

AttributesBuilder endAttributes = Attributes.builder();
underTest.onEnd(endAttributes, Collections.emptyMap(), null);
underTest.onEnd(endAttributes, Collections.emptyMap(), null, null);

// then
assertThat(startAttributes.build().isEmpty()).isTrue();
Expand Down
Loading