elixir icon indicating copy to clipboard operation
elixir copied to clipboard

Improve Logger's gen_event message

Open josevalim opened this issue 6 years ago • 7 comments

Breaking changes:

  • [ ] The timestamp should be in native units instead of a data structure
  • [ ] The metadata should be a map instead of a keyword list and we should no longer add module and function keys, furthermore file should be a charlist

These changes may also require changes to the formatter, as it receives both the old time structure and old metadata. In particular, we may want to pass the metadata keys as arguments to the formatter, instead of filtering them in the backend.

We can likely implement this today by having a flag or a function_exported? on each backend. If the new backend is used, it receives and uses the message as is. If the old backend is used, we will wrap it on another GenEvent handler and convert the old message to the new one on the fly.

We should probably start working on this around v1.12, once the Erlang's logger integration solidifies.

josevalim avatar Jul 18 '18 11:07 josevalim

Now, as #9333 is merged, we already have integration done. About backends then as soon as logger_olp will became public then we can make all current backends "old backends" and promote using :logger handlers as the "new backends". This will solve all tasks mentioned there.

hauleth avatar Nov 03 '19 14:11 hauleth

Good call. We likely want to postpone this one a bit because the answer may be: "write an Erlang backend now".

josevalim avatar Nov 03 '19 15:11 josevalim

From what I see, if we manage to provide formatter in Elixir 1.10 then it can become hard warning in lucky 1.13 or less lucky 1.14 release.

hauleth avatar Nov 04 '19 14:11 hauleth

I'm maintaining own formatter implementing Logger.Formatter... and here are my thoughts (with exciting Elixir 1.11 changes).

  • timestamp - should use :microsecond, not :native to match with :logger.timestamp/0
    • Also.. erlang defines time in metadata - so probably we should use that instead of passing it as separate argument?
  • level - should use :logger.level not Elixir's logger level (we may drop erl_level when we remove "old" backend)
  • module/function - I guess we want to use mfa key as :logger.metadata defines?

chulkilee avatar Oct 11 '20 09:10 chulkilee

@chulkilee all of that changes will be there as soon as Elixir will document how to use Erlang handlers and for matters instead of Elixir's backends. For now it is not done because:

  • Erlang handlers aren't overload protected, so writing them in safe manner is a little bit more convoluted. I am working on creating library enough to deal with that before logger_olp became publicly available.
  • Handler configuration is a little bit more complex than with Elixir, as Elixir do not respect kernel options in development.
  • Until OTP 23 default Erlang formatted didn't allowed using binaries in template, which was "unidiomatic" from the Elixir viewpoint.

As soon as Elixir will be OTP 23 only I think that we will be able to translate console backend to be backed by logger_std_h and encourage people more to use Erlang handlers. So I think, that @josevalim should change the description of the issue to state that we should shift to Erlang handlers in future.

hauleth avatar Oct 11 '20 10:10 hauleth

@josevalim looks like it's time to revisit this issue. Or it's waiting v1.15 and dropping Erlang 23 ?

zlumyo avatar Aug 02 '22 18:08 zlumyo

The Erlang Logger integration was pushed to v1.15, but it will be tackled in #9465. :)

josevalim avatar Aug 02 '22 18:08 josevalim

Latest suggestion: we should probably make the message be {:event, map}, where map is simply the event from Erlang's logger.

josevalim avatar Feb 14 '23 08:02 josevalim

Actually, I don't think this is necessary anymore. We introduced a formatter for Erlang handler and the internals end-up very similar to the current message.

josevalim avatar Feb 16 '23 10:02 josevalim