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

Why does the logger not report :error messages? #30

Closed
joakimk opened this issue Nov 26, 2015 · 8 comments
Closed

Why does the logger not report :error messages? #30

joakimk opened this issue Nov 26, 2015 · 8 comments

Comments

@joakimk
Copy link

joakimk commented Nov 26, 2015

Hello!

I've been trying to move over to this client from my own honeybadger client that pre-dates this one.

It seems to work good except this client does not handle :error-level log messages. Is this by design somehow or does it make sense to add it?

https://github.com/honeybadger-io/honeybadger-elixir/blob/master/lib/honeybadger/logger.ex seems to only handle :error_report.

I've so far relied on this in my job queue library for error reporting but that means those errors don't turn up in honeybadger when using this client. The errors are reported here:
https://github.com/joakimk/toniq/blob/296b99346b86bad8a1380ccbab1b79f61bd412ec/lib/toniq/job_runner.ex#L47

joakimk added a commit to barsoom/content_translator that referenced this issue Nov 26, 2015
@rbishop
Copy link
Contributor

rbishop commented Nov 26, 2015

Hey Joakim,

This logger is built on top of Erlang's error_logger rather than Elixir's Logger so it handles the Erlang error eventsinstead (see the list at the bottom of the page I linked to above). If you're explicitly calling Logger.error I would suggest calling Honeybadger.notify instead.

Honeybadger.notify(exception, context, stacktrace)

@joakimk
Copy link
Author

joakimk commented Nov 26, 2015

That might be fine for an application, but toniq is a general purpose background job library. Don't want that to know about any specific error handling service.

@joakimk
Copy link
Author

joakimk commented Nov 26, 2015

I'll try experimenting with using :error_logger in Toniq, but I suspect this problem will bite more people than just me in the future. It seems reasonable to expect that logging an error using the elixir logger would be reported to honeybadger by the elixir honeybadger client.

@rbishop
Copy link
Contributor

rbishop commented Nov 26, 2015

The problem with Elixir's Logger is its default translator turns everything into a String. This causes you to lose all sorts of great information such as the process dictionary. Background processing is actually the very reason I chose to use Erlang's error_logger rather than Elixir's Logger.

With error_logger any process that is SASL compliant (GenServer, GenEvent, Agent, Task, anything spawned with proc_lib) and crashes automatically notifies error_logger with an error_report event. From looking at your background worker library it seems like you are not "letting it crash." I'd recommend you do whatever cleanup you want to do and then re-raise the original failure so that error_logger gets notified and the user can specify their own failure behavior.

@joakimk
Copy link
Author

joakimk commented Nov 30, 2015

I'll look into using another way to report errors in toniq.

In general I think I'd like the honeybadger client to err on the side of caution and report all error types possible, even if that means missing out of some contextual information. If my app is not working as expected, I want to know about it! :)

@rbishop
Copy link
Contributor

rbishop commented Nov 30, 2015

The way you know about your app failing is by letting processes crash and letting the BEAM do what it does best.

On Nov 30, 2015, at 1:59 AM, Joakim Kolsjö [email protected] wrote:

I'll look into using another way to report errors in toniq.

In general I think I'd like the honeybadger client to err on the side of caution and report all error types possible, even if that means missing out of some contextual information. If my app is not working as expected, I want to know about it! :)


Reply to this email directly or view it on GitHub.

@joakimk
Copy link
Author

joakimk commented Nov 30, 2015

I agree with you, I should let it crash. I'm just afraid not every piece of code ever written in Elixir will follow that pattern, and just ignoring other errors on principle seems like it could cause problems.

Thanks for a great library and good feedback btw!

@joakimk joakimk closed this as completed Nov 30, 2015
@rbishop
Copy link
Contributor

rbishop commented Nov 30, 2015

Thank you for the discussion! I'll think about this but I think the Elixir Logger will have to change before we can accommodate it. If it were easy to have both error_logger and Logger and not duplicate messages I would do that but at this point error_logger is more robust.

On Nov 30, 2015, at 6:40 AM, Joakim Kolsjö [email protected] wrote:

I agree with you, I should let it crash. I'm just afraid not every piece of code ever written in Elixir will follow that pattern, and just ignoring other errors on principle seems like it could cause problems.

Thanks for a great library and good feedback btw!


Reply to this email directly or view it on GitHub.

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