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

Make logger_translator not be a primary filter #13741

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LostKobrakai
Copy link
Contributor

@LostKobrakai LostKobrakai commented Jul 26, 2024

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.

@josevalim
Copy link
Member

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
Copy link
Member

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.

@LostKobrakai
Copy link
Contributor Author

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.

@josevalim
Copy link
Member

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

@LostKobrakai
Copy link
Contributor Author

LostKobrakai commented Jul 27, 2024

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.

@josevalim
Copy link
Member

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
Copy link
Member

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

@LostKobrakai
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

2 participants