-
-
Notifications
You must be signed in to change notification settings - Fork 354
fix: Set user geo to the native layer. #5302
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
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d1fd647+dirty | 413.02 ms | 459.72 ms | 46.70 ms |
| 170d5ea+dirty | 407.92 ms | 422.49 ms | 14.57 ms |
| 6479fd5+dirty | 412.95 ms | 434.02 ms | 21.07 ms |
| 1226664+dirty | 347.45 ms | 386.60 ms | 39.15 ms |
| 4604da9+dirty | 366.44 ms | 398.10 ms | 31.66 ms |
| 5ee3314+dirty | 415.80 ms | 426.14 ms | 10.34 ms |
| b3b5b0d | 399.82 ms | 419.20 ms | 19.38 ms |
| e2fa43d | 451.68 ms | 462.42 ms | 10.74 ms |
| 20d5eaa | 377.62 ms | 406.50 ms | 28.88 ms |
| 2adbd1e+dirty | 433.98 ms | 427.96 ms | -6.02 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d1fd647+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 170d5ea+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 6479fd5+dirty | 17.75 MiB | 19.68 MiB | 1.94 MiB |
| 1226664+dirty | 17.75 MiB | 19.74 MiB | 1.99 MiB |
| 4604da9+dirty | 17.75 MiB | 19.74 MiB | 2.00 MiB |
| 5ee3314+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| b3b5b0d | 17.75 MiB | 19.68 MiB | 1.94 MiB |
| e2fa43d | 17.75 MiB | 20.15 MiB | 2.41 MiB |
| 20d5eaa | 17.75 MiB | 20.15 MiB | 2.41 MiB |
| 2adbd1e+dirty | 17.75 MiB | 19.70 MiB | 1.96 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 955f2eb+dirty | 388.13 ms | 433.56 ms | 45.44 ms |
| 2104bb9+dirty | 313.00 ms | 309.76 ms | -3.24 ms |
| 69602ce+dirty | 375.37 ms | 405.28 ms | 29.91 ms |
| 11ded16+dirty | 309.23 ms | 310.55 ms | 1.33 ms |
| 7be1f99+dirty | 369.02 ms | 399.60 ms | 30.58 ms |
| 6fee48d+dirty | 370.23 ms | 427.86 ms | 57.63 ms |
| 3bd3f0d+dirty | 334.38 ms | 402.19 ms | 67.81 ms |
| 5526494+dirty | 380.79 ms | 432.70 ms | 51.91 ms |
| c94a927+dirty | 411.32 ms | 443.18 ms | 31.86 ms |
| 9f211e3+dirty | 371.00 ms | 432.51 ms | 61.51 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 955f2eb+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 2104bb9+dirty | 7.15 MiB | 8.46 MiB | 1.30 MiB |
| 69602ce+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 11ded16+dirty | 7.15 MiB | 8.46 MiB | 1.31 MiB |
| 7be1f99+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 6fee48d+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 3bd3f0d+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
| 5526494+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| c94a927+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
| 9f211e3+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d751a5d+dirty | 1215.57 ms | 1220.56 ms | 4.99 ms |
| c08359e+dirty | 1235.25 ms | 1233.96 ms | -1.29 ms |
| 49ef936+dirty | 1228.42 ms | 1217.09 ms | -11.33 ms |
| 6a70a7e+dirty | 1225.82 ms | 1230.79 ms | 4.98 ms |
| 69602ce+dirty | 1235.65 ms | 1230.82 ms | -4.83 ms |
| 1e7a472+dirty | 1223.39 ms | 1232.12 ms | 8.73 ms |
| 5c16cdc+dirty | 1209.32 ms | 1210.67 ms | 1.35 ms |
| 7be1f99+dirty | 1226.69 ms | 1217.76 ms | -8.93 ms |
| 4604da9+dirty | 1232.59 ms | 1232.26 ms | -0.33 ms |
| 07808fb+dirty | 1233.31 ms | 1232.77 ms | -0.54 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d751a5d+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| c08359e+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 49ef936+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| 6a70a7e+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| 69602ce+dirty | 2.63 MiB | 3.91 MiB | 1.28 MiB |
| 1e7a472+dirty | 2.63 MiB | 4.00 MiB | 1.36 MiB |
| 5c16cdc+dirty | 2.63 MiB | 3.96 MiB | 1.33 MiB |
| 7be1f99+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 4604da9+dirty | 2.63 MiB | 4.01 MiB | 1.38 MiB |
| 07808fb+dirty | 2.63 MiB | 3.99 MiB | 1.36 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d751a5d+dirty | 1212.22 ms | 1217.94 ms | 5.71 ms |
| c08359e+dirty | 1200.59 ms | 1211.81 ms | 11.22 ms |
| 49ef936+dirty | 1221.27 ms | 1221.60 ms | 0.34 ms |
| 6a70a7e+dirty | 1231.40 ms | 1239.49 ms | 8.09 ms |
| 69602ce+dirty | 1230.59 ms | 1230.84 ms | 0.24 ms |
| 1e7a472+dirty | 1237.44 ms | 1231.14 ms | -6.29 ms |
| 5c16cdc+dirty | 1235.67 ms | 1241.18 ms | 5.51 ms |
| 7be1f99+dirty | 1222.43 ms | 1217.15 ms | -5.28 ms |
| 4604da9+dirty | 1208.67 ms | 1208.12 ms | -0.54 ms |
| 07808fb+dirty | 1240.76 ms | 1251.00 ms | 10.24 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d751a5d+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| c08359e+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 49ef936+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| 6a70a7e+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| 69602ce+dirty | 3.19 MiB | 4.48 MiB | 1.29 MiB |
| 1e7a472+dirty | 3.19 MiB | 4.56 MiB | 1.38 MiB |
| 5c16cdc+dirty | 3.19 MiB | 4.53 MiB | 1.34 MiB |
| 7be1f99+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 4604da9+dirty | 3.19 MiB | 4.58 MiB | 1.39 MiB |
| 07808fb+dirty | 3.19 MiB | 4.56 MiB | 1.37 MiB |
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.
nit: I think we don't need to change the stub.
packages/core/RNSentryAndroidTester/app/src/test/java/io/sentry/react/RNSentryModuleImplTest.kt
Show resolved
Hide resolved
antonis
left a comment
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.
Thank you for adding this @lucas-zimerman 🙇
The changes on this PR LGTM. I've added a comment related to the tests here but I'm ok to handle this on a separate PR since we are missing test coverage on this part of the code already.
Lets do on a follow up PR |
antonis
left a comment
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.
Sounds good @lucas-zimerman 👍
📢 Type of change
📜 Description
This field could be set previously on the JavaScript side but was never synced to Native. This PR syncs this data to the native layer.
💡 Motivation and Context
💚 How did you test it?
on devices.
iOS https://sentry-sdks.sentry.io/issues/6928109444/events/d9ae49c70ba2489697e46872b15057e2/?project=5428561
Android: https://sentry-sdks.sentry.io/issues/6928109444/events/528c011466274c209de07ee2b00b0a64/?project=5428561
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps
Close #2996