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

509 evss backend sentry error to info #3297

Merged
merged 5 commits into from
Sep 16, 2019

Conversation

LindseySaari
Copy link
Contributor

@LindseySaari LindseySaari commented Sep 13, 2019

Description of change

This change reduces Sentry noise by logging the EVSSBackendServiceError with a level of info instead of error, no longer creating a sentry error alert. After discussion, triggering a sentry alert is not necessary.

Testing done

Spec tests run & pass
Sentry logging remains the same for other error logging

Acceptance Criteria (Definition of Done)

  • Spec tests passing
  • Sentry alerts are no longer recording EVSSBackendServiceError with a level of error
  • The EVSSBackendServiceError is an info-level event

Applies to all PRs

  • Appropriate logging
  • Swagger docs have been updated, if applicable
  • Provide link to originating GitHub issue, or connected to it via ZenHub
  • Does not contain any sensitive information (i.e. PII/credentials/internal URLs/etc., in logging, hardcoded, or in specs)
  • Provide which alerts would indicate a problem with this functionality (if applicable)

@jholton
Copy link
Contributor

jholton commented Sep 13, 2019

I missed the first PR that this one builds off of. I would have said this then:

I think should move logic that changes an error's 'level' into a Sentry processor. We already have one that sets certain exceptions to :warn. Now we have multiple places where 'levels' are set.

We should keep this type of logic in one place (processors, IMO). Otherwise we are asking for a situation in the future where sentry_logging sets a level and then a processor sets it to something else.

Copy link
Contributor

@kfrz kfrz left a comment

Choose a reason for hiding this comment

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

Think this might live better in EVSS middleware.

lib/sentry_logging.rb Show resolved Hide resolved
@kfrz
Copy link
Contributor

kfrz commented Sep 16, 2019

@jholton just FYI - we are going to merge this change for now to supress the particular EVSS 503 response, but we will be working to refactor our overall error handling in order to leverage a sentry processor/middleware pattern.

@kfrz kfrz self-requested a review September 16, 2019 18:47
Copy link
Contributor

@kfrz kfrz left a comment

Choose a reason for hiding this comment

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

Let's merge this for now!

@va-vfs-bot va-vfs-bot temporarily deployed to 509_evss_backend_sentry_error_to_info/master September 16, 2019 19:09 Inactive
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.

4 participants