-
-
Notifications
You must be signed in to change notification settings - Fork 458
replay(feature): Adding OkHttp Request/Response bodies for sentry-java #4796
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
base: main
Are you sure you want to change the base?
Conversation
…dcrumbCallback to extract NetworkRequestData from Hint -> NetworkRequestData is landing on DefaultReplayBreadcrumbConverter via Hint.get(replay:networkDetails)
...ry-android-replay/src/main/java/io/sentry/android/replay/DefaultReplayBreadcrumbConverter.kt
Outdated
Show resolved
Hide resolved
| options.setBeforeBreadcrumb( | ||
| replayBreadcrumbConverter | ||
| ); |
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 BeforeBreadcrumbCallback overwrites 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 initializeIntegrationsAndProcessors which is called after the user config. You can access the converter via options.getReplayController().getBreadcrumbConverter later on
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.
nice one 🙈 thanks
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.
fixed
| val networkData = createNetworkRequestData(request, response, requestBodySize, responseBodySize) | ||
| it.set("replay:networkDetails", networkData) | ||
|
|
||
| // it.set(OKHTTP_REQUEST, request) |
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.
we should probably still keep these because some of our customer may rely on them being present in the hint
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.
done
|
|
||
| /** | ||
| * 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
fixed, but need to do some more testing
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you're gonna clean these up, but if you wanna keep some logs please use options.logger as it will no-op in production builds
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.
cleaned up most, left in the rest to help with debugging. will remove / clean-up before landing (added to TODOs)
These get set on the breadcrumb Hint, and then hang around in DefaultReplayBreadcrumbConverter until the replay segment has been sent off See https://github.com/getsentry/sentry-javascript/blob/632f0b953d99050c11b0edafb9f80b5f3ba88045/packages/replay-internal/src/types/performance.ts#L133-L140 https://github.com/getsentry/sentry-javascript/blob/develop/packages/replay-internal/src/types/request.ts#L12
Manually set networkDetailHasUrls to pass this check https://github.com/getsentry/sentry-javascript/blob/d1646c8a281dd8795c5a6a3b8e18f2e7069e7fa9/packages/replay-internal/src/util/handleRecordingEmit.ts#L134 https://github.com/getsentry/sentry/blob/master/static/app/views/replays/detail/network/details/getOutputType.tsx#L33 https://github.com/getsentry/sentry/blob/master/static/app/views/replays/detail/network/details/content.tsx#L55-L56
added some unit tests: ./gradlew :sentry-android-replay:testDebugUnitTest --tests="*DefaultReplayBreadcrumbConverterTest*"
Breadcrumb.java has several timestamp fields: `timestamp: Date`, `timestampMs: Long`, `nanos: Long` `hashcode` was relying solely on `timestamp`, which can be null depending on which constructor was used. => Change to use getTimestamp as 1. this is what equals does (consistency) 2. getTimestamp initialises timestamp if null.
| private val httpNetworkDetails = | ||
| Collections.synchronizedMap( | ||
| object : LinkedHashMap<Breadcrumb, NetworkRequestData>() { | ||
| override fun removeEldestEntry( | ||
| eldest: MutableMap.MutableEntry<Breadcrumb, NetworkRequestData>? | ||
| ): Boolean { | ||
| return size > MAX_HTTP_NETWORK_DETAILS | ||
| } | ||
| } | ||
| ) |
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.
Bug: Breadcrumb.equals() omits data map, causing HashMap key collisions for concurrent requests, leading to incorrect network data attribution.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The Breadcrumb.equals() method omits the data map from its comparison. This allows distinct Breadcrumb objects, particularly those generated by HTTP requests occurring within the same millisecond but having different URLs or other data map contents, to be considered equal. When these Breadcrumb objects are used as keys in the httpNetworkDetails LinkedHashMap, one entry can unintentionally overwrite another, causing network details from one request to be incorrectly associated with a different request during session replay processing.
💡 Suggested Fix
Modify Breadcrumb.equals() to include the data map in its comparison, or use a different key for httpNetworkDetails that uniquely identifies each network request, such as a UUID or a composite key including relevant data fields.
🤖 Prompt for AI Agent
Fix this bug. In
sentry-android-replay/src/main/java/io/sentry/android/replay/DefaultReplayBreadcrumbConverter.kt
at lines 33-42: The `Breadcrumb.equals()` method omits the `data` map from its
comparison. This allows distinct `Breadcrumb` objects, particularly those generated by
HTTP requests occurring within the same millisecond but having different URLs or other
`data` map contents, to be considered equal. When these `Breadcrumb` objects are used as
keys in the `httpNetworkDetails` `LinkedHashMap`, one entry can unintentionally
overwrite another, causing network details from one request to be incorrectly associated
with a different request during session replay processing.
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.
@romtsn is there any issue with updating Breadcrumb#equals and Breadcrumb#hashmap to include data in the comparison?
I left it out in my commit cuz assumed we want Breadcrumbs to be equal if they have different data (e.g. it's the same breadcrumb if some more data gets added by a different caller at some point later)
| options.setCompositePerformanceCollector(new DefaultCompositePerformanceCollector(options)); | ||
| } | ||
|
|
||
| if (options.getReplayController() != null) { // TODO: Triple-check this should be a null check |
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.
@roman can you be my triple-check? the code above uses instanceof , but afaict the NoOpReplayController always gets overridden so i put a null check here
...ry-android-replay/src/main/java/io/sentry/android/replay/DefaultReplayBreadcrumbConverter.kt
Outdated
Show resolved
Hide resolved
Entrypoint is NetworkDetailCaptureUtils (initializeForUrl) called from SentryOkHttpInterceptor - common logic to handle checking sdk options. - Accept data from http client via NetworkBodyExtractor, NetworkHeaderExtractor interfaces that can be reused in future (if needed) Placeholder impl for req/resp bodies. From https://docs.sentry.io/platforms/javascript/session-replay/configuration/ - networkDetailAllowUrls, networkDetailDenyUrls, - networkCaptureBodies - networkRequestHeaders, networkResponseHeaders These SDKOptions don't exist yet => impl acts as if they do, but have not been enabled.
9a4852b to
82404ff
Compare
to revert
…entries Removes entry when creating RRWebSpanEvent Uses syncrhonized LinkedHashMap with impl to cap size of map (avoid memory bloat)
Replaces previous placeholder logic. Now NetworkBodyParser uses io.sentry.JsonObjectReader to extract body into JSONObject, JSONArray, with fallback to plain-text String (or nothing)
82404ff to
d9f8254
Compare
|
seeing something i want to test more (safely copying out the request body for one-shot okhttp request bodies), moved back to draft for now |
| options.setBeforeBreadcrumb( | ||
| replayBreadcrumbConverter | ||
| ); | ||
| } |
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.
📜 Description
Introduce new utility/data classes (
io.sentry.util.network pkg), following format in the javascript SDK.Initalise
DefaultReplayBreadcrumbConverteras the SDK-wideBeforeBreadcrumbCallback. It is responsible for delegating to a user-providedBeforeBreadcrumb.In
BeforeBreadcrumb,NetworkDetailCaptureUtilsextracts "Network Details" data objects (NetworkRequestData,ReplayNetworkRequestOrResponse) and inserts back into the breadcrumb Hint (only sentry-okhttp for now).When converting Breadcrumb to
RRWebSpanEvent, extractNetworkRequestDatadata from Hint (name="replay:networkDetails") and insert into the replayperformanceSpanwhen creating the replay segment.TODOs
Feedback:
acceptable acceptance criteria for landing? Given so many diff types of request bodies ->
Current impl handles JSON, UrlFormEncoded, skips these binary content types, and falls back to raw String as possible
Decide on keeping changes to sentry-samples-android
I am planning to add more http request body "types" to the sample app (e.g. xml bodies,... see below). Think this could be useful / reusable. @romtsn wdyt?
Implementation:
networkDetail*SDK Options flagsx-www-form-urlencodedrequest bodiesxmlrequest bodiesPre-Land:
💡 Motivation and Context
Part of [Mobile Replay] Capture Network request/response bodies
Initially, we were trying to keep SDK changes simple and re-use the existing OKHTTP_REQUEST|RESPONSE hint data.
However, the
:sentry-android-replaygradle module doesn't compile against any of the http libs (makes sense).Because these
okhttp3.Request, etc types don't exist in:sentry-android-replay, replay accesses the NetworkDetails data via Hint data ("replay:networkDetails") on the Breadcrumb, when the SDKOptions constraints have been met.💚 How did you test it?
See recording segment data of entire replay payload
session replay captured from sentry-samples test app
https://sentry-sdks.sentry.io/explore/replays/f89535ea0e79499ca8d75e34e6270925
1. Request / Response headers respect networkDetail[Request|Response]Headers
ref
2. Request body formatted as JSON key/values

ref
3. Response body formatted as JSON key/values

ref