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

Make notification for Logger.error configurable #380

Conversation

yukster
Copy link
Contributor

@yukster yukster commented Jul 25, 2021

Follow up to upstream issue #315; adds a configuration key for controlling whether Logger.error sends a Honeybadger notification. Default is false.

@TraceyOnim
Copy link
Contributor

Tested it, this looks good

@yukster
Copy link
Contributor Author

yukster commented Jul 28, 2021

@sorentwo what do you think?

@sorentwo
Copy link
Collaborator

Admittedly, I'm out of the loop a little with all the recent issues around this. However, isn't the purpose of use_logger to attach a logger that sends notifications on error events? I'd expect the same functionality with use_logger: false that we'd get with the addition of this new option.

Chances are that I'm missing something! Is there a reason that use_logger: false is undesirable?

@yukster
Copy link
Contributor Author

yukster commented Jul 28, 2021

Hi @sorentwo! The backstory is that use_logger is how Honeybadger gets notifications for any non-web Elixir/Erlang crashes. At least that's what appeared to happen in my testing. If I set it to false then GenServer or Task exceptions did not get reported (and they did with it set to true). The discussion was in this issue: #276 (comment)

@sorentwo
Copy link
Collaborator

@yukster Ok, I see now. That's what the investigation around SASL vs regular error logging was about. Yes, to get notifications about anything built on the "gen" behaviors we need SASL logging, but we don't necessarily want reports for things reported at the :error level.

Perhaps a different option name is in order? There are other error log levels in recent OTP versions (critical, alert, emergency) and the name doesn't make it obvious which levels are reported.

What if use_logger is expanded to have an option like :sasl_only, or accept a level rather than a boolean? Or, to keep the options totally separate you can flip this option into something like sasl_logging_only. WDYT?

@yukster
Copy link
Contributor Author

yukster commented Jul 29, 2021

@sorentwo hm yeah, good point about varying log levels. Just renaming the config seemed more straightforward to me so I went ahead and did that. What do you think?

Copy link
Collaborator

@sorentwo sorentwo left a comment

Choose a reason for hiding this comment

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

Looks great with the clearer name!

@sorentwo sorentwo merged commit 49d889f into honeybadger-io:master Jul 30, 2021
@yukster
Copy link
Contributor Author

yukster commented Jul 30, 2021

Awesome, thanks @sorentwo, @TraceyOnim, @joshuap, and @effektzwm!

@yukster yukster deleted the make_logger_error_notifications_configurable branch July 30, 2021 16:05
@yukster yukster restored the make_logger_error_notifications_configurable branch July 30, 2021 16:06
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