erlang-lager / lager

A logging framework for Erlang/OTP
Apache License 2.0
1.12k stars 456 forks source link

'line' is a tuple on OTP-24 #558

Closed rlipscombe closed 9 months ago

rlipscombe commented 3 years ago

The parse transform assumes that the AST node location is the line number, and puts it into metadata as such. Since OTP-24, it's actually {Line, Col}.

See https://github.com/erlang-lager/lager/blob/3.9.2/src/lager_transform.erl#L148-L150

This survives all the way through to the default formatter, so I end up with logging that looks like the following:

2021-06-11 14:43:18.976 app@host [notice] <0.1763.0>@module:function:{168,5} Message

See that {168,5}? I was expecting 168...

Seen with 3.9.2.

Something like the following ought to fix it (only lightly tested):

--- a/src/lager_transform.erl
+++ b/src/lager_transform.erl
@@ -141,13 +141,17 @@ do_transform(Line, SinkName, Severity, Arguments0) ->

 do_transform(Line, SinkName, Severity, Arguments0, Safety) ->
     SeverityAsInt=lager_util:level_to_num(Severity),
+    LineNum = case Line of
+        {L, _C} -> L;
+        L -> L
+    end,
     DefaultAttrs0 = {cons, Line, {tuple, Line, [
                                                 {atom, Line, module}, {atom, Line, get(module)}]},
                      {cons, Line, {tuple, Line, [
                                                  {atom, Line, function}, {atom, Line, get(function)}]},
                       {cons, Line, {tuple, Line, [
                                                   {atom, Line, line},
-                                                  {integer, Line, Line}]},
+                                                  {integer, Line, LineNum}]},
                        {cons, Line, {tuple, Line, [
                                                    {atom, Line, pid},
                                                    {call, Line, {atom, Line, pid_to_list}, [
rlipscombe commented 3 years ago

Should probably also rename Line to Loc, but that would be a very noisy diff.

jadeallenx commented 3 years ago

Is there some reason why you want to throw away the column information? Or is it that it breaks some kind of reporting tool that you're using?

rlipscombe commented 3 years ago
  1. It looks unusual when formatted as a tuple. For consistency with most other tools, it should be Line:Col. For this, the metadata ought to have line and col attributes.
  2. The column information doesn't mean anything, as far as I can tell, because it's been run through a parse transform.
rlipscombe commented 3 years ago

Oh, yes. It also occurs to me that custom backends might be surprised to see a tuple in the 'line' metadata. Consider the cases where they're converting it to a fixed schema that's expecting an integer.

Rollbar, for example (see https://explorer.docs.rollbar.com/#operation/create-item) has separate fields for lineno and colno. So, while it'd be useful to have both in the metadata, having them in the same metadata attribute is likely to break any lager->rollbar backends that currently expect an integer.

paulo-ferraz-oliveira commented 3 years ago

Following what Erlang did with error_location, would an option be suitable? This would keep existing behaviour while allowing for opt-in "line only". I'd prefer it if lager didn't eat up potentially useful info.

paulo-ferraz-oliveira commented 3 years ago

Oh, yeah, on the other hand, as @rlipscombe states, if the column is wrong it's not useful and could/should be discarded.

rlipscombe commented 3 years ago

It's entirely possible that the column is not wrong, and I've just been reading it wrong. If it's useful, I don't want to throw it away.

But: in the interests of not breaking any custom backend, the 'line' metadata should be unchanged. I propose that, on OTP-24, the parse transform, should add a new metadata item 'column' in addition.

paulo-ferraz-oliveira commented 3 years ago

I agree that if it's considered a breaking change (e.g. a callback signature is spec'ed wrong) AND the column info. is OK, your approach of adding metadata is reasonable.

rlipscombe commented 3 years ago

I'm going to be explicit here. I've got an example program: https://github.com/rlipscombe/lager_loc. It uses 'line' in the formatter config for lager_default_formatter.

When compiled with OTP-23, it logs the following:

2021-06-11 20:05:27.411 [info] <0.119.0>@lager_loc_app:start:13 Hello World!

When compiled with OTP-24.0.2, it logs the following:

2021-06-11 20:08:15.284 [info] <0.118.0>@lager_loc_app:start:{13,5} Hello World!

The column information appears to be the start of the lager token in lager:info, so it's potentially useful.

rlipscombe commented 3 years ago

^^ pull request that shows what I'm suggesting.

paulo-ferraz-oliveira commented 3 years ago

FWIW (even as a fast workaround), you can achieve non-column log by adding {error_location, line} to your rebar.config's erl_opts (I tested this on your lager_loc repo.), but I do agree separating the two is more useful.

0k-1 commented 2 years ago

{error_location, line}FWIW(即使作为一种快速的解决方法),您可以通过添加到您rebar.config的 's来实现非列日志erl_opts(我在您的lager_locrepo 上对此进行了测试。),但我同意将两者分开更有用。

If LINE is Tuple type, although it is no problem to generate beam, the use of a function erl_prettypr: format (erl_synntax: form_list (AC)) to code will report an error: {var,{13,13},'String'}, {atom,{13,2},none}, {integer,{13,2},4096}, {integer,{13,2},64}, {var,{13,2},'Leveltest113'}, {var,{13,2},'Tracestest113'}, {atom,{13,2},lager_event}, {var,{13,2},'_Pidtest113'}]}]}, {clause,{13,2},[{var,{13,2},''}],[],[{atom,{13,2},ok}]}]}]}]}, {eof,{42,30}}]** exception error: bad argument in function integer_to_list/1 called as integer_to_list({13,2}) *** argument 1: not an integer in call from erl_syntax:integer_literal/1 (erl_syntax.erl, line 1699) in call from erl_prettypr:lay_2/2 (erl_prettypr.erl, line 460) in call from erl_prettypr:seq/4 (erl_prettypr.erl, line 1437) in call from erl_prettypr:seq/4 (erl_prettypr.erl, line 1440) in call from erl_prettypr:lay_2/2 (erl_prettypr.erl, line 475) in call from erl_prettypr:seq/4 (erl_prettypr.erl, line 1439) in call from erl_prettypr:seq/4 (erl_prettypr.erl, line 1440)

0k-1 commented 2 years ago

{error_location, line}FWIW(即使有一种快速的解决方法),您也可以通过添加到您rebar.config实现的非测试列日志erl_opts(我在您的lager_loc回购上进行的)。 。

Convert {Integer, Line, LINE} to {l, c} = line, {table, line, [{integer, line, l}, {integer, line, c}]} Is it a correct way?

rlipscombe commented 9 months ago

This was fixed in #560