Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SentryOkHttpEvent report exceptions only on the call root span #2961

Merged
merged 4 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- Add `CheckInUtils.withCheckIn` which abstracts away some of the manual check-ins complexity ([#2959](https://github.com/getsentry/sentry-java/pull/2959))

### Fixes

- SentryOkHttpEvent report exceptions only on the call root span ([#2961](https://github.com/getsentry/sentry-java/pull/2961))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually HTTP errors we're catching here, how about naming it this way?

Suggested change
- SentryOkHttpEvent report exceptions only on the call root span ([#2961](https://github.com/getsentry/sentry-java/pull/2961))
- Always attach `SentryOkHttpEvent` errors to the call root span ([#2961](https://github.com/getsentry/sentry-java/pull/2961))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's both
any exception caught by the listener is now associated to the call root span only, and the SentryHttpClientException is also associated to the call root span only.
What about "Always attach OkHttp errors and Http Client Errors only to call root span"?


## 6.30.0

### Features
Expand Down
5 changes: 5 additions & 0 deletions sentry-android-okhttp/api/sentry-android-okhttp.api
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,8 @@ public abstract interface class io/sentry/android/okhttp/SentryOkHttpInterceptor
public abstract fun execute (Lio/sentry/ISpan;Lokhttp3/Request;Lokhttp3/Response;)Lio/sentry/ISpan;
}

public final class io/sentry/android/okhttp/SentryOkHttpUtils {
public static final field INSTANCE Lio/sentry/android/okhttp/SentryOkHttpUtils;
public final fun captureClientError (Lio/sentry/IHub;Lokhttp3/Request;Lokhttp3/Response;)V
}

Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,13 @@ private const val ERROR_MESSAGE_KEY = "error_message"
private const val RESPONSE_BODY_TIMEOUT_MILLIS = 500L
internal const val TRACE_ORIGIN = "auto.http.okhttp"

@Suppress("TooManyFunctions")
internal class SentryOkHttpEvent(private val hub: IHub, private val request: Request) {
private val eventSpans: MutableMap<String, ISpan> = ConcurrentHashMap()
private val breadcrumb: Breadcrumb
internal val callRootSpan: ISpan?
private var response: Response? = null
private var clientErrorResponse: Response? = null
private val isReadingResponseBody = AtomicBoolean(false)

init {
Expand Down Expand Up @@ -93,6 +95,10 @@ internal class SentryOkHttpEvent(private val hub: IHub, private val request: Req
}
}

fun setClientErrorResponse(response: Response) {
this.clientErrorResponse = response
}

/** Sets the [errorMessage] if not null. */
fun setError(errorMessage: String?) {
if (errorMessage != null) {
Expand All @@ -119,8 +125,10 @@ internal class SentryOkHttpEvent(private val hub: IHub, private val request: Req
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()
Expand All @@ -134,9 +142,16 @@ internal class SentryOkHttpEvent(private val hub: IHub, private val request: Req
// We forcefully finish all spans, even if they should already have been finished through finishSpan()
eventSpans.values.filter { !it.isFinished }.forEach {
// If a status was set on the span, we use that, otherwise we set its status as error.
it.finish(it.status ?: SpanStatus.INTERNAL_ERROR)
it.status = it.status ?: SpanStatus.INTERNAL_ERROR
moveThrowableToRootSpan(it)
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.
clientErrorResponse?.let {
SentryOkHttpUtils.captureClientError(hub, it.request, it)
}
if (finishDate != null) {
callRootSpan.finish(callRootSpan.status, finishDate)
} else {
Expand All @@ -152,6 +167,15 @@ internal class SentryOkHttpEvent(private val hub: IHub, private val request: Req
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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,15 @@ import io.sentry.HubAdapter
import io.sentry.IHub
import io.sentry.ISpan
import io.sentry.IntegrationName
import io.sentry.SentryEvent
import io.sentry.SentryIntegrationPackageStorage
import io.sentry.SentryOptions.DEFAULT_PROPAGATION_TARGETS
import io.sentry.SpanDataConvention
import io.sentry.SpanStatus
import io.sentry.TypeCheckHint.OKHTTP_REQUEST
import io.sentry.TypeCheckHint.OKHTTP_RESPONSE
import io.sentry.exception.ExceptionMechanismException
import io.sentry.exception.SentryHttpClientException
import io.sentry.protocol.Mechanism
import io.sentry.util.HttpUtils
import io.sentry.util.PropagationTargetsUtils
import io.sentry.util.TracingUtils
import io.sentry.util.UrlUtils
import okhttp3.Headers
import okhttp3.Interceptor
import okhttp3.Request
import okhttp3.Response
Expand Down Expand Up @@ -70,25 +64,26 @@ class SentryOkHttpInterceptor(
val method = request.method

val span: ISpan?
val isFromEventListener: Boolean
val okHttpEvent: SentryOkHttpEvent?

if (SentryOkHttpEventListener.eventMap.containsKey(chain.call())) {
// read the span from the event listener
span = SentryOkHttpEventListener.eventMap[chain.call()]?.callRootSpan
isFromEventListener = true
okHttpEvent = SentryOkHttpEventListener.eventMap[chain.call()]
span = okHttpEvent?.callRootSpan
} else {
// read the span from the bound scope
okHttpEvent = null
span = hub.span?.startChild("http.client", "$method $url")
isFromEventListener = false
}

span?.spanContext?.origin = TRACE_ORIGIN

urlDetails.applyToSpan(span)

val isFromEventListener = okHttpEvent != null
var response: Response? = null

var code: Int? = null

try {
val requestBuilder = request.newBuilder()

Expand All @@ -114,7 +109,17 @@ class SentryOkHttpInterceptor(
// OkHttp errors (4xx, 5xx) don't throw, so it's safe to call within this block.
// breadcrumbs are added on the finally block because we'd like to know if the device
// had an unstable connection or something similar
captureEvent(request, response)
if (shouldCaptureClientError(request, response)) {
// If we capture the client error directly, it could be associated with the
// currently running span by the backend. In case the listener is in use, that is
// an inner span. So, if the listener is in use, we let it capture the client
// error, to shown it in the http root call span in the dashboard.
if (isFromEventListener && okHttpEvent != null) {
okHttpEvent.setClientErrorResponse(response)
} else {
SentryOkHttpUtils.captureClientError(hub, request, response)
}
}

return response
} catch (e: IOException) {
Expand Down Expand Up @@ -180,65 +185,18 @@ class SentryOkHttpInterceptor(
}
}

private fun captureEvent(request: Request, response: Response) {
private fun shouldCaptureClientError(request: Request, response: Response): Boolean {
// return if the feature is disabled or its not within the range
if (!captureFailedRequests || !containsStatusCode(response.code)) {
return
return false
}

// not possible to get a parameterized url, but we remove at least the
// query string and the fragment.
// url example: https://api.github.com/users/getsentry/repos/#fragment?query=query
// url will be: https://api.github.com/users/getsentry/repos/
// ideally we'd like a parameterized url: https://api.github.com/users/{user}/repos/
// but that's not possible
val urlDetails = UrlUtils.parse(request.url.toString())

// return if its not a target match
if (!PropagationTargetsUtils.contain(failedRequestTargets, request.url.toString())) {
return
}

val mechanism = Mechanism().apply {
type = "SentryOkHttpInterceptor"
}
val exception = SentryHttpClientException(
"HTTP Client Error with status code: ${response.code}"
)
val mechanismException = ExceptionMechanismException(mechanism, exception, Thread.currentThread(), true)
val event = SentryEvent(mechanismException)

val hint = Hint()
hint.set(OKHTTP_REQUEST, request)
hint.set(OKHTTP_RESPONSE, response)

val sentryRequest = io.sentry.protocol.Request().apply {
urlDetails.applyToRequest(this)
// Cookie is only sent if isSendDefaultPii is enabled
cookies = if (hub.options.isSendDefaultPii) request.headers["Cookie"] else null
method = request.method
headers = getHeaders(request.headers)

request.body?.contentLength().ifHasValidLength {
bodySize = it
}
return false
}

val sentryResponse = io.sentry.protocol.Response().apply {
// Set-Cookie is only sent if isSendDefaultPii is enabled due to PII
cookies = if (hub.options.isSendDefaultPii) response.headers["Set-Cookie"] else null
headers = getHeaders(response.headers)
statusCode = response.code

response.body?.contentLength().ifHasValidLength {
bodySize = it
}
}

event.request = sentryRequest
event.contexts.setResponse(sentryResponse)

hub.captureEvent(event, hint)
return true
}

private fun containsStatusCode(statusCode: Int): Boolean {
Expand All @@ -250,28 +208,6 @@ class SentryOkHttpInterceptor(
return false
}

private fun getHeaders(requestHeaders: Headers): MutableMap<String, String>? {
// Headers are only sent if isSendDefaultPii is enabled due to PII
if (!hub.options.isSendDefaultPii) {
return null
}

val headers = mutableMapOf<String, String>()

for (i in 0 until requestHeaders.size) {
val name = requestHeaders.name(i)

// header is only sent if isn't sensitive
if (HttpUtils.containsSensitiveHeader(name)) {
continue
}

val value = requestHeaders.value(i)
headers[name] = value
}
return headers
}

/**
* The BeforeSpan callback
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package io.sentry.android.okhttp

import io.sentry.Hint
import io.sentry.IHub
import io.sentry.SentryEvent
import io.sentry.TypeCheckHint
import io.sentry.exception.ExceptionMechanismException
import io.sentry.exception.SentryHttpClientException
import io.sentry.protocol.Mechanism
import io.sentry.util.HttpUtils
import io.sentry.util.UrlUtils
import okhttp3.Headers
import okhttp3.Request
import okhttp3.Response

object SentryOkHttpUtils {

fun captureClientError(hub: IHub, request: Request, response: Response) {
// not possible to get a parameterized url, but we remove at least the
// query string and the fragment.
// url example: https://api.github.com/users/getsentry/repos/#fragment?query=query
// url will be: https://api.github.com/users/getsentry/repos/
// ideally we'd like a parameterized url: https://api.github.com/users/{user}/repos/
// but that's not possible
val urlDetails = UrlUtils.parse(request.url.toString())

val mechanism = Mechanism().apply {
type = "SentryOkHttpInterceptor"
}
val exception = SentryHttpClientException(
"HTTP Client Error with status code: ${response.code}"
)
val mechanismException = ExceptionMechanismException(mechanism, exception, Thread.currentThread(), true)
val event = SentryEvent(mechanismException)

val hint = Hint()
hint.set(TypeCheckHint.OKHTTP_REQUEST, request)
hint.set(TypeCheckHint.OKHTTP_RESPONSE, response)

val sentryRequest = io.sentry.protocol.Request().apply {
urlDetails.applyToRequest(this)
// Cookie is only sent if isSendDefaultPii is enabled
cookies = if (hub.options.isSendDefaultPii) request.headers["Cookie"] else null
method = request.method
headers = getHeaders(hub, request.headers)

request.body?.contentLength().ifHasValidLength {
bodySize = it
}
}

val sentryResponse = io.sentry.protocol.Response().apply {
// Set-Cookie is only sent if isSendDefaultPii is enabled due to PII
cookies = if (hub.options.isSendDefaultPii) response.headers["Set-Cookie"] else null
headers = getHeaders(hub, response.headers)
statusCode = response.code

response.body?.contentLength().ifHasValidLength {
bodySize = it
}
}

event.request = sentryRequest
event.contexts.setResponse(sentryResponse)

hub.captureEvent(event, hint)
}

private fun Long?.ifHasValidLength(fn: (Long) -> Unit) {
if (this != null && this != -1L) {
fn.invoke(this)
}
}

private fun getHeaders(hub: IHub, requestHeaders: Headers): MutableMap<String, String>? {
// Headers are only sent if isSendDefaultPii is enabled due to PII
if (!hub.options.isSendDefaultPii) {
return null
}

val headers = mutableMapOf<String, String>()

for (i in 0 until requestHeaders.size) {
val name = requestHeaders.name(i)

// header is only sent if isn't sensitive
if (HttpUtils.containsSensitiveHeader(name)) {
continue
}

val value = requestHeaders.value(i)
headers[name] = value
}
return headers
}
}
Loading
Loading