Do not set default status when status is unknown.

getsentry · Apr 28, 2021 · 332d77f · 332d77f
1 parent 628a5f6
commit 332d77f
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 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
@@ -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
@@ -50,56 +50,66 @@ 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])
     }
 
     @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"])
@@ -111,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)
@@ -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)