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

Notify on :error level Logger messages #276

Open
rabidpraxis opened this issue Apr 1, 2020 · 9 comments
Open

Notify on :error level Logger messages #276

rabidpraxis opened this issue Apr 1, 2020 · 9 comments

Comments

@rabidpraxis
Copy link
Contributor

rabidpraxis commented Apr 1, 2020

We had a customer that expected the Logger.error call to notify Honeybadger. It looks like it would be a trivial change to allow notifying on logger error calls.

My question (@joshuap @sorentwo) is there a reason that we only notify on process crashes?

@sorentwo
Copy link
Collaborator

sorentwo commented Apr 2, 2020

There isn't any reason not to do this now. We already use the error logger to do our reporting. Slight refinement though, we notify on both exceptions (rescue) and crashes (catch).

@yukster
Copy link
Contributor

yukster commented Jul 21, 2021

Hey, just commenting here instead of #315 because this one is still open. I question whether this was a sensible choice. I work on several large and long-running Elixir apps and was surprised to see my HB errors go through the roof after updating a bunch of deps, including the HB client.

I'm going to unhook it from the logger by setting use_logger to false (and hoping that will only turn off the Logger.error calls going to HB) but it seems like assuming calling Logger.error is an exceptional case is a bit presumptuous. We log inter-service communication issues at error level and want to track their volume (in Datadog) but we don't need those in HB. I feel like Honeybadger is a triage system for unexpected errors: something new shows up after a deploy, you make a story (we have the Jira plugin), work up a fix, deploy it, and the error is resolved. But anything that goes into Honeybadger due to a Logger call is obviously never going to be resolved!

Anyway, my two cents... And probably a moot argument if the default for use_logger is false... someone please holler if setting use_logger to false will affect anything other than stopping Logger.error calls from notifying HB!

@joshuap
Copy link
Member

joshuap commented Jul 21, 2021

@yukster good points.

I think that use_logger: false will also prevent reporting crashes in SASL compliant processes.

It seems like use_logger may have dual responsibilities—I wonder if we should have two config options instead of one? cc @sorentwo @rabidpraxis @TraceyOnim

@yukster
Copy link
Contributor

yukster commented Jul 21, 2021

@joshuap thanks for the fast response! I have to say, I'm a bit confused by the SASL logging docs, especially because it says that SASL logger was deprecated in OTP 21 (I think we're on 23 across the board). Well, I'm going to test this throughly to make sure crashes still go to HB.

@yukster
Copy link
Contributor

yukster commented Jul 21, 2021

So I confirmed that I will only get notifications from the plug integration with use_logger: false... so no error-reporting for GenServers and the like. I think I'm just going to roll back to 0.15.0 for now. And I would love to see a config for logging errors. Maybe I can whip up a PR for it.

@TraceyOnim
Copy link
Contributor

TraceyOnim commented Jul 23, 2021

@yukster good points.

I think that use_logger: false will also prevent reporting crashes in SASL compliant processes.

It seems like use_logger may have dual responsibilities—I wonder if we should have two config options instead of one? cc @sorentwo @rabidpraxis @TraceyOnim

@joshuap , how do you mean by two config options?
What about using 'ignored_domains option . In such cases error for that domain won't be sent to HB
I have tried ignoring [:elixir] domain in my pet project:

ignored_domains: [:elixir]

with this option error logs from GenServer is sent to HB:

{
  "domain" : ["otp"],
  "error_logger" : {
    "report_cb" : "&:gen_server.format_log/1",
    "tag" : "error"
  },
  "gl" : "#PID<0.66.0>",
  "mfa" : [
    "gen_server",
    "error_info",
    7
  ]

Kindly confirm my assumption if its true, does all SASL crashes fall under [:sasl] or [:otp] domain? If yes then I think ignoring only [:elixir] domain might help in such a case

@yukster
Copy link
Contributor

yukster commented Jul 26, 2021

@joshuap how about this?: #380

@joshuap
Copy link
Member

joshuap commented Jul 26, 2021

Kindly confirm my assumption if its true, does all SASL crashes fall under [:sasl] or [:otp] domain? If yes then I think ignoring only [:elixir] domain might help in such a case

I'm not sure about the answer to that question. Might be a good question for the Elixir forums or Slack channel. If yes, I agree that ignoring the domain could be a good option, although I'm also not against adding an explicit configuration option to disable reporting Logger.error, as in #380.

@TraceyOnim
Copy link
Contributor

Kindly confirm my assumption if its true, does all SASL crashes fall under [:sasl] or [:otp] domain? If yes then I think ignoring only [:elixir] domain might help in such a case

I'm not sure about the answer to that question. Might be a good question for the Elixir forums or Slack channel.

I've posted the question on Elixir Forum, I'll follow up with it for responses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants