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

Justin/sentry #55

Closed
wants to merge 6 commits into from
Closed

Justin/sentry #55

wants to merge 6 commits into from

Conversation

JustinMi
Copy link
Contributor

@JustinMi JustinMi commented Mar 28, 2020

What's new in this PR

I integrated Sentry 🤖

There's lots of random diffs due to auto-linting, but there's only 2 files that I actually changed: App.js and app.json. In App.js, I added a couple of lines at the top to initiate Sentry. In app.json, I added some specifications for a post-publish hook.

Edit: I also added the Sentry config info to .env.example, per Annie Ro's tip

Relevant Links

Online sources

https://forum.sentry.io/t/set-release-after-init/6759/2

How to review

To test, you gotta first install Sentry. I think you can just yarn install on my branch, but if that doesn't work, then do yarn add sentry-expo.

Next, log into Sentry using the login info pinned in #chaos. Navigate to the dccentralkitchen project, where all the issues are logged. That's the page where you will check if Sentry actually catches errors.

Next, add Sentry config info into to your .env.development. You can find them in .env.example. They look like this:

...
SENTRY_ORG='...'
SENTRY_PROJECT='...'
SENTRY_AUTH_TOKEN='...'

You can use the SENTRY_AUTH_TOKEN is an org-wide token on Sentry, not user-specific. That way if we add/remove people to Sentry we don't gotta fiddle with token stuff. To find it: assuming you have access to the Blueprint org on Sentry, then go to sentry.io, and hit: Settings -> Developer Settings -> Internal Integrations -> DC Central Kitchen Auth Token -> Tokens to find it.

Finally, create an error. One thing I did was drop <button onClick={methodDoesNotExist}>Break the app</button>; somewhere in WelcomeScreen.js, and then I opened the app the in Expo simulator. You should see the app display a red error screen, and the error will also pop up as an Issue on the Sentry page. Note that if you repeatedly trigger the same error it logs under the same Issue, it doesn't create a new Issue. That confused me at first b/c it's not immediately obvious in the UI that the Issue updated.

Next steps

I'll need to add this to dccentralkitchen-clerks as well

Screenshots

What I did to elicit an error:
image

What the iOS simulator should show:
image

What the terminal should show:
image

What an Issue looks like in Sentry:
image

CC: @anniero98 @wangannie

.eslintrc.js Outdated Show resolved Hide resolved
@JustinMi JustinMi self-assigned this Mar 28, 2020
@JustinMi JustinMi requested a review from averyyip March 28, 2020 19:14
@@ -13,7 +13,7 @@ const apiKey = process.env.AIRTABLE_API_KEY;
const baseId = process.env.AIRTABLE_BASE_ID;

const BASE = new Airtable({
apiKey
apiKey,
Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries about the trailing comma diffs - it gets updated in #53 (but odd that your prettier is using a different config; I added a workspace settings.json in the same PR so hopefully that fixes it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I leave them as is then?

@annieyro
Copy link
Collaborator

Looks good! (With low context haha)

Could you add the new config variables to .env.example too? Also, not sure if we want the token to live in this PR description (does it need to be secret?)

Also, is it possible to have an example of creating an error in this PR (I don't really know how it works still 😅 , but maybe in one of the catch blocks for the Airtable calls (in ____Screen.js)? 😮

@JustinMi
Copy link
Contributor Author

Ok, added to .env.example and deleted the token from the description 👍good catch

I also updated the PR description to make the testing instructions more detailed. I forgot to add that you need to install Sentry in yarn LOL. Also added more pictures! LMK if that helps

App.js Outdated Show resolved Hide resolved
@lvlcfm lvlcfm self-requested a review April 7, 2020 06:55
Copy link
Member

@lvlcfm lvlcfm left a comment

Choose a reason for hiding this comment

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

cc @wangannie
Awesome work Justin!!!

We should continue working on this PR, but I think we might want to either create a new PR or merge in changes from master after #76 gets merged in since it's a breaking change.

Awesome job again Justin! A few items we need to add for sentry logging:

  1. We need to add a section in the log-in flow of the app where we. can capture the user in sentry (this is kind of like registering the user on sentry, so that all errors that are associated with a user can be identified), we'll need to add a configureScope call on log-in -- here is an example (associated with the user might be their airtable record_id + other unique identifying information (usually it would be an email or userid).
Sentry.configureScope(function(scope) {
  scope.setUser({
    id: six,
    email: "[email protected]"
  });
});
  1. Let's create a logUtils.js file and create a wrapper function that wraps around the Sentry.captureException(). For initial rollout, let's apply it to the .catch section in mapUtils.js (as a start for adding logging) example of the captureException method.

After this PR is complete -- we'll want to one more PR to add logging to all the airtable routes

package.json Show resolved Hide resolved
@wangannie wangannie closed this Apr 9, 2020
@annieyro annieyro deleted the justin/sentry branch April 10, 2020 09:40
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.

5 participants