diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d4f60c368..cb50b155ca 100644 --- 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: set correct transaction status for unhandled exceptions in SentryTracingFilter (#1406) * Fix: handle network errors in SentrySpanClientHttpRequestInterceptor (#1407) # 4.4.0-alpha.2 diff --git a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/it/SentrySpringIntegrationTest.kt b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/it/SentrySpringIntegrationTest.kt index 2efc2a6972..f3438f9e3c 100644 --- a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/it/SentrySpringIntegrationTest.kt +++ b/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") diff --git a/sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java b/sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java index 35d5ca6367..d72159fdd8 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java +++ b/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(); } } diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt index 0363b935d6..6f93461798 100644 --- a/sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt +++ b/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,7 +40,7 @@ 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}") @@ -47,7 +48,7 @@ class SentryTracingFilterTest { request.addHeader("sentry-trace", sentryTraceHeader) whenever(hub.startTransaction(any(), any(), 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 { + 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) + }) + } }