Fix: handle network errors in SentrySpanClientHttpRequestInterceptor. (…

…#1407)
getsentry · Apr 15, 2021 · 9a20b14 · 9a20b14
1 parent 7a51e2f
commit 9a20b14
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 * Fix: use connection and read timeouts in ApacheHttpClient based transport (#1397)
 * Ref: Refactor converting HttpServletRequest to Sentry Request in Spring integration (#1387)
+* Fix: handle network errors in SentrySpanClientHttpRequestInterceptor (#1407)
 
 # 4.4.0-alpha.2
 

diff --git a/...oot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt b/...oot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt
@@ -8,6 +8,7 @@ import io.sentry.SentryOptions
 import io.sentry.SentryTracer
 import io.sentry.SpanStatus
 import io.sentry.TransactionContext
+import java.io.IOException
 import kotlin.test.Test
 import org.assertj.core.api.Assertions.assertThat
 import org.springframework.http.HttpMethod
@@ -31,23 +32,29 @@ class SentrySpanRestTemplateCustomizerTest {
             whenever(hub.options).thenReturn(sentryOptions)
         }
 
-        fun getSut(isTransactionActive: Boolean, status: HttpStatus = HttpStatus.OK): RestTemplate {
+        fun getSut(isTransactionActive: Boolean, status: HttpStatus = HttpStatus.OK, throwIOException: Boolean = false): RestTemplate {
             customizer.customize(restTemplate)
 
             if (isTransactionActive) {
                 val scope = Scope(sentryOptions)
                 scope.setTransaction(transaction)
                 whenever(hub.span).thenReturn(transaction)
 
-                mockServer.expect(MockRestRequestMatchers.requestTo("/test/123"))
+                val scenario = mockServer.expect(MockRestRequestMatchers.requestTo("/test/123"))
                     .andExpect(MockRestRequestMatchers.method(HttpMethod.GET))
                     .andExpect {
                         // must have trace id from the parent transaction and must not contain spanId from the parent transaction
                         assertThat(it.headers["sentry-trace"]!!.first()).startsWith(transaction.spanContext.traceId.toString())
                             .endsWith("-1")
                             .doesNotContain(transaction.spanContext.spanId.toString())
                     }
-                    .andRespond(MockRestResponseCreators.withStatus(status).body("OK").contentType(MediaType.APPLICATION_JSON))
+                if (throwIOException) {
+                    scenario.andRespond {
+                        throw IOException()
+                    }
+                } else {
+                    scenario.andRespond(MockRestResponseCreators.withStatus(status).body("OK").contentType(MediaType.APPLICATION_JSON))
+                }
             } else {
                 mockServer.expect(MockRestRequestMatchers.requestTo("/test/123"))
                     .andExpect(MockRestRequestMatchers.method(HttpMethod.GET))
@@ -77,7 +84,22 @@ class SentrySpanRestTemplateCustomizerTest {
     fun `when transaction is active and response code is not 2xx, creates span with error status around RestTemplate HTTP call`() {
         try {
             fixture.getSut(isTransactionActive = true, status = HttpStatus.INTERNAL_SERVER_ERROR).getForObject("/test/{id}", String::class.java, 123)
-        } catch (e: Throwable) {}
+        } catch (e: Throwable) {
+        }
+        assertThat(fixture.transaction.spans).hasSize(1)
+        val span = fixture.transaction.spans.first()
+        assertThat(span.operation).isEqualTo("http.client")
+        assertThat(span.description).isEqualTo("GET /test/123")
+        assertThat(span.status).isEqualTo(SpanStatus.INTERNAL_ERROR)
+        fixture.mockServer.verify()
+    }
+
+    @Test
+    fun `when transaction is active and throws IO exception, creates span with error status around RestTemplate HTTP call`() {
+        try {
+            fixture.getSut(isTransactionActive = true, throwIOException = true).getForObject("/test/{id}", String::class.java, 123)
+        } catch (e: Throwable) {
+        }
         assertThat(fixture.transaction.spans).hasSize(1)
         val span = fixture.transaction.spans.first()
         assertThat(span.operation).isEqualTo("http.client")

diff --git a/...spring/src/main/java/io/sentry/spring/tracing/SentrySpanClientHttpRequestInterceptor.java b/...spring/src/main/java/io/sentry/spring/tracing/SentrySpanClientHttpRequestInterceptor.java
@@ -39,8 +39,14 @@ public SentrySpanClientHttpRequestInterceptor(final @NotNull IHub hub) {
     request.getHeaders().add(sentryTraceHeader.getName(), sentryTraceHeader.getValue());
     try {
       final ClientHttpResponse response = execution.execute(request, body);
+      // handles both success and error responses
       span.setStatus(SpanStatus.fromHttpStatusCode(response.getRawStatusCode()));
       return response;
+    } catch (Exception e) {
+      // handles cases like connection errors
+      span.setThrowable(e);
+      span.setStatus(SpanStatus.INTERNAL_ERROR);
+      throw e;
     } finally {
       span.finish();
     }