-
-
Notifications
You must be signed in to change notification settings - Fork 356
Experimental: Android UI profiling #5518
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
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the 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.
Thank you for moving this forward @alwx 🙇
The code changes look good. I would suggest adding an Android UI profiling config to the sample app so that it is easier to test.
We could also add a feature changelog entry (e.g. similar to this) with a config example.
packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
Show resolved
Hide resolved
|
Android UI Profiling Options tests are failing |
lucas-zimerman
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.
The PR is looking good!
Once a changelog is added and the tests are fixed we should be good for merge.
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 398.09 ms | 409.18 ms | 11.09 ms |
| b7aa1aa+dirty | 324.73 ms | 327.76 ms | 3.03 ms |
| 785ffb1 | 471.92 ms | 460.96 ms | -10.96 ms |
| fdbea8b+dirty | 494.72 ms | 529.06 ms | 34.34 ms |
| d1bfbde+dirty | 478.88 ms | 505.52 ms | 26.64 ms |
| ee69ed5+dirty | 409.44 ms | 441.30 ms | 31.86 ms |
| 0d6e618+dirty | 414.00 ms | 416.90 ms | 2.90 ms |
| f8d19f8+dirty | 422.98 ms | 421.98 ms | -1.00 ms |
| 8e653ac+dirty | 360.28 ms | 372.04 ms | 11.76 ms |
| 170d5ea+dirty | 407.92 ms | 422.49 ms | 14.57 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| b7aa1aa+dirty | 17.75 MiB | 19.75 MiB | 2.00 MiB |
| 785ffb1 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
| fdbea8b+dirty | 43.75 MiB | 48.05 MiB | 4.29 MiB |
| d1bfbde+dirty | 43.75 MiB | 47.99 MiB | 4.24 MiB |
| ee69ed5+dirty | 43.75 MiB | 48.04 MiB | 4.29 MiB |
| 0d6e618+dirty | 43.75 MiB | 47.99 MiB | 4.24 MiB |
| f8d19f8+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| 8e653ac+dirty | 17.75 MiB | 19.75 MiB | 2.00 MiB |
| 170d5ea+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 411.84 ms | 470.18 ms | 58.34 ms |
| b7aa1aa+dirty | 281.02 ms | 317.53 ms | 36.51 ms |
| fdbea8b+dirty | 551.94 ms | 577.02 ms | 25.08 ms |
| a02e30b+dirty | 346.13 ms | 381.76 ms | 35.62 ms |
| c7f264b+dirty | 356.98 ms | 407.46 ms | 50.48 ms |
| d1bfbde+dirty | 438.90 ms | 494.82 ms | 55.92 ms |
| ee69ed5+dirty | 411.19 ms | 447.04 ms | 35.85 ms |
| 0d6e618+dirty | 369.02 ms | 387.69 ms | 18.67 ms |
| 46bd012+dirty | 333.76 ms | 359.24 ms | 25.48 ms |
| f8d19f8+dirty | 374.17 ms | 383.40 ms | 9.23 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1c38acd+dirty | 43.94 MiB | 48.91 MiB | 4.97 MiB |
| b7aa1aa+dirty | 7.15 MiB | 8.46 MiB | 1.31 MiB |
| fdbea8b+dirty | 43.94 MiB | 48.87 MiB | 4.93 MiB |
| a02e30b+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| c7f264b+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| d1bfbde+dirty | 43.94 MiB | 48.82 MiB | 4.88 MiB |
| ee69ed5+dirty | 43.94 MiB | 48.87 MiB | 4.93 MiB |
| 0d6e618+dirty | 43.94 MiB | 48.82 MiB | 4.88 MiB |
| 46bd012+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| f8d19f8+dirty | 43.94 MiB | 48.91 MiB | 4.97 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 1213.17 ms | 1216.33 ms | 3.17 ms |
| 46bd012+dirty | 1231.78 ms | 1212.30 ms | -19.47 ms |
| eb07ba3+dirty | 1214.49 ms | 1221.59 ms | 7.10 ms |
| 0b64753+dirty | 1225.77 ms | 1232.98 ms | 7.21 ms |
| 6a70a7e+dirty | 1231.40 ms | 1239.49 ms | 8.09 ms |
| 3401245+dirty | 1216.70 ms | 1241.44 ms | 24.74 ms |
| 2104bb9+dirty | 1221.63 ms | 1214.73 ms | -6.91 ms |
| 3e0a5f9+dirty | 1233.65 ms | 1239.10 ms | 5.45 ms |
| 3bd3f0d+dirty | 1230.18 ms | 1243.41 ms | 13.22 ms |
| 955f2eb+dirty | 1225.78 ms | 1239.27 ms | 13.49 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 46bd012+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| eb07ba3+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 0b64753+dirty | 3.19 MiB | 4.55 MiB | 1.36 MiB |
| 6a70a7e+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| 3401245+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 2104bb9+dirty | 3.19 MiB | 4.57 MiB | 1.38 MiB |
| 3e0a5f9+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 3bd3f0d+dirty | 3.19 MiB | 4.55 MiB | 1.37 MiB |
| 955f2eb+dirty | 3.19 MiB | 4.55 MiB | 1.36 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 1216.40 ms | 1210.47 ms | -5.93 ms |
| 46bd012+dirty | 1220.49 ms | 1226.89 ms | 6.40 ms |
| eb07ba3+dirty | 1222.46 ms | 1220.37 ms | -2.08 ms |
| 0b64753+dirty | 1232.49 ms | 1226.96 ms | -5.53 ms |
| 6a70a7e+dirty | 1225.82 ms | 1230.79 ms | 4.98 ms |
| 3401245+dirty | 1222.60 ms | 1223.06 ms | 0.46 ms |
| 2104bb9+dirty | 1222.94 ms | 1221.16 ms | -1.77 ms |
| 3e0a5f9+dirty | 1226.94 ms | 1230.02 ms | 3.08 ms |
| 3bd3f0d+dirty | 1231.51 ms | 1229.10 ms | -2.41 ms |
| 955f2eb+dirty | 1235.06 ms | 1253.88 ms | 18.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 46bd012+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| eb07ba3+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 0b64753+dirty | 2.63 MiB | 3.98 MiB | 1.35 MiB |
| 6a70a7e+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| 3401245+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 2104bb9+dirty | 2.63 MiB | 4.00 MiB | 1.37 MiB |
| 3e0a5f9+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 3bd3f0d+dirty | 2.63 MiB | 3.99 MiB | 1.35 MiB |
| 955f2eb+dirty | 2.63 MiB | 3.98 MiB | 1.35 MiB |
| 'options' in this._integrations[MOBILE_REPLAY_INTEGRATION_NAME] | ||
| ? (this._integrations[MOBILE_REPLAY_INTEGRATION_NAME] as ReturnType<typeof mobileReplayIntegration>).options | ||
| : undefined, | ||
| androidProfilingOptions: this._options._experiments?.androidProfilingOptions, |
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.
Bug: androidProfilingOptions are read from _experiments before they are moved there, causing them to be undefined when passed to the native SDK.
Severity: HIGH
Suggested Fix
In client.ts, modify the code to read androidProfilingOptions directly from the top-level options object. Change the access from this._options._experiments?.androidProfilingOptions to this._options.androidProfilingOptions.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/js/client.ts#L227
Potential issue: In `client.ts`, the `androidProfilingOptions` are incorrectly read from
`this._options._experiments?.androidProfilingOptions`. At this stage of the
initialization process, these options are still located at the top level of
`this._options` and have not yet been moved into the `_experiments` object. This move
happens later in `wrapper.ts`. As a result, `undefined` is passed to the native SDK,
which completely disables the Android UI profiling feature even when it is correctly
configured by the user.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| ### Features | ||
|
|
||
| - Experimental support of UI profiling on Android ([#5518](https://github.com/getsentry/sentry-react-native/pull/5518)) |
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: @alwx Since the feature is experimental and we don't have documentation published yet wdyt of adding a sample like the following and a link to the doc?
| - Experimental support of UI profiling on Android ([#5518](https://github.com/getsentry/sentry-react-native/pull/5518)) | |
| - Experimental support of UI profiling on Android ([#5518](https://github.com/getsentry/sentry-react-native/pull/5518)) | |
| ```typescript | |
| Sentry.init({ | |
| // other options... | |
| _experiments: { | |
| androidProfilingOptions: { | |
| profileSessionSampleRate: 1.0, | |
| lifecycle: 'trace', | |
| startOnAppStart: true, | |
| }, | |
| } | |
| }); | |
| ``` | |
| To learn more visit [the documentation](https://docs.sentry.io/platforms/android/profiling/) |
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.
@antonis the sample is coming, yes!
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.
LGTM. Thank you for your work on this 🙇
<!-- Use this checklist to make sure your PR is ready for merge. You may delete any sections you don't need. --> ## DESCRIBE YOUR PR This PR is to support changes in UI profiling for React Native done here: getsentry/sentry-react-native#5518 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [x] None: Not urgent, can wait up to 1 week+
Fixes #5516
📢 Type of change
📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps