erlang / otp

Erlang/OTP
http://erlang.org
Apache License 2.0
11.33k stars 2.94k forks source link

Allow stateful logger handlers to register themselves in `logger_sup` #5560

Open hauleth opened 2 years ago

hauleth commented 2 years ago

Is your feature request related to a problem? Please describe.

Any logger handler that need to do something more in the handler and want to be overload protected will need to spawn the process. Currently there are 2 possible solutions to that:

Additionally both of these approaches do not work with logger_handler_watcher, so may cause additional error messages during shutdown about failing handlers.

Describe the solution you'd like

I am still working on the full design, and I would like to discuss it here a little. My current idea is to replace logger_sup with custom supervisor implementation (or add handler supervisor as an additional process there) that would allow adding just PID of the process to the supervision tree. Then the HModule:adding_handler/1 callback can be extended with possibility to return 3-ary tuple in form of:

{ok, logger:handler_config(), pid()}

And the returned PID will be registered in supervisor and will be registered in logger_handler_watcher for proper deletion of handlers that are shutting down.

This would allow the handlers to register their processes in logger_sup without exposing that module and that would allow custom handlers to be registered early in the startup process of the VM.

Alternatively we could expose a function to start process that would be supervised by logger_sup, but I think that such design would be more error prone and confusing for the handlers authors.

garazdawi commented 2 years ago

Hello!

Yes, this is a problem. My initial idea was that you should disable the default logger in kernel and then later add it when the application that has the logging backend is started. Something like this:

{kernel, logger, [{handler, default, undefined}]}.
{syslogger,
  {log_opts, [cons, pid, perror]},
  {logger, [{handler, default, syslogger,
               #{ formatter => {logger_formatter, #{single_line => true}}}}]}
}.

This is what I recommend with my syslogger. When you set the default kernel logger to undefined, it continues to use the simple_handler a default handler is added and then all log events are replayed against the newly added default handler.

This approach kind of works if you make sure that the logger backend is the first application to be started after stdlib+kernel, but you get into problems if kernel crashes for any reason as then the logging happens with the simple_haandler instead which is not very good at logging things.

I'm unsure if the best solution to this is to stick processes belonging to later applications into the kernel supervisor as that means that the code of those applications have to be loaded (and any applications they depend on) and you could create all kinds of strange deadlock issue if you are not careful.

max-au commented 2 years ago

I would prefer the logger process to be in the application supervision tree, rather than kernel-supervised. This will make it easier to attribute (including observer view). It also ensures there are no rogue/runaway processes left if the application (being temporary) crashes. It feels connected to https://github.com/erlang/otp/issues/5428 - same problem of loggers started by the application but not stopped. The (non-logger) solution we use is, the "logger" process monitors application-supervised extra loggers, and cleans up as necessary.

seriyps commented 2 years ago

Yeah, got exactly the same problem in my version of journalctl logger handler - it can't be hooked via kernel section of sys.config, only, as Lukas suggested, via logger key of my own app or manually. This is a limitation which is hard to explain to the library users.