Update okhttp span/breadcrumbs in case interceptors change the request

Author: markushi

sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpEvent.kt

@@ -28,28 +28,59 @@ internal class SentryOkHttpEvent(private val scopes: IScopes, private val reques
     private var response: Response? = null
     private var clientErrorResponse: Response? = null
     private val isEventFinished = AtomicBoolean(false)
-    private val url: String
-    private val method: String
+    private var url: String
+    private var method: String
 
     init {
         val urlDetails = UrlUtils.parse(request.url.toString())
         url = urlDetails.urlOrFallback
-        val host: String = request.url.host
-        val encodedPath: String = request.url.encodedPath
         method = request.method
 
         // We start the call span that will contain all the others
         val parentSpan = if (Platform.isAndroid()) scopes.transaction else scopes.span
-        callSpan = parentSpan?.startChild("http.client", "$method $url")
+        callSpan = parentSpan?.startChild("http.client")
         callSpan?.spanContext?.origin = TRACE_ORIGIN
+
+        breadcrumb = Breadcrumb().apply {
+            type = "http"
+            category = "http"
+            // needs this as unix timestamp for rrweb
+            setData(
+                SpanDataConvention.HTTP_START_TIMESTAMP,
+                CurrentDateProvider.getInstance().currentTimeMillis
+            )
+        }
+
+        setRequest(request)
+    }
+
+    /**
+     * Sets the request.
+     * This function may be called multiple times in case the request changes e.g. due to interceptors.
+     */
+    fun setRequest(request: Request) {
+        val urlDetails = UrlUtils.parse(request.url.toString())
+        url = urlDetails.urlOrFallback
+
+        val host: String = request.url.host
+        val encodedPath: String = request.url.encodedPath
+        method = request.method
+
+        callSpan?.description = "$method $url"
         urlDetails.applyToSpan(callSpan)
 
-        // We setup a breadcrumb with all meaningful data
-        breadcrumb = Breadcrumb.http(url, method)
         breadcrumb.setData("host", host)
         breadcrumb.setData("path", encodedPath)
-        // needs this as unix timestamp for rrweb
-        breadcrumb.setData(SpanDataConvention.HTTP_START_TIMESTAMP, CurrentDateProvider.getInstance().currentTimeMillis)
+        if (urlDetails.url != null) {
+            breadcrumb.setData("url", urlDetails.url!!)
+        }
+        breadcrumb.setData("method", method.uppercase())
+        if (urlDetails.query != null) {
+            breadcrumb.setData("http.query", urlDetails.query!!)
+        }
+        if (urlDetails.fragment != null) {
+            breadcrumb.setData("http.fragment", urlDetails.fragment!!)
+        }
 
         // We add the same data to the call span
         callSpan?.setData("url", url)

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

@@ -81,6 +81,10 @@ public open class SentryOkHttpInterceptor(
             val parentSpan = if (Platform.isAndroid()) scopes.transaction else scopes.span
             span = parentSpan?.startChild("http.client", "$method $url")
         }
+
+        // interceptors may change the request details, so let's update it here
+        // this only works correctly if SentryOkHttpInterceptor is the last one in the chain
+        okHttpEvent?.setRequest(request)
         val startTimestamp = CurrentDateProvider.getInstance().currentTimeMillis
 
         span?.spanContext?.origin = TRACE_ORIGIN

sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpEventTest.kt

@@ -17,6 +17,7 @@ import io.sentry.exception.SentryHttpClientException
 import io.sentry.test.getProperty
 import okhttp3.Protocol
 import okhttp3.Request
+import okhttp3.RequestBody.Companion.toRequestBody
 import okhttp3.Response
 import okhttp3.mockwebserver.MockWebServer
 import org.mockito.kotlin.any
@@ -234,6 +235,35 @@ class SentryOkHttpEventTest {
         )
     }
 
+    @Test
+    fun `setRequest updates both breadcrumb and span data`() {
+        val sut = fixture.getSut()
+
+        sut.setRequest(
+            Request.Builder()
+                .post("".toRequestBody())
+                .url("https://foo.bar/updated")
+                .build()
+        )
+        sut.finish()
+
+        verify(fixture.scopes).addBreadcrumb(
+            check<Breadcrumb> {
+                assertEquals("https://foo.bar/updated", it.data["url"])
+                assertEquals("foo.bar", it.data["host"])
+                assertEquals("/updated", it.data["path"])
+                assertEquals("POST", it.data["method"])
+            },
+            any()
+        )
+
+        assertNotNull(sut.callSpan)
+        assertEquals("/updated", sut.callSpan.getData("path"))
+        assertEquals("POST", sut.callSpan.getData("http.request.method"))
+        assertEquals("foo.bar", sut.callSpan.getData("host"))
+        assertEquals("https://foo.bar/updated", sut.callSpan.getData("url"))
+    }
+
     @Test
     fun `when finish multiple times, only one breadcrumb is captured`() {
         val sut = fixture.getSut()

sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt

@@ -21,12 +21,14 @@ import io.sentry.TransactionContext
 import io.sentry.TypeCheckHint
 import io.sentry.exception.SentryHttpClientException
 import io.sentry.mockServerRequestTimeoutMillis
+import okhttp3.EventListener
 import okhttp3.Interceptor
 import okhttp3.MediaType.Companion.toMediaType
 import okhttp3.OkHttpClient
 import okhttp3.Request
 import okhttp3.RequestBody
 import okhttp3.RequestBody.Companion.toRequestBody
+import okhttp3.Response
 import okhttp3.mockwebserver.MockResponse
 import okhttp3.mockwebserver.MockWebServer
 import okhttp3.mockwebserver.SocketPolicy
@@ -75,6 +77,8 @@ class SentryOkHttpInterceptorTest {
                 )
             ),
             sendDefaultPii: Boolean = false,
+            eventListener: EventListener? = null,
+            additionalInterceptors: List<Interceptor> = emptyList(),
             optionsConfiguration: Sentry.OptionsConfiguration<SentryOptions>? = null
         ): OkHttpClient {
             options = SentryOptions().also {
@@ -120,7 +124,15 @@ class SentryOkHttpInterceptorTest {
                     failedRequestStatusCodes = failedRequestStatusCodes
                 )
             }
-            return OkHttpClient.Builder().addInterceptor(interceptor).build()
+            return OkHttpClient.Builder().apply {
+                if (eventListener != null) {
+                    eventListener(eventListener)
+                }
+                for (additionalInterceptor in additionalInterceptors) {
+                    addInterceptor(additionalInterceptor)
+                }
+                addInterceptor(interceptor)
+            }.build()
         }
     }
 
@@ -613,4 +625,29 @@ class SentryOkHttpInterceptorTest {
         call.execute()
         verify(event).finish()
     }
+
+    @Test
+    fun `when an interceptor changes the request, the event is updated correctly`() {
+        val client = fixture.getSut(
+            eventListener = SentryOkHttpEventListener(fixture.scopes),
+            additionalInterceptors = listOf(
+                object : Interceptor {
+                    override fun intercept(chain: Interceptor.Chain): Response {
+                        return chain.proceed(
+                            chain.request().newBuilder()
+                                .url(chain.request().url.newBuilder().addPathSegment("v1").build())
+                                .build()
+                        )
+                    }
+                }
+            )
+        )
+
+        val request = getRequest("/hello/")
+        val call = client.newCall(request)
+        call.execute()
+
+        val okHttpEvent = SentryOkHttpEventListener.eventMap[call]!!
+        assertEquals(fixture.server.url("/hello/v1").toUrl().toString(), okHttpEvent.callSpan!!.getData("url"))
+    }
 }