-
-
Notifications
You must be signed in to change notification settings - Fork 463
feat(replay): Capture network request/response details when using SentryOkHttpListener #4919
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
Conversation
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
| a416a65 | 295.53 ms | 373.74 ms | 78.21 ms |
| 889ecea | 367.58 ms | 437.52 ms | 69.94 ms |
| a5ab36f | 320.47 ms | 389.77 ms | 69.30 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
| d217708 | 375.27 ms | 415.68 ms | 40.41 ms |
| fcec2f2 | 357.47 ms | 447.32 ms | 89.85 ms |
| 23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
| d5a29b6 | 298.62 ms | 391.78 ms | 93.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 889ecea | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| a5ab36f | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| 23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
| d5a29b6 | 1.58 MiB | 2.12 MiB | 549.37 KiB |
Previous results on branch: 43jay/MOBILE-935
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 154fb27 | 336.98 ms | 387.10 ms | 50.12 ms |
| 1a07f53 | 341.15 ms | 378.86 ms | 37.71 ms |
| 1c4ba0c | 302.22 ms | 367.21 ms | 64.98 ms |
| 7b2c5a6 | 341.45 ms | 406.52 ms | 65.07 ms |
| fedc731 | 325.81 ms | 383.02 ms | 57.21 ms |
| 6687052 | 321.98 ms | 383.35 ms | 61.38 ms |
| 3162f1d | 308.61 ms | 367.50 ms | 58.89 ms |
| 55850ea | 370.79 ms | 431.33 ms | 60.55 ms |
| 9e1feff | 261.67 ms | 369.02 ms | 107.35 ms |
| da92356 | 319.12 ms | 388.73 ms | 69.61 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 154fb27 | 1.58 MiB | 2.13 MiB | 557.32 KiB |
| 1a07f53 | 1.58 MiB | 2.12 MiB | 553.02 KiB |
| 1c4ba0c | 1.58 MiB | 2.13 MiB | 556.34 KiB |
| 7b2c5a6 | 1.58 MiB | 2.13 MiB | 557.32 KiB |
| fedc731 | 1.58 MiB | 2.13 MiB | 557.33 KiB |
| 6687052 | 1.58 MiB | 2.13 MiB | 556.25 KiB |
| 3162f1d | 1.58 MiB | 2.13 MiB | 557.32 KiB |
| 55850ea | 1.58 MiB | 2.13 MiB | 557.31 KiB |
| 9e1feff | 1.58 MiB | 2.13 MiB | 557.34 KiB |
| da92356 | 1.58 MiB | 2.13 MiB | 556.24 KiB |
cf95f21 to
2e9b796
Compare
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: Duplicate HTTP headers lost in conversion
The toMap() function loses duplicate HTTP headers with the same name. When iterating through OkHttp headers and inserting into a Map<String, String>, duplicate header names get overwritten, keeping only the last value. This causes data loss for headers like Set-Cookie, Accept, or Vary that commonly appear multiple times in HTTP messages. The captured network details will be incomplete when responses or requests contain duplicate headers.
sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt#L265-L272
sentry-java/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt
Lines 265 to 272 in 22bb832
| /** Extracts headers from OkHttp Headers object into a map */ | |
| private fun okhttp3.Headers.toMap(): Map<String, String> { | |
| val headers = linkedMapOf<String, String>() | |
| for (i in 0 until size) { | |
| headers[name(i)] = value(i) | |
| } | |
| return headers | |
| } |
sentry/src/main/java/io/sentry/util/network/NetworkDetailCaptureUtils.java
Show resolved
Hide resolved
e9a9f5f to
e6d7289
Compare
review comment - #4919 (review) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
|
…ator Issue is that commas are valid separators in certain headers (Cookie, Set-Cookie,...). Switch to semi-colon separated instead -> this only governs the formatted list that appears in the sentry dashboard so is relatively minor. review comment - #4919 (comment) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
markushi
left a comment
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.
Looking good, I've left a few comments
sentry/src/main/java/io/sentry/util/network/NetworkDetailCaptureUtils.java
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/util/network/NetworkDetailCaptureUtils.java
Outdated
Show resolved
Hide resolved
review comment - #4919 (review) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
…ator Issue is that commas are valid separators in certain headers (Cookie, Set-Cookie,...). Switch to semi-colon separated instead -> this only governs the formatted list that appears in the sentry dashboard so is relatively minor. review comment - #4919 (comment) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
d075ef2 to
6d104b8
Compare
56f8ea0 to
2d60638
Compare
|
lots of force pushes to get the markdown formatted correctly 🫠 for whatever reason android studio markdown viewer was playing up... |
romtsn
left a comment
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.
LGTM, as discussed internally let's change the options to accept lists instead of arrays
2d60638 to
174245e
Compare
Run tests ./gradlew :sentry:test --tests="*NetworkDetailCaptureUtilsTest*"
Reuse existing logic that retrieves optional SentryOkHttpEvent for the okhttp3.Call, and optionally provide NetworkRequestData for adding to Breadcrumb Hint in SentryOkHttpEvent#finish
./gradlew :sentry-okhttp:test --tests="*SentryOkHttpEventTest*setNetworkDetails*"
review comment - https://github.com/getsentry/sentry-java/pull/4919/files#r2550496731 seems possible, e.g. if a client passes null in the array to SentryReplayOptions#set[Request|Response]Headers
review comment - #4919 (review) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
…ator Issue is that commas are valid separators in certain headers (Cookie, Set-Cookie,...). Switch to semi-colon separated instead -> this only governs the formatted list that appears in the sentry dashboard so is relatively minor. review comment - #4919 (comment) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
Previously some api was expecting String[], others were using List<String>. Change everything to List<String> for consistency
8878e63 to
a151c5b
Compare
📜 Description
Make network details capture (http request/response bodies and headers - docs) available to clients using the sentry gradle plugin by adding support to
SentryOkHttpEventListener.Updates CHANGELOG.
💡 Motivation and Context
Part of [Mobile Replay] Capture Network request/response bodies.
Network Details extraction depends on okhttp3.Interceptor (
SentryOkHttpInterceptor) to intercept okhttp requests and extract the relevant data to the Breadcrumb Hint (see #4796).However, the Sentry Interceptor is only responsible for sending Breadcrumb data if for some reason the
SentryOkHttpEventListeneris not used (code ref).Given registering
SentryOkHttpEventListeneris the default gradle plugin behavior, typical SDK configuration will see Breadcrumb data sent viaSentryOkHttpEventListener. So this PR modifiesSentryOkHttpEventto receive network details data from the interceptor, to be included when sending the Breadcrumb from that path.💚 How did you test it?
Unit
✅
./gradlew :sentry-okhttp:test --tests="*SentryOkHttpEventTest*network*details*"✅
./gradlew :sentry:test --tests="*SentryReplayOptionsTest*Network*" --tests="*RRWebOptionsEventSerializationTest*network*" --tests="*NetworkDetailCaptureUtilsTest*"✅
./gradlew :sentry-android-core:testDebugUnitTest --tests="*ManifestMetadataReaderTest*network*" --rerun-tasksManual
Build sentry-samples:
% ./gradlew :sentry-okhttp:clean :sentry-android-replay:clean :sentry-samples:sentry-samples-android:clean :sentry-samples:sentry-samples-android:installDebug --no-build-cache --rerun-tasks1. OkHttpClient w/ Sentry Interceptor+EventListener will upload network details
Modify sentry-samples to create http requests with event listener & interceptor:

Verify session replay created as expected
https://sentry-sdks.sentry.io/explore/replays/09681100b32640b99f53fc68a016f00d/...
2. OKHttpClient w/ only Sentry Interceptor will upload network details
Comment out addEventListener line and rebuild/install:

Verify session replay created as expected
https://sentry-sdks.sentry.io/explore/replays/29ce82bbd31c482e9fb0a60122412d3d/...
3. OKHttpClient w/ NO Sentry Interceptor will NOT upload network details
Comment out

addInterceptorline and rebuild/install:Verify session replay created with no data as expected
https://sentry-sdks.sentry.io/explore/replays/1232a6f76a6e428281779e8021943429/...
📝 Checklist
sendDefaultPIIis enabled. N/A🔮 Next steps