Skip to content

LG-7706: Log unexpected bad request errors to new relic#7257

Merged
sheldon-b merged 2 commits intomainfrom
sbachstein/lg-7706-add-newrelic-logging-for-bad-request-errors
Nov 1, 2022
Merged

LG-7706: Log unexpected bad request errors to new relic#7257
sheldon-b merged 2 commits intomainfrom
sbachstein/lg-7706-add-newrelic-logging-for-bad-request-errors

Conversation

@sheldon-b
Copy link
Contributor

🎫 Ticket

LG-7706

🛠 Summary of changes

When the USPS polling job encounters an unexpected bad request error begin logging it to NewRelic

Sheldon Bachstein added 2 commits October 31, 2022 15:22
changelog: Improvements, In-person proofing, log more error details
@sheldon-b sheldon-b marked this pull request as ready for review November 1, 2022 13:51
@sheldon-b sheldon-b requested review from a team, NavaTim, allthesignals and eileen-nava and removed request for a team and eileen-nava November 1, 2022 13:51
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +588 to +592
it 'logs the error to NewRelic' do
expect(NewRelic::Agent).to receive(:notice_error).
with(instance_of(Faraday::BadRequestError))
job.perform(Time.zone.now)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want "logs the error to NewRelic" to be an assertion as part of the shared example set enrollment_encountering_an_exception ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. The only downside is it's a little complicated to make the example set know which type of error NewRelic::Agent#receive should be called with so I lean towards just expecting the method to be called and not specifying the error type it should receive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually there are a couple of error types which we aren't currently reporting to NewRelic, such as if USPS returns something to us other than a hash when we expect a hash. I don't know if it's useful to log those to NewRelic...I guess we could construct a StandardError with some error string content

I'm now leaning towards leaving it out of the example set, how it is right now

@sheldon-b sheldon-b merged commit ac033cb into main Nov 1, 2022
@sheldon-b sheldon-b deleted the sbachstein/lg-7706-add-newrelic-logging-for-bad-request-errors branch November 1, 2022 18:14
@aduth aduth mentioned this pull request Nov 3, 2022
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.

2 participants