chore: enable sentry performance metrics #10720
Conversation
| @@ -163,4 +164,5 @@ const InnerApp = () => ( | |||
| </Providers> | |||
| ) | |||
|
|
|||
| export const App = codePush(codePushOptions)(InnerApp) | |||
| const SentryApp = Sentry.wrap(InnerApp) | |||
There was a problem hiding this comment.
possibly we should wrap the Main component rather than whole app 👀 double check. this may need to happen after setup is called
There was a problem hiding this comment.
this is recommended by the docs though: https://docs.sentry.io/platforms/react-native/tracing/instrumentation/automatic-instrumentation/
| @@ -38,3 +39,5 @@ export const HomeContainer = () => { | |||
|
|
|||
| return <HomeQueryRenderer /> | |||
| } | |||
|
|
|||
| export const HomeContainer = Sentry.withProfiler(InnerHomeContainer) | |||
There was a problem hiding this comment.
not 100% this is doing anything currently, double check, this should give us more react profile style metrics in home screen transactions, but I didn't see much of a difference before and after
| * artwork in the home screen will fire as `screen:home` instead of `screen:artwork`. | ||
| * To fix it we find the focused route and update the context name to the correct screen name. | ||
| * This should be updated if we change our navigation setup. | ||
| */ |
There was a problem hiding this comment.
funkiness do to our nav set up, let me know if the comment makes sense, the good news is most screens fire and get registered as transactions but we need to update the names manually
| @@ -17,7 +17,7 @@ if (__DEV__) { | |||
| } catch {} | |||
| } | |||
|
|
|||
| require("./src/app/system/errorReporting/sentrySetup").setupSentry({ environment: "bootstrap" }) | |||
| require("./src/app/system/errorReporting/setupSentry").setupSentry({ environment: "bootstrap" }) | |||
There was a problem hiding this comment.
I think we should probably remove this, I will do as separate pr to avoid too much in this one. I am guessing the intention was to initialize sentry early to catch errors early in app lifecycle, but I think we pay for it with complexity in our reporting having an extra env, and initializing 2x might be leading to subtler bugs in our reporting.
brainbicycle
changed the title
| } else { | ||
| toast.show("Sentry debugging disabled", "middle") | ||
| } | ||
| }, |
There was a problem hiding this comment.
since sentry set up is no longer in a hook because it needs to be earlier in the lifecycle, call it manually on change.
| @@ -54,6 +55,14 @@ if (__DEV__) { | |||
|
|
|||
| setupFlipper() | |||
|
|
|||
| // Sentry must be setup early in the app lifecycle to hook into navigation | |||
| const debugSentry = unsafe_getDevToggle("DTDebugSentry") | |||
There was a problem hiding this comment.
👏 gread idea adding a dev toggle
This PR resolves PHIRE-1142
Description
This enables support for starting to collect tracing data in sentry by hooking into our react navigation setup following the docs here.
You can see some metrics in our performance dashboard here.
Follow ups:
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.