Preserve logger reports through translation by encapculating translated output in report_cb

[martosaur]

The thing that is bothering me and that I'm looking for a way to address is the fact that translators are destructive to the report log messages. Most reports don't survive default translator and are replaced with a chardata, but some logger handlers clearly have a demand for the original report data, since they go as far as parsing chardata to build their own report.

One idea I have is that instead of converting report to chardata, translator could wrap the report_cb callback function, since the existing one would be useless after the translation anyway.

Some caveats with this approach:

1. This can be seen as a breaking change by handlers/formatters. I think technically it's not since translation is an optional filter and the log pipeline in general can be tweaked at any point by the app, but still not very nice.
2. Not sure if using anonymous functions as report_cb can have any performance implications or run into problems with multi-node setup?
3. report_cb use is a little different from what translator does right now, so there's probably more changes to make here to make it return format string (?).

In general I would love to get some feedback on if this is the right way to approach this or if there is even a good way 🙏

[josevalim]

I am happy with this approach, if tests pass. :)

Not sure if using anonymous functions as report_cb can have any performance implications or run into problems with multi-node setup?

Yes, it is a problem. It is probably best to store the translated message in the map or metadata and have a static report_cb that reads that instead. In fact, you may flip the whole thing on its head, translate the message only if the report_cb is called, so you move the translation to inside report_cb. So what you to do is something like this:

report_cb: &__MODULE__.new_report_cb, translator_cb: {translators, old_cb}

[martosaur]

Thanks! I like the idea of storing the translated message in metadata and use a static report_cb.

In fact, you may flip the whole thing on its head, translate the message only if the report_cb is called, so you move the translation to inside report_cb. So what you to do is something like this:

I was actually considering something like this at first, but realized midway that one incidental purpose of the translator is to enrich log events with Elixir-specific metadata, such as crash_reason or callers, and that's something that is done as part of the translation. That seems important to me, but maybe there is a way around this?

[josevalim]

You are right, we cannot do it then. Let's stick to your approach then!

[martosaur]

Oh I just realized storing translated message in the metadata isn't the option, as report_cb is meant to be called exclusively on the report itself. But the report is a map/keyword, so it should be possible to store it right there, like you suggested. Unless there is a reason to not do this, I'll try to take this to the finish line then!

[josevalim]

Go for it!

[martosaur]

I made another approach to this and it seems like the problem of presenting translator output (chardata) as report_cb output (format + args) can be addressed by wrapping it in {~c"~ts", [chardata]}. All tests pass, so unless this isn't a good idea, the PR might actually be ready for review and will only be missing docs (changelog entry?)!

[josevalim]

💚 💙 💜 💛 ❤️