-
-
Notifications
You must be signed in to change notification settings - Fork 362
refactor: Add nullability handling for data serialization #5742
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
❌ 9 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f8029e2 | 1245.16 ms | 1261.32 ms | 16.16 ms |
2481950 | 1221.04 ms | 1248.98 ms | 27.94 ms |
15c8cd5 | 1220.24 ms | 1246.14 ms | 25.90 ms |
e64d3d4 | 1241.90 ms | 1260.10 ms | 18.20 ms |
fac4ca3 | 1222.81 ms | 1235.83 ms | 13.02 ms |
51f74d7 | 1236.31 ms | 1247.43 ms | 11.12 ms |
5c5648e | 1234.44 ms | 1253.79 ms | 19.35 ms |
5ec90e0 | 1235.57 ms | 1258.45 ms | 22.88 ms |
7148f97 | 1235.09 ms | 1258.07 ms | 22.98 ms |
c63e0fe | 1230.58 ms | 1253.94 ms | 23.35 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f8029e2 | 23.75 KiB | 893.72 KiB | 869.97 KiB |
2481950 | 23.74 KiB | 872.74 KiB | 849.00 KiB |
15c8cd5 | 23.75 KiB | 908.01 KiB | 884.26 KiB |
e64d3d4 | 23.75 KiB | 855.37 KiB | 831.62 KiB |
fac4ca3 | 23.75 KiB | 902.01 KiB | 878.27 KiB |
51f74d7 | 23.74 KiB | 874.08 KiB | 850.34 KiB |
5c5648e | 23.75 KiB | 879.60 KiB | 855.86 KiB |
5ec90e0 | 23.74 KiB | 872.67 KiB | 848.92 KiB |
7148f97 | 23.75 KiB | 854.78 KiB | 831.03 KiB |
c63e0fe | 23.74 KiB | 874.08 KiB | 850.33 KiB |
Sources/Sentry/SentryEnvelope.m
Outdated
@"replay_video" : videoURL | ||
NSMutableDictionary *envelopeContent = [NSMutableDictionary dictionary]; | ||
envelopeContent[@"replay_event"] = replayEventData; | ||
if (nil != recording) { |
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.
Instead of skipping the recording data, it might make sense to abort the envelope initialization instead. WDYT?
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.
If a replay event without a recording is still somewhat useful, then I would say keep the envelope with only the replay event. If Relay or the backend discards such envelopes, then we could throw away the envelope here already.
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 asked internally to find the best approach for this case.
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
Sources/Sentry/SentryEnvelope.m
Outdated
@"replay_video" : videoURL | ||
NSMutableDictionary *envelopeContent = [NSMutableDictionary dictionary]; | ||
envelopeContent[@"replay_event"] = replayEventData; | ||
if (nil != recording) { |
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.
If a replay event without a recording is still somewhat useful, then I would say keep the envelope with only the replay event. If Relay or the backend discards such envelopes, then we could throw away the envelope here already.
Result from internal discussion: When encoding of replay data fails, it should be propagated to the caller, to then drop the envelope and emit a client report. |
@philprime, we could also move the relay data serialization to its own PR. So basically, remove it from this PR and follow up. Then we can already merge this. Up to you if it's worth the effort. |
@philipphofmann I looked into the propagation of the encoding result to report the dropped event. The PR is a work-in-progress, but the main problem I encountered is, that right now we if we return If I leave the changes of the My idea would be changing the WDYT? |
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
This PR is derived from #5572 in an effort to make the large amount of changes easier to review for #5577.
Should be merged after #5737
Fixes nullability handling and tests for the
SentrySerialization
andSentryEnvelope
#skip-changelog