-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Replace error_logger handler with logger handler #84
base: main
Are you sure you want to change the base?
Replace error_logger handler with logger handler #84
Conversation
I just ran the tests:
I'll need to look at the code in detail later, but I wanted to let you know that it passes the tests on my machine. |
Just got a bit of feedback on the erlang slack: Seems like started processes are indeed not really "supervised" in the new logger api. We could start the handler under the Also I'm wondering how concerned this code should be about overload. I kinda feel like applications starting/crashing at such rates is in itself already a bit of a problem. |
Hi @LostKobrakai, I ran into the problem with catch OTP applications crashing today and revisited this PR. I also ran into the issue that the Elixir Logger globally turns off application start reports if you don't have it configured with |
Tbh I don't think the restart feature is the most important – even though we do use it. Much more important is getting to know about issues and also not bricking the device / have it stay in a state where it at best can be remote debugged. |
Totally agree. Thanks for letting me know what parts are important to you. |
Looking at this a bit more I don't actually think this matters. The |
Did you actually try it? This didn't work when I tried it without |
Nope, I just looked at what elixir actually does with the setting and it certainly doesn't look like it would affect additional logger handlers. Though might also be different on earlier versions of elixir. |
Got it. I tried Elixir 1.16.0, I believe, so not too old. |
I believe it was one of these lines, since they specify https://github.com/elixir-lang/elixir/blob/main/lib/logger/lib/logger/utils.ex#L8-L9 |
Yeah. Maybe I'm misunderstanding what this translator does. Maybe it does filter logs for all handlers, not just the primary one. |
Having had a look at it today: You're correct. Primary filters apply to all handlers not just the primary handler.
https://www.erlang.org/doc/apps/kernel/logger_chapter.html#filters |
This is a first naive port based on the docs here: https://www.erlang.org/doc/apps/kernel/logger_chapter.html#example--implement-a-handler
I've opted for doing the pattern matching in the process generating the logs, so there's less message passing involved. I expect that to be fast enough to not slow down the
application_controller
.I've not yet run tests with it, as I currently get this error on the
ShoehornTest
case:Closes #79