-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
ref: json serialization error reporting #2355
ref: json serialization error reporting #2355
Conversation
before: - test if input is valid JSON with `-[NSJSONSerialization isValidJSON]` and if not, build our own error. - problem: no hints as to why the JSON is invalid after: - attempt direct serialization with `-[NSJSONSerialization dataWithJSONObject:options:error:]` and use the library error - this can throw an exception in certain failure modes: surround the call with try/catch - use new functions in SentryError to create an NSError from either an NSException or the inout NSError - only compile like this for nonrelease builds so we don't incur the performance hit of throwing an exception (see apple's docs: "For best performance in 64-bit, you should throw exceptions only when absolutely necessary.")
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
347a8e9 | 1243.50 ms | 1265.90 ms | 22.40 ms |
8b040e4 | 1234.76 ms | 1244.71 ms | 9.95 ms |
bdfccaa | 1202.83 ms | 1228.96 ms | 26.13 ms |
075911d | 1256.73 ms | 1271.22 ms | 14.49 ms |
5025d2e | 1248.52 ms | 1251.72 ms | 3.20 ms |
4a0c282 | 1228.60 ms | 1250.74 ms | 22.14 ms |
d73ebd0 | 1200.83 ms | 1236.10 ms | 35.27 ms |
c2a9b60 | 1222.10 ms | 1240.62 ms | 18.52 ms |
5025d2e | 1245.14 ms | 1268.58 ms | 23.44 ms |
6177f2d | 1206.55 ms | 1226.20 ms | 19.65 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
347a8e9 | 20.51 KiB | 333.16 KiB | 312.65 KiB |
8b040e4 | 20.50 KiB | 333.54 KiB | 313.04 KiB |
bdfccaa | 20.50 KiB | 361.80 KiB | 341.29 KiB |
075911d | 20.51 KiB | 332.90 KiB | 312.39 KiB |
5025d2e | 20.51 KiB | 331.79 KiB | 311.28 KiB |
4a0c282 | 20.51 KiB | 333.10 KiB | 312.60 KiB |
d73ebd0 | 20.50 KiB | 365.18 KiB | 344.68 KiB |
c2a9b60 | 20.50 KiB | 333.54 KiB | 313.04 KiB |
5025d2e | 20.51 KiB | 331.79 KiB | 311.28 KiB |
6177f2d | 20.51 KiB | 332.90 KiB | 312.40 KiB |
Previous results on branch: armcknight/ref/json-serialization-error-reporting
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7083f18 | 1247.37 ms | 1280.32 ms | 32.95 ms |
fcf17d7 | 1245.12 ms | 1254.00 ms | 8.88 ms |
cfa9b01 | 1209.55 ms | 1235.36 ms | 25.81 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7083f18 | 20.50 KiB | 365.44 KiB | 344.94 KiB |
fcf17d7 | 20.50 KiB | 365.44 KiB | 344.94 KiB |
cfa9b01 | 20.76 KiB | 367.28 KiB | 346.53 KiB |
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
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.
Please add some tests for the new functionality.
Added some basic tests for the new functions and a test for the serializer with the actual scenario I encountered, trying to add a NSDate directly to a JSON dict for serialization. |
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.
Thanks for adding tests
No idea why the tests are failing with a compiler error that doesn't make sense and doesn't happen locally; rerunning ⏳ |
Even if I make the suggested changes locally, then my local build fails. Any idea what could be going on with this @philipphofmann? |
@brustolin thanks for pushing that commit, that one builds locally and on CI 👍🏻 |
…ents * master: build(deps): bump github/codeql-action from 2.1.31 to 2.1.32 (#2386) build(deps): bump fastlane from 2.210.1 to 2.211.0 (#2385) ref: json serialization error reporting (#2355) release: 7.31.0 ref: Fix outdated comment in SessionTracker (#2381) fix: Do not delete the app state (#2382) build: Split Swift and Clang format for pre-commit (#2380)
* master: (56 commits) meta: disable swiftlint file length check in TBDBClient.swift (#2435) Revert "test: shorten some tests (#2422)" (#2427) test: shorten some tests (#2422) test: include Sentry changes in hash keys (#2412) release: 7.31.2 fix: Crash in Client when reading integrations (#2398) test: tooling improvements (#2400) fix: Don't increase session's error count for dropped events (#2374) Update CHANGELOG.md (#2396) release: 7.31.1 Fix: Set the correct OOM event timestamp (#2394) test: Fix SentrySerializationTests.testSerializationFailsWithInvalidJSONObject (#2392) feat(hybrid-sdks): Add captureScreenshots to PrivateSDKOnly (#2384) ci: Increase test timeout for iOS 12 (#2391) test: Disable flaky testCrashReportCount1 (#2389) test: Disable flaky testSerializeWithUnderlyingNSException (#2387) build(deps): bump github/codeql-action from 2.1.31 to 2.1.32 (#2386) build(deps): bump fastlane from 2.210.1 to 2.211.0 (#2385) ref: json serialization error reporting (#2355) release: 7.31.0 ...
I had a JSON validation error, but couldn't tell what it was since we simply check for the boolean from
isValidJSON
. I had to implement just attempting the serialization and inspecting the inout error it gives, and learned that at least in my case, serialization also throws exceptions in certain cases, but the exception told me exactly what the problem was. I figured it would be helpful to keep around, but also that we may not want the exception handler in production, so I used some conditional compilation to keep it behaving as it was in release configs, but in all other configs, to get the info from the exception/error.To that end, added some new functions to construct NSErrors that can include an underlying NSError we may get from a Cocoa library method, or an NSException, with some light refactoring of the implementations in SentryError.
#skip-changelog
Just copying here some details from one of my commits for visibility: