Use Span.recordException for logging throwable #813

anuraaga · 2020-07-27T10:10:44Z

I found some quirks on the way

Previously, our exception assertions allowed no exception. This seems like a very fragile check, and instead I have gone though and explicitly disabled the exception test itself and we can investigate whether these are limitations of instrumentation, bugs of instrumentation, or expected in Certain instrumentation does not record exception in many tests #807
Many tests actually handle exception and convert to HTTP responses. This makes exception and error to effectively be the same test and suppressing the exception test is probably correct for them. Will confirm in Certain instrumentation does not record exception in many tests #807

Note the removal of tags from some server spans is expected - the new attributes are only for exceptions, we don't map HTTP errors onto them anymore.

I expected this PR to take a while but was even more digging than I expected :) Filed some other issues along the way.

In the process, looks like I fixed the null assertions everywhere, so fixes #806

instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy

instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy

.../io/opentelemetry/auto/instrumentation/googlehttpclient/GoogleHttpClientInstrumentation.java

trask · 2020-07-28T03:04:02Z

...p/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/BaseDecorator.java

-    StringWriter errorString = new StringWriter();
-    throwable.printStackTrace(new PrintWriter(errorString));
-    span.setAttribute(MoreAttributes.ERROR_STACK, errorString.toString());
+    span.recordException(throwable);


trask · 2020-07-28T03:06:24Z

instrumentation/grizzly-2.0/src/test/groovy/GrizzlyTest.groovy

+
+  @Override
+  boolean testException() {
+    // TODO(anuraaga): https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807


trask · 2020-07-28T03:07:28Z

instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy

-        if (endpoint.errored) {
-          "error.msg" { it == null || it == EXCEPTION.body }
-          "error.type" { it == null || it == Exception.name }
-          "error.stack" { it == null || it instanceof String }
-        }


no replacement for this?

Don't think so - the exception is recorded in the handler span so isn't on this one. Not sure if this is an issue with the instrumentation or expected though.

ah, makes sense, i think this is expected

trask · 2020-07-28T03:17:55Z

instrumentation/vertx-reactive-3.5/src/test/groovy/server/VertxRxHttpServerTest.groovy

@@ -68,6 +68,12 @@ class VertxRxHttpServerTest extends HttpServerTest<Vertx> {
    return false
  }

+  @Override
+  boolean testException() {
+    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807


just helping with the future search 😄

Suggested change

// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807

// TODO(anuraaga): https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807

trask · 2020-07-28T03:22:58Z

instrumentation/netty/netty-3.8/src/latestDepTest/groovy/Netty38ClientTest.groovy

+          event(0) {
+            eventName(SemanticAttributes.EXCEPTION_EVENT_NAME)
+            attributes {
+              "${SemanticAttributes.EXCEPTION_TYPE.key()}" errorClass.name
+              "${SemanticAttributes.EXCEPTION_MESSAGE.key()}" ~/Connection refused:( no further information:)? \/127.0.0.1:$UNUSABLE_PORT/
+              "${SemanticAttributes.EXCEPTION_STACKTRACE.key()}" String


there are a few places that look like they could use errorEvent

This one's because the message is matched using regex. I could make errorevent more powerful but didn't find enough of these places to warrant it yet.

it looks like you are using errorEvent in other places with regex?

Ah didn't even notice - message is the only one in my function without a type parameter mostly by accident :D Yeah I'll use it then

anuraaga · 2020-07-28T03:36:32Z

instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy

-        if (endpoint.errored) {
-          "error.msg" { it == null || it == EXCEPTION.body }
-          "error.type" { it == null || it == Exception.name }
-          "error.stack" { it == null || it instanceof String }
-        }


Don't think so - the exception is recorded in the handler span so isn't on this one. Not sure if this is an issue with the instrumentation or expected though.

.../io/opentelemetry/auto/instrumentation/googlehttpclient/GoogleHttpClientInstrumentation.java

instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy

anuraaga · 2020-07-28T03:38:37Z

instrumentation/netty/netty-3.8/src/latestDepTest/groovy/Netty38ClientTest.groovy

+          event(0) {
+            eventName(SemanticAttributes.EXCEPTION_EVENT_NAME)
+            attributes {
+              "${SemanticAttributes.EXCEPTION_TYPE.key()}" errorClass.name
+              "${SemanticAttributes.EXCEPTION_MESSAGE.key()}" ~/Connection refused:( no further information:)? \/127.0.0.1:$UNUSABLE_PORT/
+              "${SemanticAttributes.EXCEPTION_STACKTRACE.key()}" String


This one's because the message is matched using regex. I could make errorevent more powerful but didn't find enough of these places to warrant it yet.

instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy

anuraaga · 2020-07-28T03:40:42Z

testing-common/src/main/groovy/io/opentelemetry/auto/test/base/HttpServerTest.groovy

+        event(0) {
+          eventName(SemanticAttributes.EXCEPTION_EVENT_NAME)
+          attributes {
+            "${SemanticAttributes.EXCEPTION_TYPE.key()}" { it == null || it == Exception.name }


This one I'll update

…-instr-java into use-record-exception

instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy

…cTests.groovy Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…try-auto-instr-java into use-record-exception

Use Span.recordException

b5ba3af

anuraaga requested review from iNikem, jkwatson, trask and tylerbenson as code owners July 27, 2020 10:10

Fix tests

2059831

anuraaga force-pushed the use-record-exception branch from ad49556 to 2059831 Compare July 27, 2020 10:11

iNikem reviewed Jul 27, 2020

View reviewed changes

trask reviewed Jul 28, 2020

View reviewed changes

anuraaga commented Jul 28, 2020

View reviewed changes

Anuraag Agrawal added 2 commits July 28, 2020 13:47

Cleanup

bb53d51

Merge branch 'master' of github.com:open-telemetry/opentelemetry-auto…

1da01ba

…-instr-java into use-record-exception

trask approved these changes Jul 28, 2020

View reviewed changes

instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy Outdated Show resolved Hide resolved

Update instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasi…

90c20e9

…cTests.groovy Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

iNikem approved these changes Jul 28, 2020

View reviewed changes

Anuraag Agrawal added 3 commits July 28, 2020 14:08

Spotless (IntelliJ acts weird with groovy files for me :()

fcf849d

Merge branch 'use-record-exception' of github.com:anuraaga/openteleme…

a154bc6

…try-auto-instr-java into use-record-exception

Update for merge

d8a96cc

anuraaga merged commit f6594e1 into open-telemetry:master Jul 28, 2020

anuraaga mentioned this pull request Jul 31, 2020

Use new Span.recordException method #723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Span.recordException for logging throwable #813

Use Span.recordException for logging throwable #813

anuraaga commented Jul 27, 2020 •

edited

Loading

trask Jul 28, 2020

trask Jul 28, 2020

trask Jul 28, 2020

anuraaga Jul 28, 2020

trask Jul 28, 2020

trask Jul 28, 2020

trask Jul 28, 2020

anuraaga Jul 28, 2020

trask Jul 28, 2020

anuraaga Jul 28, 2020

anuraaga Jul 28, 2020

anuraaga Jul 28, 2020

anuraaga Jul 28, 2020

	// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807
	// TODO(anuraaga): https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/807

Use Span.recordException for logging throwable #813

Use Span.recordException for logging throwable #813

Conversation

anuraaga commented Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Jul 27, 2020 •

edited

Loading