-
-
Notifications
You must be signed in to change notification settings - Fork 465
fix: improve network body parsing and truncation handling #4958
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
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/util/network/NetworkBodyParser.java
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/util/network/NetworkBodyParser.java
Outdated
Show resolved
Hide resolved
sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fc5ccaf | 276.52 ms | 370.46 ms | 93.93 ms |
| 91bb874 | 311.00 ms | 363.47 ms | 52.47 ms |
| fc5ccaf | 279.11 ms | 353.34 ms | 74.23 ms |
| d364ace | 384.53 ms | 453.51 ms | 68.98 ms |
| 3d205d0 | 352.15 ms | 432.53 ms | 80.38 ms |
| dba088c | 320.59 ms | 361.29 ms | 40.70 ms |
| fc5ccaf | 256.80 ms | 322.36 ms | 65.56 ms |
| ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
| fcec2f2 | 314.96 ms | 373.66 ms | 58.70 ms |
| ae7fed0 | 293.84 ms | 380.22 ms | 86.38 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fc5ccaf | 1.58 MiB | 2.13 MiB | 557.54 KiB |
| 91bb874 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
| fc5ccaf | 1.58 MiB | 2.13 MiB | 557.54 KiB |
| d364ace | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| 3d205d0 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| dba088c | 1.58 MiB | 2.13 MiB | 558.99 KiB |
| fc5ccaf | 1.58 MiB | 2.13 MiB | 557.54 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| ae7fed0 | 1.58 MiB | 2.12 MiB | 551.77 KiB |
Previous results on branch: fix/network-request-response-body-parsing
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 99d7a28 | 292.30 ms | 347.96 ms | 55.66 ms |
| 8287b45 | 308.53 ms | 359.32 ms | 50.79 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 99d7a28 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
| 8287b45 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
...ry-android-replay/src/main/java/io/sentry/android/replay/DefaultReplayBreadcrumbConverter.kt
Show resolved
Hide resolved
| private final @NotNull String value; | ||
| // Based on | ||
| // https://github.com/getsentry/sentry/blob/ccb61aa9b0f33e1333830093a5ce3bd5db88ef33/static/app/utils/replays/replay.tsx#L5-L12 | ||
| public enum NetworkBodyWarning { |
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.
so we can't exactly match all of the reasons on Android?
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.
Some of them simply don't make sense on mobile, like BODY_PARSE_TIMEOUT. So I only added those which we actually use
| } | ||
| return new NetworkBody(data, warnings); | ||
| } | ||
| } catch (Exception e) { |
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.
so we still wanna catch Exception and not Throwable?
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've read up on this and it seems like it's not safe it simply catch a StackOverflow as the JVM could be in an inconsistent state. I've added a max_depth const for parsing instead and combined with the truncation I think we have enough safe guards in place now.
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.
wondering if we this is already a problem elsewhere where we catch Throwables 😅
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!
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
| reader.endObject(); | ||
| } catch (Exception e) { | ||
| result.errored = true; | ||
| return map; |
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.
nit: unnecessary return
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.
| } catch (UnsupportedEncodingException e) { | ||
| return NetworkBody.fromString( | ||
| "[Failed to decode truncated bytes, " + bytes.length + " bytes]"); | ||
| } |
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: Form URL parsing doesn't catch URLDecoder IllegalArgumentException
The parseFormUrlEncoded method catches only UnsupportedEncodingException, but URLDecoder.decode() can also throw IllegalArgumentException when the input contains malformed percent-encoded sequences (e.g., truncated %2 instead of %20). This is inconsistent with parseJson, which catches all Exception types. When parsing truncated form-urlencoded data that cuts in the middle of a percent-encoded character, the exception will propagate instead of returning a NetworkBody with BODY_PARSE_ERROR warning. The catch clause at line 164 needs to catch Exception to match the robust error handling in parseJson.
…ents (#15851) ## DESCRIBE YOUR PR [ANDROID-250: Fix Network Body parsing logic for large JSON bodies](https://linear.app/getsentry/issue/ANDROID-250/fix-network-body-parsing-logic-for-large-json-bodies) will cause a RuntimeException when parsing certain large JSON payloads. _SentryReplayOptions.MAX_NETWORK_BODY_SIZE <= json payload size < 2*SentryReplayOptions.MAX_NETWORK_BODY_SIZE_ 1. Fixed in [#4958](getsentry/sentry-java#4958) so updating docs for minimum required sdk version. 2. Also adding reference to required gradle version. ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [x] Other deadline: _ASAP - minor change, would like to share docs with potential adopters._ - [ ] None: Not urgent, can wait up to 1 week+ ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [x] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs)
📜 Description
This PR refactors the
NetworkBodyclass and improves how network request/response bodies are parsed and handled, particularly for large or truncated content.Key changes:
NetworkBodyfrom an interface with multiple implementations to a single class with abodyfield and optionalwarningslistNetworkBodyWarningenum to communicate parsing issues (JSON_TRUNCATED, TEXT_TRUNCATED, INVALID_JSON, BODY_PARSE_ERROR)NetworkBodyParserto properly detect and mark truncated contentJsonObjectReaderto vendorJsonReaderfor more robust JSON parsingSentryOkHttpInterceptorto peek at the correct amount of bytes to detect truncation💡 Motivation and Context
Previously, the network body parsing had issues with:
This change provides a cleaner API and better error/warning reporting.
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
🤖 Generated with Claude Code