Remove calls to error_logger #9469
Conversation
hauleth
force-pushed
the
chore/remove-error_logger-calls
branch
from
91ea50e to
bcffdb0
Compare
hauleth
force-pushed
the
chore/remove-error_logger-calls
branch
from
bcffdb0 to
b57a3ed
Compare
hauleth
force-pushed
the
chore/remove-error_logger-calls
branch
from
b57a3ed to
2c7e23c
Compare
| error_logger: %{tag: :error_msg}, | ||
| report_cb: &__MODULE__.format_report/2 | ||
| } | ||
| ) |
lib/elixir/lib/task/supervised.ex
Outdated
| %{ | ||
| domain: [:otp, :elixir], | ||
| error_logger: %{tag: :error_msg}, | ||
| report_cb: &__MODULE__.format_report/2 |
There was a problem hiding this comment.
We should keep this one private too and make the name consistent between files (format_log or format_report).
There was a problem hiding this comment.
From what I see, almost all code in OTP uses remote calls to the report_cb, probably the reasoning is the same as in telemetry, as this is clearer for the VM.
There was a problem hiding this comment.
We need to have a reasoning though as doing it just in case is not good enough. Especially because it exports a public API that we would need to @doc false. So I would:
- Use local functions for now
- Verify with the OTP team why they have used remote ones
Can you please check those? Thanks.
There was a problem hiding this comment.
Using an external function means the whole second argument will be a compile-time literal. A local function can't be optimised like that.
Additionally remove TODO, as OTP also mostly ignores 2nd argument.
hauleth
force-pushed
the
chore/remove-error_logger-calls
branch
from
29bf3fb to
6f4f33b
Compare
|
|
||
| {:ok, msg, metadata} | ||
| {:ok, msg, metadata} | ||
| end |
There was a problem hiding this comment.
I have ported it as is, but is there any reason why do that in translator instead of using report_cb directly? I think it would make more sense to have uniform error reports.
There was a problem hiding this comment.
that Is a separate discussion. Unifying reports would be a good option but we would definitely have to push some things uostream before. For example, we include more information on debug level.
There was a problem hiding this comment.
@josevalim you mean in inspect? This is what 2nd argument in report_cb is for, to pass additional informations to formatters.
There was a problem hiding this comment.
I mean we show more information in general. Compare both reports in debug mode for a crashed GenServer and you will see the differences. :)
|
Ok, let’s go with remote calls then, thanks Michał. Btw, any reason why we
can’t convert local ones to literals?
--
*José Valimwww.plataformatec.com.br
<http://www.plataformatec.com.br/>Founder and Director of R&D*
|
|
The main technical issue is that it's not possible to represent "static" local funs in the external term format that is used for encoding the literal chunk in the BEAM files. Local funs that don't capture the environment (are not closures) are convered to literals when loaded, but this still means they can't be embedded within other literals. |
Related to #9464