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

Handle network errors #661

Merged
merged 4 commits into from
Feb 26, 2025
Merged

Handle network errors #661

merged 4 commits into from
Feb 26, 2025

Conversation

mxosman
Copy link
Contributor

@mxosman mxosman commented Feb 25, 2025

Description of the change

Handle network errors by wrapping the fetchMetrics function in a try-catch. This should throw a JS error which will hopefully be caught by our Sentry integration.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes [#XXXX]

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@mxosman mxosman added the enhancement New feature or request label Feb 25, 2025
@mxosman
Copy link
Contributor Author

mxosman commented Feb 25, 2025

I'm still trying to think of a good way to validate this will send an error to Sentry on prod - but, per the doc, if the Sentry integration picks up on JS errors, this should accomplish that.

},
method: "POST",

try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lilidworkin - thank you for exploring this and coming up with this solution!

@mxosman mxosman requested a review from lilidworkin February 25, 2025 22:56
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13533022774

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 78.595%

Totals Coverage Status
Change from base Build 13162198974: 0.01%
Covered Lines: 2138
Relevant Lines: 2559

💛 - Coveralls

@mxosman
Copy link
Contributor Author

mxosman commented Feb 26, 2025

We could deploy a version of this that throws an error to staging and test that way - I was playing around on the staging URL and was able to send an error through. Not ideal, but could validate this.

Copy link
Contributor

@lilidworkin lilidworkin left a comment

Choose a reason for hiding this comment

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

Thank you @mxosman, appreciate this !! Looks exactly like what I had in mind. I think it's okay to assume this works for now, not sure it's worth the effort of trying to figure out how to test.

@lilidworkin
Copy link
Contributor

Rather than making it the oncall responsibilty to check Sentry (which feels annoying given how infrequently we expect errors to happen here) @mxosman maybe you and I can just turn our Sentry alerts on for that project -- I was able to do that here:
Screenshot 2025-02-26 at 10 33 07 AM

@mxosman
Copy link
Contributor Author

mxosman commented Feb 26, 2025

Sounds great, Lili! Great call! I just turned on alerts for Sentry as well - thanks for the screenshot.

@mxosman mxosman merged commit ebef8fe into main Feb 26, 2025
15 checks passed
@mxosman mxosman deleted the mahmoud/handle-network-errors branch February 26, 2025 18:40
@lilidworkin
Copy link
Contributor

Thanks so much @mxosman !

If you can deploy this to prod when you get a chance, that would be great.

@mxosman
Copy link
Contributor Author

mxosman commented Feb 26, 2025

For sure! I was already on my way to do that and poked around in staging and noticed a strange crash happening unrelated to our work:
https://spotlight-staging.recidiviz.org/us-nd (when you click the Parole section, this is what shows)
Screenshot 2025-02-26 at 1 17 01 PM

It's not happening in prod though, so it might be that the backend deploy is out of sync in staging.

I wanted to do a deploy of the api to staging, but realize I don't have the updated env files - do you know where I can get those variables?

@mxosman
Copy link
Contributor Author

mxosman commented Feb 26, 2025

I'll deploy this to prod in the meantime, but thought it was worth resolving just to be extra safe. I think likely what happened was at some point both the backend and frontend were deployed directly to prod, and an updated version of the backend wasn't deployed to staging.

When I deployed my previous changes, they were purely FE only, so I didn't need to deploy the backend. This change should be fine to go directly to prod and I'll double check just to be safe/roll back if it shows the same crash.

@lilidworkin
Copy link
Contributor

Ok sounds good, thank you for noticing this and looking into it! You should be able to get the env variables from 1password in public-dashboard-untracked-config-files.zip, do you see them there?

@mxosman
Copy link
Contributor Author

mxosman commented Feb 26, 2025

OK - prod deploy was a success! No issues. :)

You should be able to get the env variables from 1password in public-dashboard-untracked-config-files.zip, do you see them there?

I don't see that zip file. I only have my private vault and the Justice Counts vault. I do see a March 7, 2022 spotlight-staging-config-files.zip in the JC vault - but, I imagine it might be stale by now.

Would you mind sharing yours with me? If you right click on it > click Share > Share and check "Can be viewed only 1 time" - it should be safe for me to access. Maybe via DM when Slack is back up and running?

@lilidworkin
Copy link
Contributor

Yes, will email you!

@mxosman
Copy link
Contributor Author

mxosman commented Feb 28, 2025

Quick update on the staging deploy of the backend: I'm still waiting to figure out permissions w/ Peter to be able to deploy via gcloud - the go/jit form isn't working to give me the necessary permissions even though he added the spotlight-staging project to the form - will let you know once this is all squared away.

@lilidworkin
Copy link
Contributor

@mxosman weren't you able to deploy Spotlight before? What changed?

@mxosman
Copy link
Contributor Author

mxosman commented Feb 28, 2025

Not the backend. The previous changes I made were purely FE so there was no need to deploy the BE. Deploying the BE involves using Google Cloud with the following command: gcloud app deploy gae-staging.yaml --project ... which requires an App Engine deployer permission.

@lilidworkin
Copy link
Contributor

Oh got it! Why do we need to deploy the backend? This PR only touches the FE?

@mxosman
Copy link
Contributor Author

mxosman commented Feb 28, 2025

This is to fix the unrelated crash that was going on in staging I mentioned before, just to keep things sync'd up. It makes it harder to test in staging with the crash, which I think is coming because the backend is out-of-sync in staging, but sync'd up in prod (my hunch anyway). Not super critical, but threw me for a loop when I was trying to test this in staging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants