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

enhance ignore_if to allow "by-notifier" customization #418

Open
fursich opened this issue May 25, 2018 · 1 comment
Open

enhance ignore_if to allow "by-notifier" customization #418

fursich opened this issue May 25, 2018 · 1 comment

Comments

@fursich
Copy link

fursich commented May 25, 2018

Hello, thanks a million maintaining this great gem!

Here's something I hope this gem to have as a new feature - I'm very happy to PR if that makes sense.

OPTION PROPOSAL (no breaking changes)

Just wanted to propose a new API that enables us to selectively ignore exceptions on notifier basis - something like:

Rails.application.config.middleware.use ExceptionNotification::Rack,
  :ignore_notifier_if => ->(env, exception, notifier) {
    exception.message =~ /^A Not-So-Important-Yet-Needs-To-Be-Logged Exception/ &&  \
     notifier == :slack_notifier
  },
  :email => {
    :email_prefix         => "[PREFIX] ",
    :sender_address       => %{"notifier" <[email protected]>},
    :exception_recipients => %w{[email protected]},
  },
  :slack => {
    :webhook_url => "[Your webhook url]",
    :channel => "#major_exceptions",
  }

  # Keep ignore_if option as it is now, so that it continues to be available for
  # lamda's with only two arguments

Or more simply, by using config

ExceptionNotification.configure do |config|
  config.ignore_if do |exception, options, notifier|
    exception.message =~ /^A Not-So-Important-Yet-Needs-To-Be-Logged Error/ &&  \
    notifier == :slack_notifier
  end

  # this also accepts two arguments as it used to do (notifier optional)
  config.ignore_if do |exception, options|
    exception.message =~ /^Another Error That Needs To Be Totally Ignored/
  end
end

Again, there is no breaking changes (back compatible) - current API is available as it is. Just wanted to make a new argument available.

Do you have any thoughts/advises on this..?
I'll be submitting a PR for this to show the details above with some decent tests attached - hope that makes sense!

USE CASE

We need this as we have multiple notifiers to serve for different purposes - we need our slack to alert only major incidents, but we still want to collect all the error logs just in case using other notifiers.

@LeEnno
Copy link

LeEnno commented Aug 14, 2018

I'd really like to see some way to do this. My first idea was to add ignore_if on a per-notifier basis. Global ignore_if for all notifiers + local ignore_if just for the current notifier:

Rails.application.config.middleware.use ExceptionNotification::Rack,
  :ignore_if => ->(env, exception) { exception.message =~ /^A Just-Ignore-Me-Everywhere Exception/ },
  :email => {
    :email_prefix         => "[PREFIX] ",
    :sender_address       => %{"notifier" <[email protected]>},
    :exception_recipients => %w{[email protected]},
  },
  :slack => {
    :webhook_url => "[Your webhook url]",
    :channel => "#major_exceptions",
    :ignore_if => ->(env, exception) { exception.message =~ /^A Not-So-Important-Yet-Needs-To-Be-Logged Exception/ },
  }

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

No branches or pull requests

2 participants