elixir icon indicating copy to clipboard operation
elixir copied to clipboard

Make logger_translator not be a primary filter

Open LostKobrakai opened this issue 1 year ago • 6 comments

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.

LostKobrakai avatar Jul 26 '24 16:07 LostKobrakai

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 :(

josevalim avatar Jul 27 '24 19:07 josevalim

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.

josevalim avatar Jul 27 '24 19:07 josevalim

I think this can be problematic because it means if I add a new handler, now everything will be logged in Erlang terms?

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.

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 :(

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.

LostKobrakai avatar Jul 27 '24 19:07 LostKobrakai

The reasons are mostly related to history. The previous :error_logger did not have a unified interface, so the idea of OTP/SASL concerns were baked right into it and we had to account for that. In particular, while both "OTP/SASL" events were sent to :error_logger, the default implementation in Erlang would would discard the SASL ones and, if the :sasl application was started, it'd register a handler to also print the :sasl ones. Therefore our design was similar but we controlled with a flag, instead of having two handlers.

When the new Erlang :logger came in, we could unify the interfaces, and now Elixir handlers and Erlang handlers are the same. For example, our docs have instructions on how to log to a file, and most Elixir developers expect those logs to be in Elixir terms too, so I don't think we should change default. It is the most convenient and the most performant. But what I can see happening is:

  1. People want to enable sasl for some handlers. In this case, they want to set handle_sasl_reports to true and then disable it with a filter on their default handlers. This is your case and achievable today

  2. People want to disable translations for some handlers. They would need to remove the default translator and add translator at specific places, I don't believe we expose this API yet

josevalim avatar Jul 27 '24 20:07 josevalim

Yeah. I'm kinda seeing these things:

  • Split up the filtering of message from the translator, so they can be applied separately
  • Add APIs, which provide those filters to users to be attached manually to handlers (the sasl filtering might even be to trivial for a need to be provided, but makes sense for the translator)
  • Let people control which/if those filters are added globally (opt out)

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.

LostKobrakai avatar Jul 27 '24 20:07 LostKobrakai

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 :logger_filters.domain/2 to apply it to a specific config. So we only need to add filters related to the translation itself (which seems to be unrelated to the feature you are asking).

josevalim avatar Jul 27 '24 20:07 josevalim

@LostKobrakai any updates here? Should we move forward with the decoupling the translation from filters? Thank you.

josevalim avatar Oct 30 '24 07:10 josevalim

@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.

LostKobrakai avatar Nov 05 '24 12:11 LostKobrakai

Closing this per the above. Please let us know if it should be revisited instead.

josevalim avatar Jan 22 '25 09:01 josevalim