-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
(2.1) Handles Capture feedback form network connection issues #4364
base: antonis/3859-newCaptureFeedbackAPI-Form
Are you sure you want to change the base?
(2.1) Handles Capture feedback form network connection issues #4364
Conversation
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a06f6ba+dirty | 381.50 ms | 429.77 ms | 48.27 ms |
1dd8d17+dirty | 383.20 ms | 432.62 ms | 49.41 ms |
d33790a+dirty | 404.87 ms | 473.06 ms | 68.19 ms |
a3ba405+dirty | 359.67 ms | 436.86 ms | 77.19 ms |
0781f75+dirty | 406.72 ms | 454.80 ms | 48.08 ms |
f4a5053+dirty | 391.02 ms | 427.04 ms | 36.02 ms |
03c9048+dirty | 397.35 ms | 417.73 ms | 20.37 ms |
50c70c0+dirty | 385.30 ms | 433.06 ms | 47.76 ms |
cadf235+dirty | 455.51 ms | 451.64 ms | -3.87 ms |
27e1bf3+dirty | 398.69 ms | 439.39 ms | 40.69 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a06f6ba+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
1dd8d17+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
d33790a+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
a3ba405+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
0781f75+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
f4a5053+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
03c9048+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
50c70c0+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
cadf235+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
27e1bf3+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
Previous results on branch: antonis/4359-Feedback-Form-NetworkError
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d232ce7+dirty | 401.80 ms | 423.51 ms | 21.71 ms |
274d5b5+dirty | 413.06 ms | 456.54 ms | 43.48 ms |
06ab320+dirty | 382.32 ms | 433.65 ms | 51.33 ms |
25507c5+dirty | 378.89 ms | 434.94 ms | 56.05 ms |
0131975+dirty | 402.71 ms | 454.00 ms | 51.29 ms |
5679aac+dirty | 403.96 ms | 470.57 ms | 66.61 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d232ce7+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
274d5b5+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
06ab320+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
25507c5+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
0131975+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
5679aac+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
50c70c0+dirty | 1228.06 ms | 1224.43 ms | -3.64 ms |
e0624b6+dirty | 1221.86 ms | 1226.42 ms | 4.57 ms |
d33790a+dirty | 1234.19 ms | 1231.76 ms | -2.43 ms |
a06f6ba+dirty | 1230.45 ms | 1227.09 ms | -3.36 ms |
27e1bf3+dirty | 1230.92 ms | 1232.33 ms | 1.41 ms |
f4a5053+dirty | 1225.32 ms | 1231.47 ms | 6.15 ms |
03c9048+dirty | 1235.37 ms | 1238.15 ms | 2.77 ms |
561640f+dirty | 1220.45 ms | 1227.02 ms | 6.57 ms |
26fc306+dirty | 1227.25 ms | 1225.85 ms | -1.40 ms |
1dd8d17+dirty | 1235.22 ms | 1218.96 ms | -16.27 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
50c70c0+dirty | 2.36 MiB | 3.11 MiB | 760.92 KiB |
e0624b6+dirty | 2.36 MiB | 3.11 MiB | 761.16 KiB |
d33790a+dirty | 2.36 MiB | 3.11 MiB | 761.06 KiB |
a06f6ba+dirty | 2.36 MiB | 3.11 MiB | 761.35 KiB |
27e1bf3+dirty | 2.36 MiB | 3.11 MiB | 761.03 KiB |
f4a5053+dirty | 2.36 MiB | 3.11 MiB | 761.72 KiB |
03c9048+dirty | 2.36 MiB | 3.11 MiB | 761.74 KiB |
561640f+dirty | 2.36 MiB | 3.11 MiB | 761.19 KiB |
26fc306+dirty | 2.36 MiB | 3.11 MiB | 761.18 KiB |
1dd8d17+dirty | 2.36 MiB | 3.11 MiB | 761.66 KiB |
Previous results on branch: antonis/4359-Feedback-Form-NetworkError
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
06ab320+dirty | 1222.00 ms | 1220.50 ms | -1.50 ms |
274d5b5+dirty | 1221.69 ms | 1227.56 ms | 5.87 ms |
0131975+dirty | 1231.33 ms | 1227.10 ms | -4.23 ms |
5679aac+dirty | 1237.88 ms | 1231.70 ms | -6.18 ms |
d232ce7+dirty | 1223.47 ms | 1226.79 ms | 3.32 ms |
25507c5+dirty | 1230.96 ms | 1229.94 ms | -1.02 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
06ab320+dirty | 2.36 MiB | 3.11 MiB | 761.14 KiB |
274d5b5+dirty | 2.36 MiB | 3.11 MiB | 761.55 KiB |
0131975+dirty | 2.36 MiB | 3.11 MiB | 761.34 KiB |
5679aac+dirty | 2.36 MiB | 3.11 MiB | 761.93 KiB |
d232ce7+dirty | 2.36 MiB | 3.11 MiB | 761.50 KiB |
25507c5+dirty | 2.36 MiB | 3.11 MiB | 761.92 KiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
50c70c0+dirty | 1226.61 ms | 1225.02 ms | -1.59 ms |
e0624b6+dirty | 1229.19 ms | 1232.18 ms | 3.00 ms |
d33790a+dirty | 1247.14 ms | 1242.86 ms | -4.28 ms |
a06f6ba+dirty | 1235.31 ms | 1238.76 ms | 3.45 ms |
27e1bf3+dirty | 1245.78 ms | 1244.38 ms | -1.40 ms |
f4a5053+dirty | 1233.04 ms | 1240.02 ms | 6.98 ms |
03c9048+dirty | 1231.52 ms | 1225.96 ms | -5.56 ms |
561640f+dirty | 1237.10 ms | 1229.59 ms | -7.51 ms |
26fc306+dirty | 1229.10 ms | 1227.88 ms | -1.22 ms |
1dd8d17+dirty | 1229.28 ms | 1224.92 ms | -4.36 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
50c70c0+dirty | 2.92 MiB | 3.67 MiB | 773.48 KiB |
e0624b6+dirty | 2.92 MiB | 3.67 MiB | 773.62 KiB |
d33790a+dirty | 2.92 MiB | 3.67 MiB | 773.59 KiB |
a06f6ba+dirty | 2.92 MiB | 3.67 MiB | 773.87 KiB |
27e1bf3+dirty | 2.92 MiB | 3.67 MiB | 773.54 KiB |
f4a5053+dirty | 2.92 MiB | 3.67 MiB | 774.18 KiB |
03c9048+dirty | 2.92 MiB | 3.67 MiB | 774.29 KiB |
561640f+dirty | 2.92 MiB | 3.67 MiB | 773.72 KiB |
26fc306+dirty | 2.92 MiB | 3.67 MiB | 773.77 KiB |
1dd8d17+dirty | 2.92 MiB | 3.67 MiB | 774.21 KiB |
Previous results on branch: antonis/4359-Feedback-Form-NetworkError
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
06ab320+dirty | 1235.38 ms | 1241.82 ms | 6.43 ms |
274d5b5+dirty | 1235.24 ms | 1234.41 ms | -0.84 ms |
0131975+dirty | 1242.96 ms | 1240.12 ms | -2.83 ms |
5679aac+dirty | 1242.61 ms | 1234.55 ms | -8.06 ms |
d232ce7+dirty | 1234.69 ms | 1240.02 ms | 5.33 ms |
25507c5+dirty | 1226.49 ms | 1228.98 ms | 2.49 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
06ab320+dirty | 2.92 MiB | 3.67 MiB | 773.58 KiB |
274d5b5+dirty | 2.92 MiB | 3.67 MiB | 774.02 KiB |
0131975+dirty | 2.92 MiB | 3.67 MiB | 773.85 KiB |
5679aac+dirty | 2.92 MiB | 3.67 MiB | 774.41 KiB |
d232ce7+dirty | 2.92 MiB | 3.67 MiB | 773.91 KiB |
25507c5+dirty | 2.92 MiB | 3.67 MiB | 774.58 KiB |
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
03c9048 | 500.96 ms | 486.65 ms | -14.31 ms |
561640f | 461.96 ms | 458.11 ms | -3.85 ms |
a3ba405 | 438.16 ms | 435.78 ms | -2.38 ms |
50c70c0 | 496.82 ms | 526.02 ms | 29.20 ms |
cadf235 | 462.20 ms | 463.34 ms | 1.14 ms |
f4a5053 | 478.22 ms | 458.35 ms | -19.87 ms |
1dd8d17 | 399.65 ms | 393.81 ms | -5.84 ms |
a06f6ba | 424.02 ms | 415.82 ms | -8.20 ms |
0781f75 | 452.32 ms | 457.22 ms | 4.91 ms |
e0624b6 | 447.67 ms | 441.08 ms | -6.59 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
03c9048 | 17.74 MiB | 20.10 MiB | 2.37 MiB |
561640f | 17.74 MiB | 20.09 MiB | 2.35 MiB |
a3ba405 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
50c70c0 | 17.74 MiB | 20.10 MiB | 2.36 MiB |
cadf235 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
f4a5053 | 17.74 MiB | 20.10 MiB | 2.36 MiB |
1dd8d17 | 17.74 MiB | 20.10 MiB | 2.36 MiB |
a06f6ba | 17.74 MiB | 20.09 MiB | 2.35 MiB |
0781f75 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
e0624b6 | 17.74 MiB | 20.10 MiB | 2.36 MiB |
Previous results on branch: antonis/4359-Feedback-Form-NetworkError
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0131975 | 436.28 ms | 427.60 ms | -8.68 ms |
274d5b5 | 436.44 ms | 436.02 ms | -0.42 ms |
25507c5 | 448.50 ms | 439.85 ms | -8.65 ms |
5679aac | 452.07 ms | 470.16 ms | 18.09 ms |
06ab320 | 438.67 ms | 425.40 ms | -13.27 ms |
d232ce7 | 434.55 ms | 442.12 ms | 7.57 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0131975 | 17.74 MiB | 20.10 MiB | 2.36 MiB |
274d5b5 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
25507c5 | 17.74 MiB | 20.10 MiB | 2.37 MiB |
5679aac | 17.74 MiB | 20.10 MiB | 2.37 MiB |
06ab320 | 17.74 MiB | 20.10 MiB | 2.36 MiB |
d232ce7 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
onDisconnected(); | ||
} | ||
} catch (error) { | ||
onDisconnected(); |
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: We should add an onError call and pass error as a parameter, so users know they are facing an issue that isn't unrelated to connectivity issues (for example, wrong dsn, server rejecting the feedback, some sdk error,...)
a suggestion for the error message: "Unable to send feedback due to an unexpected error."
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.
Good idea 👍
Added an onError method to communicate the unexpected error with 40b4fd3
…359-Feedback-Form-NetworkError
…359-Feedback-Form-NetworkError # Conflicts: # packages/core/src/js/feedback/FeedbackForm.tsx
I plan to iterate with the alternative approach since the latest JS SDK includes the SendFeedback fix |
…359-Feedback-Form-NetworkError
…359-Feedback-Form-NetworkError # Conflicts: # packages/core/src/js/feedback/FeedbackForm.tsx # packages/core/test/feedback/FeedbackForm.test.tsx
So this PR is blocked by #4383 ? |
…359-Feedback-Form-NetworkError
…359-Feedback-Form-NetworkError # Conflicts: # packages/core/src/js/feedback/FeedbackForm.types.ts
📢 Type of change
Based on #4328
📜 Description
Checks the network connection before sending user feedback.
Alternative approach
An alternative approach was initially tested that used
sendFeedback
function from the JS SDK that catches errors and returns related messages. The problem with that approach was that it always returned an error even when the call was successful. I think this might be due to a missing return statement on the promise resolve.Given that the JS SDK seems to only check for network errors I considered the current simple approach good enough for now. I've opened a PR getsentry/sentry-javascript#14683 to fix this on the JS side (related issue) and we can iterate when the next JS SDK is released.
Related react native code patch
💡 Motivation and Context
Fixes #4359
💚 How did you test it?
Manual testing, Unit tests
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps
#skip-changelog