lager icon indicating copy to clipboard operation
lager copied to clipboard

'line' is a tuple on OTP-24

Open rlipscombe opened this issue 3 years ago • 13 comments

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 avatar Jun 11 '21 13:06 rlipscombe

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

rlipscombe avatar Jun 11 '21 13:06 rlipscombe

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?

jadeallenx avatar Jun 11 '21 14:06 jadeallenx

  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 avatar Jun 11 '21 14:06 rlipscombe

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.

rlipscombe avatar Jun 11 '21 14:06 rlipscombe

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 avatar Jun 11 '21 18:06 paulo-ferraz-oliveira

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

paulo-ferraz-oliveira avatar Jun 11 '21 18:06 paulo-ferraz-oliveira

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.

rlipscombe avatar Jun 11 '21 18:06 rlipscombe

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.

paulo-ferraz-oliveira avatar Jun 11 '21 18:06 paulo-ferraz-oliveira

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 avatar Jun 11 '21 19:06 rlipscombe

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

rlipscombe avatar Jun 11 '21 19:06 rlipscombe

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.

paulo-ferraz-oliveira avatar Jun 14 '21 10:06 paulo-ferraz-oliveira

{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 avatar May 10 '22 06:05 0k-1

{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?

0k-1 avatar May 10 '22 06:05 0k-1

This was fixed in #560

rlipscombe avatar Jan 25 '24 16:01 rlipscombe