Fix: set correct transaction status for unhandled exceptions in SentryTracingFilter.

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: set correct transaction status for unhandled exceptions in SentryTracingFilter (#1406)
 * Fix: handle network errors in SentrySpanClientHttpRequestInterceptor (#1407)
 
 # 4.4.0-alpha.2

sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/it/SentrySpringIntegrationTest.kt

@@ -31,6 +31,7 @@ import org.springframework.context.annotation.Configuration
 import org.springframework.http.HttpEntity
 import org.springframework.http.HttpHeaders
 import org.springframework.http.HttpMethod
+import org.springframework.http.HttpStatus
 import org.springframework.http.ResponseEntity
 import org.springframework.security.config.annotation.web.builders.HttpSecurity
 import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter
@@ -147,6 +148,14 @@ class SentrySpringIntegrationTest {
         }
     }
 
+    @Test
+    fun `tracing filter does not overwrite resposne status code`() {
+        val restTemplate = TestRestTemplate().withBasicAuth("user", "password")
+
+        val response = restTemplate.getForEntity("http://localhost:$port/performance", String::class.java)
+        assertThat(response.statusCode).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR)
+    }
+
     @Test
     fun `does not send events for handled exceptions`() {
         val restTemplate = TestRestTemplate().withBasicAuth("user", "password")

sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java

@@ -67,16 +67,23 @@ protected void doFilterInternal(
       final ITransaction transaction = startTransaction(httpRequest, sentryTraceHeader);
       try {
         filterChain.doFilter(httpRequest, httpResponse);
+      } catch (Exception e) {
+        // exceptions that are not handled by Spring
+        transaction.setStatus(SpanStatus.INTERNAL_ERROR);
+        throw e;
       } finally {
         // after all filters run, templated path pattern is available in request attribute
         final String transactionName = transactionNameProvider.provideTransactionName(httpRequest);
         // if transaction name is not resolved, the request has not been processed by a controller
-        // and
-        // we should not report it to Sentry
+        // and we should not report it to Sentry
         if (transactionName != null) {
           transaction.setName(transactionName);
           transaction.setOperation(TRANSACTION_OP);
-          transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus()));
+          // if exception has been thrown, transaction status is already set to INTERNAL_ERROR, and
+          // httpResponse.getStatus() returns 200.
+          if (transaction.getStatus() == null) {
+            transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus()));
+          }
           transaction.finish();
         }
       }

sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt

@@ -22,6 +22,7 @@ import javax.servlet.http.HttpServletRequest
 import kotlin.test.Test
 import kotlin.test.assertNotNull
 import kotlin.test.assertTrue
+import kotlin.test.fail
 import org.assertj.core.api.Assertions.assertThat
 import org.springframework.mock.web.MockHttpServletRequest
 import org.springframework.mock.web.MockHttpServletResponse
@@ -39,15 +40,15 @@ class SentryTracingFilterTest {
             whenever(hub.options).thenReturn(SentryOptions())
         }
 
-        fun getSut(isEnabled: Boolean = true, sentryTraceHeader: String? = null): SentryTracingFilter {
+        fun getSut(isEnabled: Boolean = true, status: Int = 200, sentryTraceHeader: String? = null): SentryTracingFilter {
             request.requestURI = "/product/12"
             request.method = "POST"
             request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/product/{id}")
             if (sentryTraceHeader != null) {
                 request.addHeader("sentry-trace", sentryTraceHeader)
                 whenever(hub.startTransaction(any(), any<CustomSamplingContext>(), eq(true))).thenAnswer { SentryTracer(it.arguments[0] as TransactionContext, hub) }
             }
-            response.status = 200
+            response.status = status
             whenever(hub.startTransaction(any(), any(), any(), eq(true))).thenAnswer { SentryTracer(TransactionContext(it.arguments[0] as String, it.arguments[1] as String), hub) }
             whenever(hub.isEnabled).thenReturn(isEnabled)
             return SentryTracingFilter(hub, transactionNameProvider)
@@ -62,7 +63,7 @@ class SentryTracingFilterTest {
 
         filter.doFilter(fixture.request, fixture.response, fixture.chain)
 
-        verify(fixture.hub).startTransaction(eq("POST /product/12"), eq("http.server"), check<CustomSamplingContext> {
+        verify(fixture.hub).startTransaction(eq("POST /product/12"), eq("http.server"), check {
             assertNotNull(it["request"])
             assertTrue(it["request"] is HttpServletRequest)
         }, eq(true))
@@ -74,6 +75,28 @@ class SentryTracingFilterTest {
         })
     }
 
+    @Test
+    fun `sets correct span status based on the response status`() {
+        val filter = fixture.getSut(status = 500)
+
+        filter.doFilter(fixture.request, fixture.response, fixture.chain)
+
+        verify(fixture.hub).captureTransaction(check {
+            assertThat(it.contexts.trace!!.status).isEqualTo(SpanStatus.INTERNAL_ERROR)
+        })
+    }
+
+    @Test
+    fun `does not set span status for response status that dont match predefined span statuses`() {
+        val filter = fixture.getSut(status = 302)
+
+        filter.doFilter(fixture.request, fixture.response, fixture.chain)
+
+        verify(fixture.hub).captureTransaction(check {
+            assertThat(it.contexts.trace!!.status).isNull()
+        })
+    }
+
     @Test
     fun `when sentry trace is not present, transaction does not have parentSpanId set`() {
         val filter = fixture.getSut()
@@ -109,4 +132,19 @@ class SentryTracingFilterTest {
         verifyNoMoreInteractions(fixture.hub)
         verifyZeroInteractions(fixture.transactionNameProvider)
     }
+
+    @Test
+    fun `sets status to internal server error when chain throws exception`() {
+        val filter = fixture.getSut()
+        whenever(fixture.chain.doFilter(any(), any())).thenThrow(RuntimeException("error"))
+
+        try {
+            filter.doFilter(fixture.request, fixture.response, fixture.chain)
+            fail("filter is expected to rethrow exception")
+        } catch (_: Exception) {
+        }
+        verify(fixture.hub).captureTransaction(check {
+            assertThat(it.status).isEqualTo(SpanStatus.INTERNAL_ERROR)
+        })
+    }
 }