-
-
Notifications
You must be signed in to change notification settings - Fork 344
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: Bump to Sentry Javascript V9 #4568
base: v7
Are you sure you want to change the base?
Conversation
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
35eb9e7+dirty | 1213.46 ms | 1221.42 ms | 7.96 ms |
987dc4d+dirty | 1216.13 ms | 1227.20 ms | 11.08 ms |
e54b1ed+dirty | 1227.33 ms | 1254.46 ms | 27.14 ms |
71f7309+dirty | 1222.02 ms | 1224.81 ms | 2.79 ms |
345e497+dirty | 1218.83 ms | 1218.73 ms | -0.10 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
35eb9e7+dirty | 2.63 MiB | 3.73 MiB | 1.10 MiB |
987dc4d+dirty | 2.63 MiB | 3.73 MiB | 1.10 MiB |
e54b1ed+dirty | 2.63 MiB | 3.68 MiB | 1.05 MiB |
71f7309+dirty | 2.63 MiB | 3.73 MiB | 1.10 MiB |
345e497+dirty | 2.63 MiB | 3.68 MiB | 1.05 MiB |
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
987dc4d | 422.12 ms | 478.34 ms | 56.22 ms |
345e497 | 397.98 ms | 423.40 ms | 25.42 ms |
35eb9e7 | 383.45 ms | 415.65 ms | 32.21 ms |
71f7309 | 412.45 ms | 431.62 ms | 19.18 ms |
e54b1ed | 434.79 ms | 429.52 ms | -5.27 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
987dc4d | 17.75 MiB | 20.10 MiB | 2.36 MiB |
345e497 | 17.75 MiB | 20.10 MiB | 2.36 MiB |
35eb9e7 | 17.75 MiB | 20.10 MiB | 2.36 MiB |
71f7309 | 17.75 MiB | 20.10 MiB | 2.36 MiB |
e54b1ed | 17.75 MiB | 20.10 MiB | 2.36 MiB |
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
987dc4d+dirty | 411.89 ms | 418.60 ms | 6.71 ms |
35eb9e7+dirty | 426.84 ms | 453.53 ms | 26.69 ms |
71f7309+dirty | 361.15 ms | 369.90 ms | 8.75 ms |
345e497+dirty | 375.33 ms | 377.23 ms | 1.90 ms |
e54b1ed+dirty | 435.06 ms | 429.19 ms | -5.87 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
987dc4d+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
35eb9e7+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
71f7309+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
345e497+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
e54b1ed+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
35eb9e7+dirty | 1219.31 ms | 1217.37 ms | -1.94 ms |
987dc4d+dirty | 1209.94 ms | 1215.25 ms | 5.31 ms |
e54b1ed+dirty | 1227.90 ms | 1230.92 ms | 3.02 ms |
71f7309+dirty | 1206.00 ms | 1214.28 ms | 8.28 ms |
345e497+dirty | 1210.67 ms | 1207.50 ms | -3.17 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
35eb9e7+dirty | 3.19 MiB | 4.30 MiB | 1.11 MiB |
987dc4d+dirty | 3.19 MiB | 4.30 MiB | 1.11 MiB |
e54b1ed+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
71f7309+dirty | 3.19 MiB | 4.30 MiB | 1.11 MiB |
345e497+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
I will wait for some information in regard to getsentry/sentry-javascript#15471 in case it's a problem on the JavaScript SDK |
@lucas-zimerman The How have you upgraded to the JS v9? We should review it and ensure only the The |
I fixed on 028dbf9, At the time there were a conflict with yarn.lock, and I created a new one. Next time I will get the target branch yarn.lock and install again on the PR |
Thank you @lucas-zimerman, now the |
@antonis @krystofwoldrich I'd say now it's ready for review :D |
|
CHANGELOG.md
Outdated
|
||
### Major Changes | ||
|
||
- Remove autoSessionTracking option |
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 Browser SDK now recommends directly using https://github.com/getsentry/sentry-javascript/blob/defeaaf99ff6180be714ae770ce1ed0da6243676/packages/browser/src/integrations/browsersession.ts#L12
RN SDK has also enableAutoSessionTracking
enableAutoSessionTracking?: boolean; |
autoSessionTracking
and enableAutoSessionTracking
were two unrelated flags.
We should make it clear to the users.
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.
In regard to this, should we remove the integration autoSessionTracking
when running natively?
Before it was only added when autoSessionTracking was added, but now its added by default.
We could exclude it when enableAutioSessionTracking is set to true and it's running on browser, what do you think?
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.
Yes, makes sense, let's add the browserSessionIntegration
when enableAutioSessionTracking
and we are running in a browser.
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.
We can also do this in a follow up PR.
packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
Outdated
Show resolved
Hide resolved
@@ -157,6 +157,7 @@ export function wrap<P extends Record<string, unknown>>( | |||
const profilerProps = { | |||
...(options?.profilerProps ?? {}), | |||
name: RootComponent.displayName ?? 'Root', | |||
updateProps: {} |
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.
Is this required? I don't know the prop.
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.
It was always required, but for for some reason it only started to complain once I started working on V9
https://github.com/getsentry/sentry-javascript/blob/6914a2453472b3683fe76e67e248414d7ec4ec06/packages/react/src/profiler.tsx#L24
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.
Okay, I see, thank you for the details.
Could you open a follow-up PR with options?.profilerProps
type update to exclude the update props and children. https://github.com/getsentry/sentry-javascript/blob/1048a437b09955d31118960624b6d58242389c45/packages/react/src/profiler.tsx#L158
jest.mock('react-native', () => { | ||
const RN = jest.requireActual('react-native'); | ||
RN.UIManager = {}; | ||
return RN; | ||
}); | ||
|
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.
Was it just not needed or does it break the test after the JS SDK upgrade?
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 test was failing with the mock
FAIL test/tracing/timetodisplaynative.web.test.tsx
● Test suite failed to run
TypeError: Cannot set property UIManager of #<Object> which has only a getter
1 | jest.mock('react-native', () => {
2 | const RN = jest.requireActual('react-native');
> 3 | RN.UIManager = {};
| ^
4 | return RN;
5 | });
6 | jest.mock('../../src/js/utils/rnlibraries', () => {
at test/tracing/timetodisplaynative.web.test.tsx:3:15
at Object.require (src/js/tracing/timetodisplaynative.tsx:3:1)
at Object.<anonymous> (test/tracing/timetodisplaynative.web.test.tsx:13:1)
Since removing the mock made the error to not happen again I opted to not mock it.
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 issue with removing the mock is that this test is now not ensuring the component is not throwing in react-native-web environment.
RN Web UIManager https://github.com/necolas/react-native-web/blob/fcbe2d1e9225282671e39f9f639e2cb04c7e1e65/packages/react-native-web/src/exports/UIManager/index.js#L55
has a different interface from the RN UIManager, for example, the hasViewManagerConfig
doesn't exist there.
I would prefer to update the mock in a way so we avoid the type error but still ensure we test for the web env.
Maybe we can mock the whole RN object, instead of overwritting the UIManager property.
Overall the PR looks good. Thank you for all the changes. I have just a few suggestions and some questions to understand the v9 changes. |
Looks good, thank you for all the comments, changes and explanations. Two things we should do in a follow-up PRs:
One item to finalize this PR:
|
@@ -6,7 +8,7 @@ export const useEncodePolyfill = (): void => { | |||
(RN_GLOBAL_OBJ.__SENTRY__ as Partial<(typeof RN_GLOBAL_OBJ)['__SENTRY__']>) = {}; | |||
} | |||
|
|||
RN_GLOBAL_OBJ.__SENTRY__.encodePolyfill = encodePolyfill; | |||
RN_GLOBAL_OBJ.__SENTRY__[SDK_VERSION].encodePolyfill = encodePolyfill; |
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 SDK_VERSION
is created as a side effect of creating Sentry Logger (https://github.com/getsentry/sentry-javascript/blob/bf323b111b7e7b781e60e6299e432ce89f144970/packages/core/src/utils-hoist/logger.ts#L102)
But it's not guaranteed that it will exist.
We should use https://github.com/getsentry/sentry-javascript/blob/d773cb7324480ed3cffc14504f0e41951e344d19/packages/core/src/carrier.ts#L42
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.
Also can be a follow up PR.
📢 Type of change
📜 Description
This PR bumps JavaScript to version V9 and also fixes any break changes found.
In short, normal users not experience any changes, as shown on the sample apps, there were no required changes for this bump.
Changes handled:
In regard to shutdownTimeout, we can keep it as an additional parameter or completely remove it if that is the plan.
💡 Motivation and Context
💚 How did you test it?
Sample App, CI, Sentry.io discover
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps
#skip-changelog for now