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

Enable Sentry error monitoring #71

Merged
merged 7 commits into from
Oct 21, 2024
Merged

Enable Sentry error monitoring #71

merged 7 commits into from
Oct 21, 2024

Conversation

heykarimoff
Copy link
Contributor

@heykarimoff heykarimoff commented Oct 2, 2024

Enable sending errors/exceptions to Sentry monitoring system. This PR completes a part of #17 which does the following:

@heykarimoff heykarimoff self-assigned this Oct 2, 2024
@heykarimoff heykarimoff requested review from zahraaalizadeh, a-musing-moose and jadedarko and removed request for zahraaalizadeh October 2, 2024 04:44
@heykarimoff heykarimoff force-pushed the sentry-error-monitoring branch from c952aca to e0b4dcf Compare October 2, 2024 04:51
Copy link
Contributor

@a-musing-moose a-musing-moose left a comment

Choose a reason for hiding this comment

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

Thanks for this - I don't think that we should include a view for triggering errors that anyone can visit at any time.

@heykarimoff heykarimoff force-pushed the sentry-error-monitoring branch from e0b4dcf to cdadba9 Compare October 2, 2024 05:02
G-Rath
G-Rath previously requested changes Oct 2, 2024
Copy link

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

I've added details about what our traditional settings are for Sentry across our apps - otherwise this looks good.

I would also suggest possibly adding this as a commented out line after the config:

sentry_sdk.integrations.logging.ignore_logger("django.security.DisallowedHost")

because

  1. sometimes our apps have reasons for requests that trigger this error in which case we don't want to be told about it
    • i.e. one client runs tennable.io from behind their WAF, and another client manages the infrastructure and has decided to make the app publicly accessible - in both cases, this error is the correct and desired response, so it just creates noise having it reported to Sentry
  2. it strongly hints at how you can handle these kind of errors in situations where they're overly noisy, as it doesn't seem to be signposted well enough
    • for some reason the most common solution I see people PRing is creating a custom before_send filter, which as things go isn't the worst thing, but it's still more complex than this 😅

@heykarimoff heykarimoff force-pushed the sentry-error-monitoring branch from cdadba9 to e2431fb Compare October 11, 2024 07:01
@heykarimoff heykarimoff requested a review from G-Rath October 11, 2024 07:09
@heykarimoff heykarimoff force-pushed the sentry-error-monitoring branch from 0505b44 to 2737ba0 Compare October 15, 2024 01:34
@heykarimoff heykarimoff dismissed G-Rath’s stale review October 21, 2024 02:46

The issue has been resolved.

@heykarimoff heykarimoff merged commit 2021286 into main Oct 21, 2024
3 checks passed
@heykarimoff heykarimoff deleted the sentry-error-monitoring branch October 21, 2024 02:46
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