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

Use Erlang's logger as main logging implementation #9333

Merged
merged 78 commits into from
Nov 1, 2019

Conversation

hauleth
Copy link
Contributor

@hauleth hauleth commented Sep 8, 2019

This implements Elixir's Logger module on top of Erlang's logger while providing compatibility layer.

  • use logger:log/2-3 to dispatch log messages
  • translate Erlang messages to Elixir's logger compatible ones
  • use Erlang functions instead of custom Elixir ones for utilities
  • use Erlang formatter for formatting messages instead of handler
  • use Erlang filters instead of handler

lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
@hauleth
Copy link
Contributor Author

hauleth commented Sep 9, 2019

@josevalim also I believe that the handle_sasl_reports option should be deprecated and always set to true, because according to docs:

The new default behaviour is that the SASL application no longer affects which log events that are logged. Supervisor reports and crash reports are logged via the default logger handler which is setup by Kernel. Progress reports are by default not logged, but can be enabled by setting the primary log level to info, for example by using the Kernel configuration parameter logger_level.

@josevalim
Copy link
Member

Sounds good, but not in this PR please :) we can do it before or after!

@hauleth
Copy link
Contributor Author

hauleth commented Sep 9, 2019

I am leaving comments in all places that need deprecations and/or changes in future.

Only "new features" are:

  • Support for :warning log level
  • Logger.warning macro that is currently synonim of Logger.warn.

@hauleth
Copy link
Contributor Author

hauleth commented Sep 9, 2019

Questions needed resolving:

  1. Should handle_sasl_reports be enabled globally for all logger handlers or rather it should be applied only to the :logger legacy handler? Currently it is later, so other handlers will receive everything.
  2. Should Logger.disable/Logger.enable be local to the legacy handler or rather global? Currently it is later, so if someone opt-out of logging, then all logs will be disabled.
  3. Should we name our handler :default or rather Logger? Currently it is later, as I think it should be considered temporary with target to dump legacy handler (and probably whole application) in Elixir 2.0 (if it gonna happen).
  4. Should we implement support for :logger.olp_config/0 format of options for handling overload with compatibility layer or rather we should keep old API as is?

@josevalim
Copy link
Member

Should handle_sasl_reports be enabled globally for all logger handlers or rather it should be applied only to the :logger legacy handler? Currently it is later, so other handlers will receive everything.

Agreed.

Should Logger.disable/Logger.enable be local to the legacy handler or rather global? Currently it is later, so if someone opt-out of logging, then all logs will be disabled.

Agreed.

Should we name our handler :default or rather Logger? Currently it is later, as I think it should be considered temporary with target to dump legacy handler (and probably whole application) in Elixir 2.0 (if it gonna happen).

Agreed. I am not sure if it will ever be removed, it would be a major backwards incompatible change and we don't want to use major versions as an excuse to break user code. Plus, Elixir's handler is still a convenient way of having safe built-in handler with overload protection. AFAIK, Erlang doesn't expose its mechanisms for overload protection.

Should we implement support for :logger.olp_config/0 format of options for handling overload with compatibility layer or rather we should keep old API as is?

No because of the above. AFAIK Erlang doesn't expose the building blocks for building olp config, so I don't want to be tied to an API we have no control and that we can't rely on.

@hauleth
Copy link
Contributor Author

hauleth commented Sep 10, 2019

I have removed all usage of persistent_term in Logger.Config as it is now stored as configuration options in Logger.LegacyHandler (which is then handled by Erlang's logger with hope that it uses the best store available). This greatly reduces complexity of Logger.Config.

Improvements to be made:

  • Register Logger.Config handler within adding_handler/1 callback and deregister it in removing_handler/1 callback.
  • If that happens review the need for Logger.Watcher server
  • How to handle failures within Logger.LegacyHandler? If there happen any error within handler it will be deregistered, we do not want that, as this will result with no logger at all and can cause missing logs entries. Currently it is done by handling all errors within handler and IO.inspecting them when they happen (very not ideal solution).

lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
lib/logger/lib/logger.ex Outdated Show resolved Hide resolved
@hauleth hauleth force-pushed the feat/logger branch 2 times, most recently from db3036b to 8b1ef08 Compare September 10, 2019 15:20
@hauleth hauleth force-pushed the feat/logger branch 2 times, most recently from 5f5253c to 173a250 Compare September 11, 2019 15:59
@hauleth hauleth changed the title WIP: Use Erlang's logger as main logging implementation Use Erlang's logger as main logging implementation Sep 25, 2019
@josevalim
Copy link
Member

Thank you so much @hauleth for the amazing work! ❤️ 💚 💙 💛 💜

@josevalim josevalim merged commit 67dec46 into elixir-lang:master Nov 1, 2019
@josevalim josevalim deleted the feat/logger branch November 1, 2019 14:53
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

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