-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 logger_translator not be a primary filter #13741
base: main
Are you sure you want to change the base?
Make logger_translator not be a primary filter #13741
Conversation
Thanks for the PR! I think this can be problematic because it means if I add a new handler, now everything will be logged in Erlang terms? Maybe we should decouple handle_otp_reports/handle_sasl_reports from the translator and instead push that elsewhere? Although it may now mean that we can translate something that won't ever get logged :( |
If we can't find a solution, then maybe the best approach would be to keep it as is, and instead you set handle_sasl_reports to true in your application. |
I was surprised when I saw that elixir forces its own spin on loggers globally. I was expecting APIs to allow people to opt into what elixir adds to their handlers instead of them inheriting them from the global state. Given your next sentence I'm wondering though if the reason here was to prevent translation to happen multiple times if multiple handlers want the translation.
Yeah, I can see the downside here. Even more so as my immediate usecase wouldn't even want to log the message it receives, it's just interested in getting to know the sasl report – for which a logger handler seems to be the only way. Hence why I'm a bit relucant needing people to enable sasl report logging. The library using an logger handler is an implementation detail. |
The reasons are mostly related to history. The previous When the new Erlang
|
Yeah. I'm kinda seeing these things:
The last one could be done by "removing" the translator with existing APIs, but that would only happen once the first user controlled app is started and they keys of the filters need to be documented and stable. |
Exactly. Although we probably don't need filters for the OTP/SASL handling? The boolean flag is equivalent to adding/removing them and I think you can use |
@LostKobrakai any updates here? Should we move forward with the decoupling the translation from filters? Thank you. |
@josevalim Given your above mentioned details I don't think it's technically needed. I however think it would clarify that these are not inherently linked behaviours if they wouldn't be coupled. I'm not sure it would be helpful to my immediate usecase, given it would require configuration to be documented in any case. |
Followup to https://groups.google.com/g/elixir-lang-core/c/bQFdc2RfG28
Not done yet, but here to get some first feedback.
Essentially the goal here is to remove the fact that elixir is adding primary filters, which mean you either e.g. get sasl logs or not at all instead of being able to make that decision on a per handler basis.