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

chore(suite-native): make it simple to use Sentry locally in debug build #16356

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

matejkriz
Copy link
Member

Description

Turning on Sentry in the local debug build is now complicated and one has to rewrite several places. This PR tries to simplify it.

From now on it should be enough to switch EXPO_PUBLIC_IS_SENTRY_ENABLED in .env.development.local and do a prebuild.

@matejkriz matejkriz added sentry Reports linked from Sentry.io or Sentry enhancements mobile Suite Lite issues and PRs labels Jan 14, 2025
@@ -94,4 +93,4 @@ const PureApp = () => (
</GestureHandlerRootView>
);

export const App = isDebugEnv() ? PureApp : Sentry.wrap(PureApp);
export const App = Sentry.wrap(PureApp);
Copy link
Member Author

Choose a reason for hiding this comment

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

I know that we added that condition because of some warning during local development, but now I can't see that warning any more and I guess it is because I removed the condition for Sentry.init and just playing with the enabled property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought:

When we enable EXPO_PUBLIC_IS_SENTRY_ON_DEBUG_BUILD_ENABLED in .env, we wont see anywhere that it is active. Do we want handle someone forgetting to switch it off and spamming sentry from develop?

What about to log a warning, that sentry is enabled, so devs can notice better in such case? Wdyt? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

If you edit the ENV in git tracked file it's impossible to forget about. Worst case you accidentally commit it and CR catches it.
Do you have a non-git-tracked env file in suite-native? 🤔
If yes, then I'd add a comment here: # only modify it in *this file*, so you don't forget to turn it off

Copy link
Member Author

Choose a reason for hiding this comment

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

.env.development.local is not tracked in git and I think it's good to use it. I see no big harm in sending more events from debug build to sentry. It's super easy to filter them out in Sentry. If we start sending hundreds of thousands of events, than we can think about some optimization 😉

@matejkriz matejkriz marked this pull request as ready for review January 14, 2025 16:32
@matejkriz matejkriz requested a review from a team as a code owner January 14, 2025 16:32
@matejkriz matejkriz requested a review from Lemonexe January 14, 2025 16:33
Copy link

github-actions bot commented Jan 15, 2025

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 22
  • More info

Learn more about 𝝠 Expo Github Action

Copy link
Contributor

@Lemonexe Lemonexe left a comment

Choose a reason for hiding this comment

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

LGTM, now it's nice and clear. Tested ✔️

suite-native/app/src/SentryProvider.tsx Show resolved Hide resolved
@matejkriz matejkriz added the no-project This label is used to specify that PR doesn't need to be added to a project label Jan 15, 2025
@matejkriz
Copy link
Member Author

/rebase

Copy link

@trezor-ci trezor-ci force-pushed the chore/sentry-on-debug-env branch from f98d859 to 2a51eb9 Compare January 15, 2025 16:30
@matejkriz matejkriz requested a review from vdovhanych as a code owner January 15, 2025 16:40
@matejkriz matejkriz force-pushed the chore/sentry-on-debug-env branch from c44e15a to 7c56d99 Compare January 17, 2025 15:27
@matejkriz matejkriz merged commit ef64819 into develop Jan 18, 2025
35 checks passed
@matejkriz matejkriz deleted the chore/sentry-on-debug-env branch January 18, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Suite Lite issues and PRs no-project This label is used to specify that PR doesn't need to be added to a project sentry Reports linked from Sentry.io or Sentry enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants