-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Support captureFeedback
#2230
Support captureFeedback
#2230
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2230 +/- ##
==========================================
+ Coverage 84.43% 85.08% +0.64%
==========================================
Files 248 241 -7
Lines 8811 8751 -60
==========================================
+ Hits 7440 7446 +6
+ Misses 1371 1305 -66 ☔ View full report in Codecov by Sentry. |
iOS Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e8603bb | 1240.85 ms | 1254.79 ms | 13.94 ms |
f172c4d | 1350.66 ms | 1408.49 ms | 57.83 ms |
d5fb969 | 1228.79 ms | 1256.17 ms | 27.38 ms |
f79eecf | 1210.25 ms | 1221.65 ms | 11.40 ms |
a609134 | 1254.50 ms | 1265.08 ms | 10.58 ms |
f2db4ec | 1244.14 ms | 1259.79 ms | 15.65 ms |
24f71aa | 1267.47 ms | 1272.00 ms | 4.53 ms |
c3e6c82 | 1256.93 ms | 1276.17 ms | 19.24 ms |
613760b | 1263.10 ms | 1277.27 ms | 14.16 ms |
636cb61 | 1266.06 ms | 1271.38 ms | 5.31 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e8603bb | 8.33 MiB | 9.40 MiB | 1.07 MiB |
f172c4d | 8.33 MiB | 9.62 MiB | 1.29 MiB |
d5fb969 | 8.33 MiB | 9.64 MiB | 1.31 MiB |
f79eecf | 8.29 MiB | 9.36 MiB | 1.07 MiB |
a609134 | 8.16 MiB | 9.16 MiB | 1.01 MiB |
f2db4ec | 8.10 MiB | 9.16 MiB | 1.07 MiB |
24f71aa | 8.10 MiB | 9.16 MiB | 1.07 MiB |
c3e6c82 | 8.32 MiB | 9.38 MiB | 1.06 MiB |
613760b | 8.15 MiB | 9.13 MiB | 1000.46 KiB |
636cb61 | 8.28 MiB | 9.34 MiB | 1.06 MiB |
Previous results on branch: feat/capture-feedback
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
34b2626 | 1263.36 ms | 1287.91 ms | 24.55 ms |
3114a70 | 1226.08 ms | 1253.48 ms | 27.40 ms |
d5c1f0d | 1231.00 ms | 1244.22 ms | 13.22 ms |
c86b10e | 1254.92 ms | 1269.71 ms | 14.80 ms |
eb98dd2 | 1242.64 ms | 1263.37 ms | 20.72 ms |
4bb3ceb | 1249.02 ms | 1281.71 ms | 32.69 ms |
06a4a5f | 1247.92 ms | 1267.12 ms | 19.21 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
34b2626 | 8.38 MiB | 9.73 MiB | 1.36 MiB |
3114a70 | 8.38 MiB | 9.74 MiB | 1.36 MiB |
d5c1f0d | 8.38 MiB | 9.71 MiB | 1.33 MiB |
c86b10e | 8.38 MiB | 9.71 MiB | 1.33 MiB |
eb98dd2 | 8.38 MiB | 9.74 MiB | 1.36 MiB |
4bb3ceb | 8.38 MiB | 9.71 MiB | 1.33 MiB |
06a4a5f | 8.38 MiB | 9.71 MiB | 1.33 MiB |
I'm really looking forward to this, to use it in https://pub.dev/packages/feedback Thanks for tackling it! |
@ueman on a side note, after this PR we are also looking into implementing the feedback widget maybe it makes sense to adopt some code from your package? but let's see I haven't taken a deep dive yet into that |
@buenaflor I'm not sure if i have all the pieces in place that we need, would you mind taking a quick look over it and giving me feedback? /cc ueman |
I would advise against using my package, at least as of know, as there are a couple of fundamental issues with it, that would require a lot of effort to fix. I would love to see those fixed though. As for the (Sentry) feedback widget, there's already a very simple but working implementation at https://github.com/getsentry/sentry-dart/blob/main/flutter/example/lib/user_feedback_dialog.dart With a bit of polishing (mainly around handling issues during sending the feedback, maybe UI adjustments), it should be quite easy to ship.
Unfortunately, I also already failed once to implement this spec. So I'm afraid I can't really help here :/ |
Android Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d301b11 | 337.96 ms | 395.04 ms | 57.08 ms |
11fb408 | 320.10 ms | 380.24 ms | 60.14 ms |
eb1a7c1 | 332.98 ms | 381.55 ms | 48.57 ms |
9f9f94f | 331.04 ms | 368.92 ms | 37.88 ms |
0bed04d | 382.15 ms | 458.33 ms | 76.18 ms |
95d0636 | 301.46 ms | 357.98 ms | 56.52 ms |
ebfead1 | 298.44 ms | 374.28 ms | 75.84 ms |
4477d2e | 392.75 ms | 472.69 ms | 79.94 ms |
daa1b33 | 366.98 ms | 451.59 ms | 84.61 ms |
824df58 | 436.68 ms | 548.80 ms | 112.12 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d301b11 | 6.06 MiB | 7.09 MiB | 1.03 MiB |
11fb408 | 6.06 MiB | 7.10 MiB | 1.04 MiB |
eb1a7c1 | 5.94 MiB | 6.92 MiB | 1005.76 KiB |
9f9f94f | 5.94 MiB | 6.95 MiB | 1.01 MiB |
0bed04d | 6.33 MiB | 7.30 MiB | 987.71 KiB |
95d0636 | 6.16 MiB | 7.14 MiB | 1007.32 KiB |
ebfead1 | 6.06 MiB | 7.03 MiB | 989.24 KiB |
4477d2e | 6.33 MiB | 7.26 MiB | 950.38 KiB |
daa1b33 | 6.27 MiB | 7.20 MiB | 956.36 KiB |
824df58 | 6.35 MiB | 7.35 MiB | 1021.71 KiB |
Previous results on branch: feat/capture-feedback
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
eb98dd2 | 463.65 ms | 521.66 ms | 58.01 ms |
c86b10e | 432.18 ms | 491.02 ms | 58.84 ms |
4bb3ceb | 416.62 ms | 421.69 ms | 5.07 ms |
34b2626 | 429.74 ms | 486.14 ms | 56.40 ms |
3114a70 | 487.19 ms | 536.04 ms | 48.85 ms |
06a4a5f | 416.80 ms | 480.00 ms | 63.20 ms |
d5c1f0d | 484.33 ms | 580.04 ms | 95.71 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
eb98dd2 | 6.49 MiB | 7.56 MiB | 1.07 MiB |
c86b10e | 6.52 MiB | 7.59 MiB | 1.06 MiB |
4bb3ceb | 6.52 MiB | 7.59 MiB | 1.06 MiB |
34b2626 | 6.52 MiB | 7.61 MiB | 1.09 MiB |
3114a70 | 6.49 MiB | 7.55 MiB | 1.07 MiB |
06a4a5f | 6.52 MiB | 7.59 MiB | 1.06 MiB |
d5c1f0d | 6.52 MiB | 7.59 MiB | 1.06 MiB |
@aliu39 we believe its most likely an SDK internal issue, we'll investigate it |
@buenaflor Waiting for getsentry/sentry-java#3687 |
@buenaflor getsentry/sentry-java#3687 was merged. Waiting for it beeing released, then we can finish here. |
Can't this be already merged? I mean the code works, and the Android dependency will be updated anyway as soon as it's release right? |
@ueman Think we could, but then we'd potentially block releases if Android takes longer than expected, no? |
@buenaflor What do you think? Should we merge this with the info that Android support will come later? Would be easier, as i have to merge in main every other day and fixing issues that arise. |
@denrase I'll do a release soon, either today or tomorrow, we can merge it afterwards so we can wait for the android release |
just tested and android works fine after bumping to Android will merge after tests are green |
📜 Description
Implementation based on https://develop.sentry.dev/application/feedback-architecture/#feedback-events
💡 Motivation and Context
Relates to #1593
💚 How did you test it?
Unit tests
📝 Checklist
sendDefaultPii
is enabled🔮 Next steps