RFF(replay): Adding OkHttp Request/Response bodies for sentry-java #4796
+774
−14
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,22 @@ | ||
| package io.sentry.android.replay | ||
|
|
||
| import android.util.Log | ||
| import io.sentry.Breadcrumb | ||
| import io.sentry.Hint | ||
| import io.sentry.ReplayBreadcrumbConverter | ||
| import io.sentry.SentryLevel | ||
| import io.sentry.SentryOptions | ||
| import io.sentry.SentryOptions.BeforeBreadcrumbCallback | ||
| import io.sentry.SpanDataConvention | ||
| import io.sentry.rrweb.RRWebBreadcrumbEvent | ||
| import io.sentry.rrweb.RRWebEvent | ||
| import io.sentry.rrweb.RRWebSpanEvent | ||
| import io.sentry.util.network.NetworkRequestData | ||
| import kotlin.LazyThreadSafetyMode.NONE | ||
|
|
||
| public open class DefaultReplayBreadcrumbConverter( | ||
| private val userBeforeBreadcrumbCallback: BeforeBreadcrumbCallback? = null | ||
| ) : ReplayBreadcrumbConverter, SentryOptions.BeforeBreadcrumbCallback { | ||
| internal companion object { | ||
| private val snakecasePattern by lazy(NONE) { "_[a-z]".toRegex() } | ||
| private val supportedNetworkData = | ||
|
|
@@ -24,10 +31,11 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter { | |
| } | ||
|
|
||
| private var lastConnectivityState: String? = null | ||
| private val httpBreadcrumbData = mutableMapOf<Breadcrumb, NetworkRequestData>() | ||
|
There was a problem hiding this comment. Bug: Memory Leak in Breadcrumb Data StorageThe There was a problem hiding this comment. not sure if you want to use a ring buffer here, but if you do we already have an implementation in the SDK available: https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/CircularFifoQueue.java There was a problem hiding this comment. Potential bug: The singleton's
|
||
|
|
||
| override fun convert(breadcrumb: Breadcrumb): RRWebEvent? { | ||
| var breadcrumbMessage: String? = null | ||
| var breadcrumbCategory: String? | ||
| var breadcrumbLevel: SentryLevel? = null | ||
| val breadcrumbData = mutableMapOf<String, Any?>() | ||
| when { | ||
|
|
@@ -120,10 +128,62 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * By default, ReplayIntegration provides its own BeforeBreadcrumbCallback, | ||
| * delegating to user-provided callback (if exists). | ||
| */ | ||
| override fun execute(breadcrumb: Breadcrumb, hint: Hint): Breadcrumb? { | ||
| Log.d("SentryNetwork", "SentryNetwork: BeforeBreadcrumbCallback - Hint: $hint, Breadcrumb: $breadcrumb") | ||
| return userBeforeBreadcrumbCallback?.let { | ||
| it.execute(breadcrumb, hint)?.also { processedBreadcrumb -> | ||
| extractNetworkRequestDataFromHint(processedBreadcrumb, hint)?.let { networkData -> | ||
| httpBreadcrumbData[processedBreadcrumb] = networkData | ||
| } | ||
| } | ||
| } ?: run { | ||
| // No user callback - store hint and return original breadcrumb | ||
| extractNetworkRequestDataFromHint(breadcrumb, hint)?.let { networkData -> | ||
| httpBreadcrumbData[breadcrumb] = networkData | ||
| } | ||
| breadcrumb | ||
| } | ||
| } | ||
|
|
||
| private fun extractNetworkRequestDataFromHint(breadcrumb: Breadcrumb, breadcrumbHint: Hint): NetworkRequestData? { | ||
| if (breadcrumb.type != "http" && breadcrumb.category != "http") { | ||
| return null | ||
| } | ||
|
|
||
| // First try to get the structured network data from the hint | ||
| val networkDetails = breadcrumbHint.get("replay:networkDetails") as? NetworkRequestData | ||
| if (networkDetails != null) { | ||
| Log.d("SentryNetwork", "SentryNetwork: Found structured NetworkRequestData in hint: $networkDetails") | ||
|
There was a problem hiding this comment. I guess you're gonna clean these up, but if you wanna keep some logs please use |
||
| return networkDetails | ||
| } | ||
|
|
||
| Log.d("SentryNetwork", "SentryNetwork: No structured NetworkRequestData found on hint") | ||
| return null | ||
| } | ||
|
|
||
| private fun Breadcrumb.isValidForRRWebSpan(): Boolean { | ||
| val url = data["url"] as? String | ||
| val hasStartTimestamp = SpanDataConvention.HTTP_START_TIMESTAMP in data | ||
| val hasEndTimestamp = SpanDataConvention.HTTP_END_TIMESTAMP in data | ||
|
|
||
| val urlValid = !url.isNullOrEmpty() | ||
| val isValid = urlValid && hasStartTimestamp && hasEndTimestamp | ||
|
|
||
| val reasons = mutableListOf<String>() | ||
| if (!urlValid) reasons.add("missing or empty URL") | ||
| if (!hasStartTimestamp) reasons.add("missing start timestamp") | ||
| if (!hasEndTimestamp) reasons.add("missing end timestamp") | ||
|
|
||
| Log.d("SentryReplay", "Breadcrumb RRWeb span validation: ${if (isValid) "VALID" else "INVALID"}" + | ||
| if (!isValid) " (${reasons.joinToString(", ")})" else "" + | ||
| " - URL: ${url ?: "null"}, Category: ${category}") | ||
|
|
||
| return isValid | ||
| } | ||
|
|
||
| private fun String.snakeToCamelCase(): String = | ||
| replace(snakecasePattern) { it.value.last().toString().uppercase() } | ||
|
|
@@ -132,6 +192,14 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter { | |
| val breadcrumb = this | ||
| val httpStartTimestamp = breadcrumb.data[SpanDataConvention.HTTP_START_TIMESTAMP] | ||
| val httpEndTimestamp = breadcrumb.data[SpanDataConvention.HTTP_END_TIMESTAMP] | ||
|
|
||
| // Get the NetworkRequestData if available | ||
| val networkRequestData = httpBreadcrumbData[breadcrumb] | ||
|
|
||
| Log.d("SentryNetwork", "SentryNetwork: convert(breadcrumb=${breadcrumb.type}) httpBreadcrumbData map size: ${httpBreadcrumbData.size}, " + | ||
| "contains current breadcrumb: ${httpBreadcrumbData.containsKey(breadcrumb)}, " + | ||
| "network data for current: ${httpBreadcrumbData[breadcrumb]}") | ||
|
|
||
| return RRWebSpanEvent().apply { | ||
| timestamp = breadcrumb.timestamp.time | ||
| op = "resource.http" | ||
|
|
@@ -151,13 +219,50 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter { | |
| } | ||
|
|
||
| val breadcrumbData = mutableMapOf<String, Any?>() | ||
|
|
||
| // Add data from NetworkRequestData if available | ||
| if (networkRequestData != null) { | ||
| networkRequestData.method?.let { breadcrumbData["method"] = it } | ||
| networkRequestData.statusCode?.let { breadcrumbData["statusCode"] = it } | ||
| networkRequestData.requestBodySize?.let { breadcrumbData["requestBodySize"] = it } | ||
| networkRequestData.responseBodySize?.let { breadcrumbData["responseBodySize"] = it } | ||
|
|
||
| // Add request and response data if available | ||
| networkRequestData.request?.let { request -> | ||
| val requestData = mutableMapOf<String, Any?>() | ||
| request.size?.let { requestData["size"] = it } | ||
| request.body?.let { requestData["body"] = it } | ||
| if (request.headers.isNotEmpty()) { | ||
| requestData["headers"] = request.headers | ||
| } | ||
| if (requestData.isNotEmpty()) { | ||
| breadcrumbData["request"] = requestData | ||
| } | ||
| } | ||
|
|
||
| networkRequestData.response?.let { response -> | ||
| val responseData = mutableMapOf<String, Any?>() | ||
| response.size?.let { responseData["size"] = it } | ||
| response.body?.let { responseData["body"] = it } | ||
| if (response.headers.isNotEmpty()) { | ||
| responseData["headers"] = response.headers | ||
| } | ||
| if (responseData.isNotEmpty()) { | ||
| breadcrumbData["response"] = responseData | ||
| } | ||
| } | ||
| } | ||
| // Original breadcrumb data processing | ||
| // TODO: Remove if superceded by more detailed data (above). | ||
| for ((key, value) in breadcrumb.data) { | ||
| if (key in supportedNetworkData) { | ||
| breadcrumbData[ | ||
| key.replace("content_length", "body_size").substringAfter(".").snakeToCamelCase(), | ||
| ] = value | ||
| } | ||
| } | ||
|
|
||
|
|
||
| data = breadcrumbData | ||
| } | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,8 @@ import io.sentry.util.PropagationTargetsUtils | |
| import io.sentry.util.SpanUtils | ||
| import io.sentry.util.TracingUtils | ||
| import io.sentry.util.UrlUtils | ||
| import io.sentry.util.network.NetworkRequestData | ||
|
There was a problem hiding this comment. these are missing from the PR, but I assume they are just data holders so nothing fancy there :) There was a problem hiding this comment. yikes. adding now - and yes i just copied the naming + types from JS |
||
| import io.sentry.util.network.ReplayNetworkRequestOrResponse | ||
| import java.io.IOException | ||
| import okhttp3.Interceptor | ||
| import okhttp3.Request | ||
|
|
@@ -172,18 +174,30 @@ public open class SentryOkHttpInterceptor( | |
| startTimestamp: Long, | ||
| ) { | ||
| val breadcrumb = Breadcrumb.http(request.url.toString(), request.method, code) | ||
|
|
||
| // Track request and response body sizes for the breadcrumb | ||
| var requestBodySize: Long? = null | ||
| var responseBodySize: Long? = null | ||
|
|
||
| request.body?.contentLength().ifHasValidLength { | ||
| breadcrumb.setData("http.request_content_length", it) | ||
| requestBodySize = it | ||
| } | ||
|
|
||
| response?.body?.contentLength().ifHasValidLength { | ||
| breadcrumb.setData(SpanDataConvention.HTTP_RESPONSE_CONTENT_LENGTH_KEY, it) | ||
| responseBodySize = it | ||
| } | ||
|
|
||
| val hint = Hint().also { | ||
| // Set the structured network data for replay | ||
| val networkData = createNetworkRequestData(request, response, requestBodySize, responseBodySize) | ||
| it.set("replay:networkDetails", networkData) | ||
|
|
||
| // it.set(OKHTTP_REQUEST, request) | ||
|
There was a problem hiding this comment. we should probably still keep these because some of our customer may rely on them being present in the hint |
||
| // response?.let { resp -> it[OKHTTP_RESPONSE] = resp } | ||
| } | ||
|
|
||
| // needs this as unix timestamp for rrweb | ||
| breadcrumb.setData(SpanDataConvention.HTTP_START_TIMESTAMP, startTimestamp) | ||
| breadcrumb.setData( | ||
|
|
@@ -194,6 +208,116 @@ public open class SentryOkHttpInterceptor( | |
| scopes.addBreadcrumb(breadcrumb, hint) | ||
| } | ||
|
|
||
| /** | ||
| * Extracts headers from OkHttp Headers object into a map | ||
| */ | ||
| private fun okhttp3.Headers.toMap(): Map<String, String> { | ||
| val headers = mutableMapOf<String, String>() | ||
| for (name in names()) { | ||
| headers[name] = get(name) ?: "" | ||
| } | ||
| return headers | ||
| } | ||
|
|
||
| /** | ||
| * Extracts body metadata from OkHttp RequestBody or ResponseBody | ||
| * Note: We don't consume the actual body stream to avoid interfering with the request/response | ||
|
There was a problem hiding this comment. hm, is it not possible to consume/copy it? I think would be great if we could do that, given that the JS sdk does that too. Otherwise, it's probably not very helpful to just have the headers and some metadata |
||
| */ | ||
| private fun extractBodyMetadata( | ||
| contentLength: Long?, | ||
| contentType: okhttp3.MediaType? | ||
| ): Pair<Long?, Any?> { | ||
| val bodySize = contentLength?.takeIf { it >= 0 } | ||
| val bodyInfo = if (contentLength != null && contentLength != 0L) { | ||
| mapOf( | ||
| "contentType" to contentType?.toString(), | ||
| "hasBody" to true | ||
| ) | ||
| } else null | ||
|
|
||
| return bodySize to bodyInfo | ||
| } | ||
|
|
||
| /** | ||
| * Creates a NetworkRequestData object from the request and response | ||
| */ | ||
| private fun createNetworkRequestData( | ||
| request: Request, | ||
| response: Response?, | ||
| requestBodySize: Long?, | ||
| responseBodySize: Long? | ||
| ): NetworkRequestData { | ||
| // Log the incoming request details | ||
| println("SentryNetwork: Creating NetworkRequestData for: ${request.method} ${request.url}") | ||
| scopes.options.logger.log( | ||
| io.sentry.SentryLevel.INFO, | ||
| "SentryNetwork: Creating NetworkRequestData for: ${request.method} ${request.url}" | ||
| ) | ||
|
|
||
| // Extract request data | ||
| val requestHeaders = request.headers.toMap() | ||
| val (reqBodySize, reqBodyInfo) = extractBodyMetadata( | ||
| request.body?.contentLength(), | ||
| request.body?.contentType() | ||
| ) | ||
|
|
||
| scopes.options.logger.log( | ||
| io.sentry.SentryLevel.INFO, | ||
| "SentryNetwork: Request - Headers count: ${requestHeaders.size}, Body size: $reqBodySize, Body info: $reqBodyInfo" | ||
| ) | ||
|
There was a problem hiding this comment. Bug: Logging Overload and Network Data LossDebug logging statements (Android Additional Locations (5)
|
||
|
|
||
| val requestData = ReplayNetworkRequestOrResponse( | ||
| reqBodySize, | ||
| reqBodyInfo, | ||
| requestHeaders | ||
| ) | ||
|
|
||
| // Extract response data if available | ||
| val responseData = response?.let { | ||
| val responseHeaders = it.headers.toMap() | ||
| val (respBodySize, respBodyInfo) = extractBodyMetadata( | ||
| it.body?.contentLength(), | ||
| it.body?.contentType() | ||
| ) | ||
|
|
||
| scopes.options.logger.log( | ||
| io.sentry.SentryLevel.INFO, | ||
| "SentryNetwork: Response - Status: ${it.code}, Headers count: ${responseHeaders.size}, Body size: $respBodySize, Body info: $respBodyInfo" | ||
| ) | ||
|
|
||
| ReplayNetworkRequestOrResponse( | ||
| respBodySize, | ||
| respBodyInfo, | ||
| responseHeaders | ||
| ) | ||
| } | ||
|
|
||
| // Determine final body sizes (prefer the explicit sizes passed in) | ||
| val finalResponseBodySize = response?.let { | ||
| val (respBodySize, _) = extractBodyMetadata( | ||
| it.body?.contentLength(), | ||
| it.body?.contentType() | ||
| ) | ||
| responseBodySize ?: respBodySize | ||
| } | ||
|
|
||
| val networkData = NetworkRequestData( | ||
| request.method, | ||
| response?.code, | ||
| requestBodySize ?: reqBodySize, | ||
| finalResponseBodySize, | ||
| requestData, | ||
| responseData | ||
| ) | ||
|
|
||
| scopes.options.logger.log( | ||
| io.sentry.SentryLevel.INFO, | ||
| "SentryNetwork: Created NetworkRequestData: $networkData" | ||
| ) | ||
|
|
||
| return networkData | ||
| } | ||
|
|
||
| private fun finishSpan( | ||
| span: ISpan?, | ||
| request: Request, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,14 +75,17 @@ | |
| <activity android:name=".FrameDataForSpansActivity" | ||
| android:exported="false"/> | ||
|
|
||
| <activity android:name=".TriggerHttpRequestActivity" | ||
| android:exported="false"/> | ||
|
|
||
| <!-- NOTE: Replace the test DSN below with YOUR OWN DSN to see the events from this app in your Sentry project/dashboard--> | ||
| <meta-data android:name="io.sentry.dsn" android:value="https://[email protected]/5428559" /> | ||
|
|
||
| <!-- how to enable Sentry's debug mode--> | ||
| <meta-data android:name="io.sentry.debug" android:value="${sentryDebug}" /> | ||
|
|
||
| <!-- how to disable verbose logging of the session replay feature--> | ||
| <meta-data android:name="io.sentry.session-replay.debug" android:value="true" /> | ||
|
|
||
| <!-- how to set a custom debug level--> | ||
| <!-- <meta-data android:name="io.sentry.debug.level" android:value="info" />--> | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: A user-configured
BeforeBreadcrumbCallbackoverwrites the SDK's replay converter during initialization, silently disabling network capture for replays.Description: During
SentryAndroid.init(), theDefaultReplayBreadcrumbConverteris set as theBeforeBreadcrumbCallbackbefore the user's configuration callback is executed. If a user provides their ownBeforeBreadcrumbCallbackin the configuration, it replaces the SDK's converter. This breaks the replay feature's ability to capture network request data, as the logic inDefaultReplayBreadcrumbConverteris never called. The feature fails silently for any user following the common practice of setting a breadcrumb callback.Suggested fix: Modify the initialization logic to chain the callbacks instead of overwriting. The
DefaultReplayBreadcrumbConvertershould be initialized with the user's callback, which is retrieved after the user's configuration has run. The converter would then execute the user's callback before its own logic.severity: 0.7, confidence: 0.99
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is legit, so we probably have to move setting the beforeBreadcrumb callback over to
initializeIntegrationsAndProcessorswhich is called after the user config. You can access the converter viaoptions.getReplayController().getBreadcrumbConverterlater onThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice one 🙈 thanks