Change OkHttp sub-spans to span attributes (#3556)

* removed sub-span creation from SentryOkHttpEvent and replaced them with setData
* removed http timeout handling in SentryOkHttpEventListener
* SentryOkHttpInterceptor now closes the http call
* renamed constants to make them nicer in the UI

Author: stefanosiano

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+### Breaking Changes
+
+- Change OkHttp sub-spans to span attributes ([#3556](https://github.com/getsentry/sentry-java/pull/3556))
+  - This will reduce the number of spans created by the SDK
+
 ### Fixes
 
 - Use OpenTelemetry span name as fallback for transaction name ([#3557](https://github.com/getsentry/sentry-java/pull/3557))

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

@@ -5,38 +5,28 @@ import io.sentry.Hint
 import io.sentry.IScopes
 import io.sentry.ISpan
 import io.sentry.SentryDate
-import io.sentry.SentryLevel
 import io.sentry.SpanDataConvention
 import io.sentry.TypeCheckHint
-import io.sentry.okhttp.SentryOkHttpEventListener.Companion.CONNECTION_EVENT
-import io.sentry.okhttp.SentryOkHttpEventListener.Companion.CONNECT_EVENT
-import io.sentry.okhttp.SentryOkHttpEventListener.Companion.REQUEST_BODY_EVENT
-import io.sentry.okhttp.SentryOkHttpEventListener.Companion.REQUEST_HEADERS_EVENT
-import io.sentry.okhttp.SentryOkHttpEventListener.Companion.RESPONSE_BODY_EVENT
-import io.sentry.okhttp.SentryOkHttpEventListener.Companion.RESPONSE_HEADERS_EVENT
-import io.sentry.okhttp.SentryOkHttpEventListener.Companion.SECURE_CONNECT_EVENT
 import io.sentry.util.Platform
 import io.sentry.util.UrlUtils
 import okhttp3.Request
 import okhttp3.Response
 import java.util.Locale
 import java.util.concurrent.ConcurrentHashMap
-import java.util.concurrent.RejectedExecutionException
+import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicBoolean
 
 private const val PROTOCOL_KEY = "protocol"
 private const val ERROR_MESSAGE_KEY = "error_message"
-private const val RESPONSE_BODY_TIMEOUT_MILLIS = 800L
 internal const val TRACE_ORIGIN = "auto.http.okhttp"
 
 @Suppress("TooManyFunctions")
 internal class SentryOkHttpEvent(private val scopes: IScopes, private val request: Request) {
-    private val eventSpans: MutableMap<String, ISpan> = ConcurrentHashMap()
+    private val eventDates: MutableMap<String, SentryDate> = ConcurrentHashMap()
     private val breadcrumb: Breadcrumb
-    internal val callRootSpan: ISpan?
+    internal val callSpan: ISpan?
     private var response: Response? = null
     private var clientErrorResponse: Response? = null
-    private val isReadingResponseBody = AtomicBoolean(false)
     private val isEventFinished = AtomicBoolean(false)
     private val url: String
     private val method: String
@@ -50,52 +40,52 @@ internal class SentryOkHttpEvent(private val scopes: IScopes, private val reques
 
         // We start the call span that will contain all the others
         val parentSpan = if (Platform.isAndroid()) scopes.transaction else scopes.span
-        callRootSpan = parentSpan?.startChild("http.client", "$method $url")
-        callRootSpan?.spanContext?.origin = TRACE_ORIGIN
-        urlDetails.applyToSpan(callRootSpan)
+        callSpan = parentSpan?.startChild("http.client", "$method $url")
+        callSpan?.spanContext?.origin = TRACE_ORIGIN
+        urlDetails.applyToSpan(callSpan)
 
         // We setup a breadcrumb with all meaningful data
         breadcrumb = Breadcrumb.http(url, method)
         breadcrumb.setData("host", host)
         breadcrumb.setData("path", encodedPath)
 
-        // We add the same data to the root call span
-        callRootSpan?.setData("url", url)
-        callRootSpan?.setData("host", host)
-        callRootSpan?.setData("path", encodedPath)
-        callRootSpan?.setData(SpanDataConvention.HTTP_METHOD_KEY, method.uppercase(Locale.ROOT))
+        // We add the same data to the call span
+        callSpan?.setData("url", url)
+        callSpan?.setData("host", host)
+        callSpan?.setData("path", encodedPath)
+        callSpan?.setData(SpanDataConvention.HTTP_METHOD_KEY, method.uppercase(Locale.ROOT))
     }
 
     /**
      * Sets the [Response] that will be sent in the breadcrumb [Hint].
-     * Also, it sets the protocol and status code in the breadcrumb and the call root span.
+     * Also, it sets the protocol and status code in the breadcrumb and the call span.
      */
     fun setResponse(response: Response) {
         this.response = response
         breadcrumb.setData(PROTOCOL_KEY, response.protocol.name)
         breadcrumb.setData("status_code", response.code)
-        callRootSpan?.setData(PROTOCOL_KEY, response.protocol.name)
-        callRootSpan?.setData(SpanDataConvention.HTTP_STATUS_CODE_KEY, response.code)
+        callSpan?.setData(PROTOCOL_KEY, response.protocol.name)
+        callSpan?.setData(SpanDataConvention.HTTP_STATUS_CODE_KEY, response.code)
     }
 
     fun setProtocol(protocolName: String?) {
         if (protocolName != null) {
             breadcrumb.setData(PROTOCOL_KEY, protocolName)
-            callRootSpan?.setData(PROTOCOL_KEY, protocolName)
+            callSpan?.setData(PROTOCOL_KEY, protocolName)
         }
     }
 
     fun setRequestBodySize(byteCount: Long) {
         if (byteCount > -1) {
             breadcrumb.setData("request_content_length", byteCount)
-            callRootSpan?.setData("http.request_content_length", byteCount)
+            callSpan?.setData("http.request_content_length", byteCount)
         }
     }
 
     fun setResponseBodySize(byteCount: Long) {
         if (byteCount > -1) {
             breadcrumb.setData("response_content_length", byteCount)
-            callRootSpan?.setData(SpanDataConvention.HTTP_RESPONSE_CONTENT_LENGTH_KEY, byteCount)
+            callSpan?.setData(SpanDataConvention.HTTP_RESPONSE_CONTENT_LENGTH_KEY, byteCount)
         }
     }
 
@@ -107,44 +97,33 @@ internal class SentryOkHttpEvent(private val scopes: IScopes, private val reques
     fun setError(errorMessage: String?) {
         if (errorMessage != null) {
             breadcrumb.setData(ERROR_MESSAGE_KEY, errorMessage)
-            callRootSpan?.setData(ERROR_MESSAGE_KEY, errorMessage)
+            callSpan?.setData(ERROR_MESSAGE_KEY, errorMessage)
         }
     }
 
-    /** Starts a span, if the callRootSpan is not null. */
-    fun startSpan(event: String) {
-        // Find the parent of the span being created. E.g. secureConnect is child of connect
-        val parentSpan = findParentSpan(event)
-        val span = parentSpan?.startChild("http.client.$event", "$method $url") ?: return
-        if (event == RESPONSE_BODY_EVENT) {
-            // We save this event is reading the response body, so that it will not be auto-finished
-            isReadingResponseBody.set(true)
-        }
-        span.spanContext.origin = TRACE_ORIGIN
-        eventSpans[event] = span
+    /** Record event start if the callRootSpan is not null. */
+    fun onEventStart(event: String) {
+        callSpan ?: return
+        eventDates[event] = scopes.options.dateProvider.now()
     }
 
-    /** Finishes a previously started span, and runs [beforeFinish] on it, on its parent and on the call root span. */
-    fun finishSpan(event: String, beforeFinish: ((span: ISpan) -> Unit)? = null): ISpan? {
-        val span = eventSpans[event] ?: return null
-        val parentSpan = findParentSpan(event)
-        beforeFinish?.invoke(span)
-        moveThrowableToRootSpan(span)
-        if (parentSpan != null && parentSpan != callRootSpan) {
-            beforeFinish?.invoke(parentSpan)
-            moveThrowableToRootSpan(parentSpan)
-        }
-        callRootSpan?.let { beforeFinish?.invoke(it) }
-        span.finish()
-        return span
+    /** Record event finish and runs [beforeFinish] on the call span. */
+    fun onEventFinish(event: String, beforeFinish: ((span: ISpan) -> Unit)? = null) {
+        val eventDate = eventDates.remove(event) ?: return
+        callSpan ?: return
+        beforeFinish?.invoke(callSpan)
+        val eventDurationNanos = scopes.options.dateProvider.now().diff(eventDate)
+        callSpan.setData(event, TimeUnit.NANOSECONDS.toMillis(eventDurationNanos))
     }
 
-    /** Finishes the call root span, and runs [beforeFinish] on it. Then a breadcrumb is sent. */
-    fun finishEvent(finishDate: SentryDate? = null, beforeFinish: ((span: ISpan) -> Unit)? = null) {
+    /** Finishes the call span, and runs [beforeFinish] on it. Then a breadcrumb is sent. */
+    fun finish(beforeFinish: ((span: ISpan) -> Unit)? = null) {
         // If the event already finished, we don't do anything
         if (isEventFinished.getAndSet(true)) {
             return
         }
+        // We clear any date left, in case an event started, but never finished. Shouldn't happen.
+        eventDates.clear()
         // We put data in the hint and send a breadcrumb
         val hint = Hint()
         hint.set(TypeCheckHint.OKHTTP_REQUEST, request)
@@ -153,75 +132,12 @@ internal class SentryOkHttpEvent(private val scopes: IScopes, private val reques
         // We send the breadcrumb even without spans.
         scopes.addBreadcrumb(breadcrumb, hint)
 
-        // No span is created (e.g. no transaction is running)
-        if (callRootSpan == null) {
-            // We report the client error even without spans.
-            clientErrorResponse?.let {
-                SentryOkHttpUtils.captureClientError(scopes, it.request, it)
-            }
-            return
-        }
-
-        // We forcefully finish all spans, even if they should already have been finished through finishSpan()
-        eventSpans.values.filter { !it.isFinished }.forEach {
-            moveThrowableToRootSpan(it)
-            if (finishDate != null) {
-                it.finish(it.status, finishDate)
-            } else {
-                it.finish()
-            }
-        }
-        beforeFinish?.invoke(callRootSpan)
-        // We report the client error here, after all sub-spans finished, so that it will be bound
-        // to the root call span.
+        callSpan?.let { beforeFinish?.invoke(it) }
+        // We report the client error here so that it will be bound to the call span. We send it even if there is no running span.
         clientErrorResponse?.let {
             SentryOkHttpUtils.captureClientError(scopes, it.request, it)
         }
-        if (finishDate != null) {
-            callRootSpan.finish(callRootSpan.status, finishDate)
-        } else {
-            callRootSpan.finish()
-        }
+        callSpan?.finish()
         return
     }
-
-    /** Move any throwable from an inner span to the call root span. */
-    private fun moveThrowableToRootSpan(span: ISpan) {
-        if (span != callRootSpan && span.throwable != null && span.status != null) {
-            callRootSpan?.throwable = span.throwable
-            callRootSpan?.status = span.status
-            span.throwable = null
-        }
-    }
-
-    private fun findParentSpan(event: String): ISpan? = when (event) {
-        // PROXY_SELECT, DNS, CONNECT and CONNECTION are not children of one another
-        SECURE_CONNECT_EVENT -> eventSpans[CONNECT_EVENT]
-        REQUEST_HEADERS_EVENT -> eventSpans[CONNECTION_EVENT]
-        REQUEST_BODY_EVENT -> eventSpans[CONNECTION_EVENT]
-        RESPONSE_HEADERS_EVENT -> eventSpans[CONNECTION_EVENT]
-        RESPONSE_BODY_EVENT -> eventSpans[CONNECTION_EVENT]
-        else -> callRootSpan
-    } ?: callRootSpan
-
-    fun scheduleFinish(timestamp: SentryDate) {
-        try {
-            scopes.options.executorService.schedule({
-                if (!isReadingResponseBody.get() &&
-                    (eventSpans.values.all { it.isFinished } || callRootSpan?.isFinished != true)
-                ) {
-                    finishEvent(timestamp)
-                }
-            }, RESPONSE_BODY_TIMEOUT_MILLIS)
-        } catch (e: RejectedExecutionException) {
-            scopes.options
-                .logger
-                .log(
-                    SentryLevel.ERROR,
-                    "Failed to call the executor. OkHttp span will not be finished " +
-                        "automatically. Did you call Sentry.close()?",
-                    e
-                )
-        }
-    }
 }