From 628a5f6156d2ca80e02d0de073b2fb305399b45b Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Wed, 28 Apr 2021 13:18:36 +0200 Subject: [PATCH 1/3] Fix: Set Span status for OkHttp integration Fixes #1421 --- .../android/okhttp/SentryOkHttpInterceptor.kt | 5 ++++- .../okhttp/SentryOkHttpInterceptorTest.kt | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt b/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt index fe33aaac19..a5feb631ac 100644 --- a/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt +++ b/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt @@ -32,7 +32,10 @@ class SentryOkHttpInterceptor( code = response.code return response } catch (e: IOException) { - span?.throwable = e + span?.apply { + this.throwable = e + this.status = SpanStatus.INTERNAL_ERROR + } throw e } finally { span?.finish(SpanStatus.fromHttpStatusCode(code, SpanStatus.INTERNAL_ERROR)) diff --git a/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt b/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt index 032b525c58..53b74e5cdc 100644 --- a/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt +++ b/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt @@ -17,6 +17,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.test.assertTrue import kotlin.test.fail import okhttp3.Interceptor import okhttp3.MediaType.Companion.toMediaType @@ -25,6 +26,7 @@ import okhttp3.Request import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.SocketPolicy class SentryOkHttpInterceptorTest { @@ -38,11 +40,11 @@ class SentryOkHttpInterceptorTest { whenever(hub.options).thenReturn(SentryOptions()) } - fun getSut(isSpanActive: Boolean = true, httpStatusCode: Int = 201, responseBody: String = "success"): OkHttpClient { + fun getSut(isSpanActive: Boolean = true, httpStatusCode: Int = 201, responseBody: String = "success", socketPolicy: SocketPolicy = SocketPolicy.KEEP_OPEN): OkHttpClient { if (isSpanActive) { whenever(hub.span).thenReturn(sentryTracer) } - server.enqueue(MockResponse().setBody(responseBody).setResponseCode(httpStatusCode)) + server.enqueue(MockResponse().setBody(responseBody).setSocketPolicy(socketPolicy).setResponseCode(httpStatusCode)) server.start() return OkHttpClient.Builder().addInterceptor(interceptor).build() } @@ -120,4 +122,15 @@ class SentryOkHttpInterceptorTest { assertEquals("http", it.type) }) } + + @Test + fun `sets status and throwable when call results in IOException`() { + val sut = fixture.getSut(socketPolicy = SocketPolicy.DISCONNECT_AT_START) + try { + sut.newCall(Request.Builder().get().url(fixture.server.url("/hello")).build()).execute() + } catch (e: IOException) {} + val httpClientSpan = fixture.sentryTracer.children.first() + assertEquals(SpanStatus.INTERNAL_ERROR, httpClientSpan.status) + assertTrue(httpClientSpan.throwable is IOException) + } } From 332d77f446d18bac91b0e1a6dfa9c772261dbd00 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Wed, 28 Apr 2021 13:54:19 +0200 Subject: [PATCH 2/3] Do not set default status when status is unknown. --- .../android/okhttp/SentryOkHttpInterceptor.kt | 4 +-- .../okhttp/SentryOkHttpInterceptorTest.kt | 34 ++++++++++++------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt b/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt index a5feb631ac..9535c9703d 100644 --- a/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt +++ b/sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt @@ -30,6 +30,7 @@ class SentryOkHttpInterceptor( } response = chain.proceed(request) code = response.code + span?.status = SpanStatus.fromHttpStatusCode(code) return response } catch (e: IOException) { span?.apply { @@ -38,8 +39,7 @@ class SentryOkHttpInterceptor( } throw e } finally { - span?.finish(SpanStatus.fromHttpStatusCode(code, SpanStatus.INTERNAL_ERROR)) - + span?.finish() val breadcrumb = Breadcrumb.http(request.url.toString(), request.method, code) request.body?.contentLength().ifHasValidLength { breadcrumb.setData("requestBodySize", it) diff --git a/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt b/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt index 53b74e5cdc..38cab58b84 100644 --- a/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt +++ b/sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt @@ -50,12 +50,15 @@ class SentryOkHttpInterceptorTest { } } - val fixture = Fixture() + private val fixture = Fixture() + + private val getRequest = { Request.Builder().get().url(fixture.server.url("/hello")).build() } + private val postRequest = { Request.Builder().post("request-body".toRequestBody("text/plain".toMediaType())).url(fixture.server.url("/hello")).build() } @Test fun `when there is an active span, adds sentry trace header to the request`() { val sut = fixture.getSut() - sut.newCall(Request.Builder().get().url(fixture.server.url("/hello")).build()).execute() + sut.newCall(getRequest()).execute() val recorderRequest = fixture.server.takeRequest() assertNotNull(recorderRequest.headers[SentryTraceHeader.SENTRY_TRACE_HEADER]) } @@ -63,7 +66,7 @@ class SentryOkHttpInterceptorTest { @Test fun `when there is no active span, does not add sentry trace header to the request`() { val sut = fixture.getSut(isSpanActive = false) - sut.newCall(Request.Builder().get().url(fixture.server.url("/hello")).build()).execute() + sut.newCall(getRequest()).execute() val recorderRequest = fixture.server.takeRequest() assertNull(recorderRequest.headers[SentryTraceHeader.SENTRY_TRACE_HEADER]) } @@ -71,35 +74,42 @@ class SentryOkHttpInterceptorTest { @Test fun `does not overwrite response body`() { val sut = fixture.getSut() - val response = sut.newCall(Request.Builder().get().url(fixture.server.url("/hello")).build()).execute() + val response = sut.newCall(getRequest()).execute() assertEquals("success", response.body?.string()) } @Test fun `creates a span around the request`() { val sut = fixture.getSut() - val url = fixture.server.url("/hello") - sut.newCall(Request.Builder().get().url(url).build()).execute() + val request = getRequest() + sut.newCall(request).execute() assertEquals(1, fixture.sentryTracer.children.size) val httpClientSpan = fixture.sentryTracer.children.first() assertEquals("http.client", httpClientSpan.operation) - assertEquals("GET $url", httpClientSpan.description) + assertEquals("GET ${request.url}", httpClientSpan.description) assertEquals(SpanStatus.OK, httpClientSpan.status) } @Test fun `maps http status code to SpanStatus`() { val sut = fixture.getSut(httpStatusCode = 400) - val url = fixture.server.url("/hello") - sut.newCall(Request.Builder().get().url(url).build()).execute() + sut.newCall(getRequest()).execute() val httpClientSpan = fixture.sentryTracer.children.first() assertEquals(SpanStatus.INVALID_ARGUMENT, httpClientSpan.status) } + @Test + fun `does not map unmapped http status code to SpanStatus`() { + val sut = fixture.getSut(httpStatusCode = 502) + sut.newCall(getRequest()).execute() + val httpClientSpan = fixture.sentryTracer.children.first() + assertNull(httpClientSpan.status) + } + @Test fun `adds breadcrumb when http calls succeeds`() { val sut = fixture.getSut(responseBody = "response body") - sut.newCall(Request.Builder().post("request-body".toRequestBody("text/plain".toMediaType())).url(fixture.server.url("/hello")).build()).execute() + sut.newCall(postRequest()).execute() verify(fixture.hub).addBreadcrumb(check { assertEquals("http", it.type) assertEquals(13L, it.data["responseBodySize"]) @@ -111,7 +121,7 @@ class SentryOkHttpInterceptorTest { fun `adds breadcrumb when http calls results in exception`() { val chain = mock() whenever(chain.proceed(any())).thenThrow(IOException()) - whenever(chain.request()).thenReturn(Request.Builder().get().url(fixture.server.url("/hello")).build()) + whenever(chain.request()).thenReturn(getRequest()) try { fixture.interceptor.intercept(chain) @@ -127,7 +137,7 @@ class SentryOkHttpInterceptorTest { fun `sets status and throwable when call results in IOException`() { val sut = fixture.getSut(socketPolicy = SocketPolicy.DISCONNECT_AT_START) try { - sut.newCall(Request.Builder().get().url(fixture.server.url("/hello")).build()).execute() + sut.newCall(getRequest()).execute() } catch (e: IOException) {} val httpClientSpan = fixture.sentryTracer.children.first() assertEquals(SpanStatus.INTERNAL_ERROR, httpClientSpan.status) From c4ad84312bcf191f068e1c6dd7f77783e27759f4 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Wed, 28 Apr 2021 13:56:03 +0200 Subject: [PATCH 3/3] Changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f5b9eb938..3aaa6ec96f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * Fix: Pass maxBreadcrumbs config. to sentry-native (#1425) * Fix: Run event processors and enrich transactions with contexts (#1430) * Bump: sentry-native to 0.4.9 (#1431) +* Fix: Set Span status for OkHttp integration (#1447) # 4.4.0-alpha.2