Skip to content
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

feat: Add new captureFeedback API to RN SDK #4320

Merged
merged 13 commits into from
Dec 2, 2024

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Nov 27, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Notes:

💡 Motivation and Context

Fixes #3859 (part of #4302)

💚 How did you test it?

CI, Manual testing with the sample app:

Screenshot 2024-11-27 at 1 58 32 PM

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

Copy link
Contributor

github-actions bot commented Nov 27, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 2bb104b

Copy link
Contributor

github-actions bot commented Nov 27, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 473.12 ms 490.78 ms 17.66 ms
Size 17.74 MiB 20.09 MiB 2.35 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
728164b 414.34 ms 449.22 ms 34.88 ms
484813b 434.55 ms 452.31 ms 17.75 ms
86d6d2c+dirty 332.90 ms 352.45 ms 19.55 ms
7fd512a 442.18 ms 437.57 ms -4.61 ms
9cd0e9f 449.65 ms 433.39 ms -16.26 ms
ad6c299 375.94 ms 382.02 ms 6.08 ms
0d3e677 422.82 ms 411.90 ms -10.92 ms
d8668ce 452.13 ms 435.06 ms -17.07 ms
0677344 327.74 ms 337.14 ms 9.40 ms
e540498 436.26 ms 433.00 ms -3.26 ms

App size

Revision Plain With Sentry Diff
728164b 17.73 MiB 19.85 MiB 2.12 MiB
484813b 17.73 MiB 20.07 MiB 2.33 MiB
86d6d2c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
7fd512a 17.74 MiB 20.08 MiB 2.35 MiB
9cd0e9f 17.74 MiB 20.08 MiB 2.34 MiB
ad6c299 17.73 MiB 19.75 MiB 2.02 MiB
0d3e677 17.74 MiB 20.07 MiB 2.34 MiB
d8668ce 17.74 MiB 20.08 MiB 2.34 MiB
0677344 17.73 MiB 19.81 MiB 2.07 MiB
e540498 17.73 MiB 20.11 MiB 2.37 MiB

Previous results on branch: antonis/3859-newCaptureFeedbackAPI

Startup times

Revision Plain With Sentry Diff
a673305 436.21 ms 443.04 ms 6.84 ms
b2f7f82 436.78 ms 435.20 ms -1.58 ms
68362f2 461.20 ms 457.04 ms -4.16 ms
beb77ad 510.00 ms 534.31 ms 24.31 ms

App size

Revision Plain With Sentry Diff
a673305 17.74 MiB 20.09 MiB 2.35 MiB
b2f7f82 17.74 MiB 20.09 MiB 2.35 MiB
68362f2 17.74 MiB 20.09 MiB 2.35 MiB
beb77ad 17.74 MiB 20.09 MiB 2.35 MiB

@antonis antonis marked this pull request as ready for review November 27, 2024 12:30
Copy link
Contributor

github-actions bot commented Nov 27, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 401.81 ms 426.80 ms 24.99 ms
Size 7.15 MiB 8.37 MiB 1.22 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
a989877+dirty 383.04 ms 400.92 ms 17.88 ms
e1ea4a8+dirty 451.98 ms 497.58 ms 45.60 ms
5bb8d5f+dirty 356.71 ms 389.65 ms 32.94 ms
86d6d2c+dirty 267.21 ms 325.24 ms 58.04 ms
e2b64fe+dirty 258.82 ms 304.26 ms 45.44 ms
f06c879+dirty 361.27 ms 407.88 ms 46.61 ms
4a6664f+dirty 357.02 ms 394.91 ms 37.89 ms
ac41368+dirty 395.91 ms 451.17 ms 55.26 ms
9cab16b+dirty 370.82 ms 416.37 ms 45.55 ms
63ed251+dirty 485.02 ms 531.16 ms 46.14 ms

App size

Revision Plain With Sentry Diff
a989877+dirty 7.15 MiB 8.35 MiB 1.20 MiB
e1ea4a8+dirty 7.15 MiB 8.35 MiB 1.20 MiB
5bb8d5f+dirty 7.15 MiB 8.21 MiB 1.06 MiB
86d6d2c+dirty 7.15 MiB 8.09 MiB 962.69 KiB
e2b64fe+dirty 7.15 MiB 8.07 MiB 947.16 KiB
f06c879+dirty 7.15 MiB 8.12 MiB 997.78 KiB
4a6664f+dirty 7.15 MiB 8.22 MiB 1.07 MiB
ac41368+dirty 7.15 MiB 8.39 MiB 1.24 MiB
9cab16b+dirty 7.15 MiB 8.35 MiB 1.20 MiB
63ed251+dirty 7.15 MiB 8.35 MiB 1.20 MiB

Previous results on branch: antonis/3859-newCaptureFeedbackAPI

Startup times

Revision Plain With Sentry Diff
b2f7f82+dirty 377.85 ms 440.47 ms 62.61 ms
beb77ad+dirty 393.65 ms 451.71 ms 58.06 ms
a673305+dirty 369.77 ms 414.12 ms 44.36 ms
68362f2+dirty 344.64 ms 400.24 ms 55.59 ms

App size

Revision Plain With Sentry Diff
b2f7f82+dirty 7.15 MiB 8.37 MiB 1.22 MiB
beb77ad+dirty 7.15 MiB 8.37 MiB 1.22 MiB
a673305+dirty 7.15 MiB 8.37 MiB 1.22 MiB
68362f2+dirty 7.15 MiB 8.37 MiB 1.22 MiB

Copy link
Contributor

github-actions bot commented Nov 27, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1214.98 ms 1228.34 ms 13.36 ms
Size 2.36 MiB 3.11 MiB 759.80 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d7401ac+dirty 1252.38 ms 1275.04 ms 22.66 ms
07e58c9+dirty 1226.02 ms 1228.35 ms 2.33 ms
e22745e+dirty 1222.73 ms 1224.98 ms 2.25 ms
690220d+dirty 1228.27 ms 1233.55 ms 5.29 ms
575f9da+dirty 1266.22 ms 1274.84 ms 8.62 ms
728164b+dirty 1256.10 ms 1259.08 ms 2.98 ms
946a600+dirty 1238.20 ms 1236.85 ms -1.35 ms
57448c5+dirty 1228.73 ms 1235.90 ms 7.16 ms
52a8031+dirty 1280.88 ms 1289.78 ms 8.90 ms
15c80ab+dirty 1223.74 ms 1228.96 ms 5.22 ms

App size

Revision Plain With Sentry Diff
d7401ac+dirty 2.36 MiB 2.83 MiB 481.14 KiB
07e58c9+dirty 2.36 MiB 3.10 MiB 752.28 KiB
e22745e+dirty 2.36 MiB 3.10 MiB 752.32 KiB
690220d+dirty 2.36 MiB 3.10 MiB 753.57 KiB
575f9da+dirty 2.36 MiB 2.87 MiB 520.20 KiB
728164b+dirty 2.36 MiB 2.88 MiB 530.38 KiB
946a600+dirty 2.36 MiB 3.10 MiB 759.74 KiB
57448c5+dirty 2.36 MiB 3.10 MiB 752.34 KiB
52a8031+dirty 2.36 MiB 2.82 MiB 469.44 KiB
15c80ab+dirty 2.36 MiB 2.83 MiB 474.49 KiB

Previous results on branch: antonis/3859-newCaptureFeedbackAPI

Startup times

Revision Plain With Sentry Diff
b2f7f82+dirty 1224.27 ms 1232.24 ms 7.97 ms
68362f2+dirty 1236.22 ms 1228.98 ms -7.25 ms
beb77ad+dirty 1229.25 ms 1228.02 ms -1.23 ms
a673305+dirty 1221.76 ms 1224.72 ms 2.97 ms

App size

Revision Plain With Sentry Diff
b2f7f82+dirty 2.36 MiB 3.10 MiB 759.76 KiB
68362f2+dirty 2.36 MiB 3.11 MiB 759.82 KiB
beb77ad+dirty 2.36 MiB 3.10 MiB 759.77 KiB
a673305+dirty 2.36 MiB 3.11 MiB 759.83 KiB

@kahest
Copy link
Member

kahest commented Nov 27, 2024

@bruno-garcia FYI

Copy link
Contributor

github-actions bot commented Nov 27, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1245.86 ms 1244.17 ms -1.69 ms
Size 2.92 MiB 3.67 MiB 772.42 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d7401ac+dirty 1288.10 ms 1289.54 ms 1.44 ms
07e58c9+dirty 1216.42 ms 1210.60 ms -5.82 ms
e22745e+dirty 1246.02 ms 1233.60 ms -12.42 ms
690220d+dirty 1227.45 ms 1221.67 ms -5.78 ms
575f9da+dirty 1272.00 ms 1284.38 ms 12.38 ms
728164b+dirty 1280.06 ms 1285.26 ms 5.20 ms
946a600+dirty 1236.12 ms 1234.94 ms -1.18 ms
57448c5+dirty 1244.65 ms 1249.20 ms 4.55 ms
52a8031+dirty 1255.96 ms 1273.00 ms 17.04 ms
15c80ab+dirty 1248.41 ms 1251.24 ms 2.83 ms

App size

Revision Plain With Sentry Diff
d7401ac+dirty 2.92 MiB 3.40 MiB 488.06 KiB
07e58c9+dirty 2.92 MiB 3.66 MiB 756.65 KiB
e22745e+dirty 2.92 MiB 3.66 MiB 756.73 KiB
690220d+dirty 2.92 MiB 3.66 MiB 758.77 KiB
575f9da+dirty 2.92 MiB 3.43 MiB 524.26 KiB
728164b+dirty 2.92 MiB 3.44 MiB 533.26 KiB
946a600+dirty 2.92 MiB 3.67 MiB 772.40 KiB
57448c5+dirty 2.92 MiB 3.66 MiB 756.65 KiB
52a8031+dirty 2.92 MiB 3.38 MiB 475.71 KiB
15c80ab+dirty 2.92 MiB 3.39 MiB 481.56 KiB

Previous results on branch: antonis/3859-newCaptureFeedbackAPI

Startup times

Revision Plain With Sentry Diff
b2f7f82+dirty 1242.42 ms 1234.87 ms -7.54 ms
68362f2+dirty 1232.73 ms 1226.80 ms -5.92 ms
beb77ad+dirty 1228.63 ms 1222.55 ms -6.08 ms
a673305+dirty 1239.94 ms 1236.31 ms -3.63 ms

App size

Revision Plain With Sentry Diff
b2f7f82+dirty 2.92 MiB 3.67 MiB 772.44 KiB
68362f2+dirty 2.92 MiB 3.67 MiB 772.35 KiB
beb77ad+dirty 2.92 MiB 3.67 MiB 772.36 KiB
a673305+dirty 2.92 MiB 3.67 MiB 772.46 KiB

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All seems good, thank you for splitting the PR!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Krystof Woldrich <[email protected]>
@krystofwoldrich
Copy link
Member

https://github.com/getsentry/sentry-javascript/blob/dafd51054d8b2ab2030fa0b16ad0fd70493b6e08/packages/core/test/lib/feedback.test.ts#L14

Let's copy this tests of the API to our RN repository, to ensure it behaves as expected. I would like to start doing this going forward, as we encountered in the past bug caused by change of behavior of the API which was not obvious from the changelog and thus we missed it.

@antonis
Copy link
Collaborator Author

antonis commented Nov 28, 2024

https://github.com/getsentry/sentry-javascript/blob/dafd51054d8b2ab2030fa0b16ad0fd70493b6e08/packages/core/test/lib/feedback.test.ts#L14
Let's copy this tests of the API to our RN repository, to ensure it behaves as expected. I would like to start doing this going forward, as we encountered in the past bug caused by change of behavior of the API which was not obvious from the changelog and thus we missed it.

Copied the tests with f9d2b59

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect, thank you for the updates.

@krystofwoldrich krystofwoldrich merged commit 6c56eb1 into main Dec 2, 2024
62 checks passed
@krystofwoldrich krystofwoldrich deleted the antonis/3859-newCaptureFeedbackAPI branch December 2, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new captureFeedback API to RN SDK
4 participants