-
-
Notifications
You must be signed in to change notification settings - Fork 361
fix(client): Fix attachment processor dropping additional attachments #5989
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5989 +/- ##
=============================================
+ Coverage 86.672% 86.726% +0.054%
=============================================
Files 424 424
Lines 36714 36713 -1
Branches 17363 17364 +1
=============================================
+ Hits 31821 31840 +19
+ Misses 4846 4827 -19
+ Partials 47 46 -1
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c5bedc0 | 1203.83 ms | 1236.17 ms | 32.33 ms |
aadbffe | 1228.73 ms | 1251.59 ms | 22.86 ms |
2de3f92 | 1207.56 ms | 1234.96 ms | 27.40 ms |
ed85746 | 1231.79 ms | 1248.55 ms | 16.75 ms |
21efa18 | 1227.73 ms | 1250.04 ms | 22.31 ms |
7148f97 | 1235.09 ms | 1258.07 ms | 22.98 ms |
d0f70ce | 1226.54 ms | 1247.04 ms | 20.50 ms |
5bf2b17 | 1213.53 ms | 1238.54 ms | 25.01 ms |
1339919 | 1214.82 ms | 1238.98 ms | 24.16 ms |
2a36c3f | 1227.27 ms | 1246.89 ms | 19.63 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c5bedc0 | 23.75 KiB | 920.63 KiB | 896.89 KiB |
aadbffe | 23.75 KiB | 912.77 KiB | 889.02 KiB |
2de3f92 | 23.75 KiB | 919.69 KiB | 895.94 KiB |
ed85746 | 23.75 KiB | 920.83 KiB | 897.08 KiB |
21efa18 | 23.75 KiB | 919.70 KiB | 895.95 KiB |
7148f97 | 23.75 KiB | 854.78 KiB | 831.03 KiB |
d0f70ce | 23.75 KiB | 913.09 KiB | 889.34 KiB |
5bf2b17 | 23.75 KiB | 913.27 KiB | 889.52 KiB |
1339919 | 23.75 KiB | 919.70 KiB | 895.95 KiB |
2a36c3f | 23.75 KiB | 874.45 KiB | 850.71 KiB |
Previous results on branch: philprime/fix-attachment-processing
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6c9100d | 1239.02 ms | 1263.27 ms | 24.24 ms |
0421faf | 1223.55 ms | 1247.63 ms | 24.08 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6c9100d | 23.74 KiB | 926.48 KiB | 902.73 KiB |
0421faf | 23.75 KiB | 926.72 KiB | 902.96 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
📜 Description
Changes the attachment processors to reuse the output of previous integrations, to not drop attachments.
💡 Motivation and Context
When enabling the options
attachScreenshot
andattachViewHierarchy
, the two respective integrationsSentryScreenshotIntegration
andSentryViewHierarchyIntegration
are installed respectively.The current implementation of the
[SentryClient processAttachmentsForEvent:attachments:]
is simply replacing theprocessedAttachments
containing the output of the previous integration, instead of merging the result, therefore only one integration can be used at a time.This PR changes the loop to call each processor with the output of previous ones. This allows processors to add and remove attachments of previous processors. This also introduces an implicit order of integrations based on the installation order in SentryOptionsInternal.m.
We are ignoring this order for now, as we only have two integrations processing attachments, and they are both only adding attachments to the result. I added a note in the code for this.
💚 How did you test it?
TBD
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.