Fix: Set Span status for OkHttp integration (#1447)

Fixes #1421

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)
 * Fix: Set user on transaction in Spring & Spring Boot integrations (#1443)
 
 # 4.4.0-alpha.2

sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt

@@ -30,13 +30,16 @@ class SentryOkHttpInterceptor(
             }
             response = chain.proceed(request)
             code = response.code
+            span?.status = SpanStatus.fromHttpStatusCode(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))
-
+            span?.finish()
             val breadcrumb = Breadcrumb.http(request.url.toString(), request.method, code)
             request.body?.contentLength().ifHasValidLength {
                 breadcrumb.setData("requestBodySize", it)

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,66 +40,76 @@ 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()
         }
     }
 
-    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])
     }
 
     @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])
     }
 
     @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<Breadcrumb> {
             assertEquals("http", it.type)
             assertEquals(13L, it.data["responseBodySize"])
@@ -109,7 +121,7 @@ class SentryOkHttpInterceptorTest {
     fun `adds breadcrumb when http calls results in exception`() {
         val chain = mock<Interceptor.Chain>()
         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)
@@ -120,4 +132,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(getRequest()).execute()
+        } catch (e: IOException) {}
+        val httpClientSpan = fixture.sentryTracer.children.first()
+        assertEquals(SpanStatus.INTERNAL_ERROR, httpClientSpan.status)
+        assertTrue(httpClientSpan.throwable is IOException)
+    }
 }