Skip to content

LG-8514: PO Search: Handle unhandled timeout errors#7559

Merged
allthesignals merged 3 commits intomainfrom
wmg/8514-error-msg
Jan 3, 2023
Merged

LG-8514: PO Search: Handle unhandled timeout errors#7559
allthesignals merged 3 commits intomainfrom
wmg/8514-error-msg

Conversation

@allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Dec 30, 2022

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

USPS facilities search controller handles all exceptions by returning static pilot locations as a fallback. Logs errors to NewRelic. PR a temporary fallback to avoid entirely blocking a user from proofing.

Feedback from @tomas-nava:

  1. when the controller returns the pilot locations after it rescues the error, the front end shouldn't show the "There are 7 participating Post Offices within 50 miles of..." message. We can maybe check with Kelli on Tuesday morning about appropriate text for that message, or whether it should just be left blank (I'd leave it blank if you make the change this evening).
  2. we should null out the distance values in the config/ipp_pilot_usps_facilities.json pilot locations file, and the front end, if it gets null distance values, should not try to display distances.

Changes here address those concerns.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Test spec updated accordingly
  • Manual tested locally

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

After:

image

@allthesignals allthesignals requested review from a team and tomas-nava December 30, 2022 18:10
@allthesignals allthesignals merged commit 5291892 into main Jan 3, 2023
@allthesignals allthesignals deleted the wmg/8514-error-msg branch January 3, 2023 20:40
rescue Faraday::ConnectionFailed => _error
nil
rescue => err
Rails.logger.warn(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to log this in a more structured way, such as

Rails.logger.warn(exception_class: err.class, exception_message: err.message)

That way we can easily query these exception messages in cloudwatch later, eg make a table of exception classes by count. I don't think the default stringification of exceptions will make it easy on us if we're trying to sort through hundreds of error messages down the road

And then update the spec to check for a more specific error message instead of just that warn was called

Copy link
Contributor

Choose a reason for hiding this comment

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

if we did that, I think we need to throw a {}.to_json around that, but otherwise yes, big fan of structured logs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add this as part of an upcoming ticket so we can make sure we're getting useful info.

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.

3 participants