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

Sentry: initialize on app start, and add utility for logging to it #1476

Merged
merged 3 commits into from
Jul 7, 2020

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Jul 6, 2020

See #1457 (comment).

I didn't add the SENTRY_DOMAIN configuration yet, because I couldn't find an easy, obvious way to match window.location.href with a wildcard rule like *.aragon.org (though certainly this must exist and be easy!).


May need to be updated if #1475 is merged.

src/index.js Outdated
@@ -1,6 +1,9 @@
import 'core-js/stable'
import 'regenerator-runtime/runtime'

// Initialize Sentry immediately if enabled
import './sentry'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we feel like doing this is a bit sketchy / hidden? Would it be better if it were done directly in the index.js instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about exporting a initSentry() function that would get called from here? Or is it to capture errors on import for react, react-dom and @aragon/ui?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, no good reason :).

I was going back and forth a couple times on it, but agree with you guys that mangling the global space on an import like this is probably less of a good idea.

@auto-assign auto-assign bot requested review from bpierre and Evalir July 6, 2020 20:36
if (process.env.NODE_ENV !== 'production') {
if (
process.env.NODE_ENV !== 'production' &&
process.env.NODE_ENV !== 'test'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a few tests (routing) that are causing the logger to emit statements during tests, so this turns them off.

Copy link
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

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

LGTM

src/index.js Outdated
@@ -1,6 +1,9 @@
import 'core-js/stable'
import 'regenerator-runtime/runtime'

// Initialize Sentry immediately if enabled
import './sentry'
Copy link
Contributor

Choose a reason for hiding this comment

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

What about exporting a initSentry() function that would get called from here? Or is it to capture errors on import for react, react-dom and @aragon/ui?

Copy link
Contributor

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

Same opinion as Pierre! (as discussed offline)

@sohkai sohkai merged commit 02d4ebc into master Jul 7, 2020
@sohkai sohkai deleted the sentry-configuration branch July 7, 2020 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants