add line number support by jimmyolo · Pull Request #879 · log4js-node/log4js-node

jimmyolo · 2019-05-15T18:16:59Z

logger add enableCallStack(boolean)
more layout support
- %f fileName
- %l lineNumber
- %o columnNumber
- %s call stack

codecov · 2019-05-15T18:22:41Z

Codecov Report

Merging #879 into master will decrease coverage by 0.05%.
The diff coverage is 96%.

@@            Coverage Diff             @@
##           master     #879      +/-   ##
==========================================
- Coverage   98.03%   97.98%   -0.06%     
==========================================
  Files          25       25              
  Lines         968      991      +23     
==========================================
+ Hits          949      971      +22     
- Misses         19       20       +1

Impacted Files	Coverage Δ
lib/log4js.js	`98.03% <ø> (ø)`	⬆️
lib/categories.js	`100% <100%> (ø)`	⬆️
lib/layouts.js	`96% <100%> (+0.16%)`	⬆️
lib/LoggingEvent.js	`100% <100%> (ø)`	⬆️
lib/logger.js	`97.67% <88.88%> (-2.33%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 853c9c9...ea6cbf0. Read the comment docs.

nomiddlename

This is great - thanks for working on it. Apart from the spelling mistake in the function name, the only other thing I could think of that might need doing is to add a global config parameter to turn stack traces on or off for all loggers. What do you think?

What I imagine is that a user will want all their logs to have the line numbers, and so would expect to do:

log4js.configure({
  appenders: ...,
  categories: ...,
  enableCallStack: true
});

or maybe if we wanted something a bit more granular:

log4js.configure({
  appenders: ...,
  categories: {
    default: { appenders: [...], level: 'debug', enableCallStack: true }
  }
});

What do you think? I'm happy to accept this PR as it is now, it will be useful to a lot of users - but if you've got the time/energy to do a little bit more I think we could make it even more useful.

nomiddlename · 2019-05-15T22:21:50Z

    this.context = {};
  }
+
+  enabelCallStack(bool = true) {


Spelling mistake here - should be enableCallStack

jimmyolo · 2019-05-16T09:06:02Z

I prefer second XD
ps: I try to auto detect appender's layout.patter /%[f|l|o|s]/.test() === true then enableCallStack, but failed

jimmyolo · 2019-05-16T11:17:44Z

@nomiddlename I saw in /lib/log4js:126
even the category is same, still return new Logger(), will not cause any performance issue ?
what happen if I use the same logger when category is same

jimmyolo · 2019-05-16T14:25:08Z

@nomiddlename
last commit

add a loggers Map to store the same category's logger => try to reduce LoggerEvent count
log4js add useCallStack(boolean) for enable/disable all loggers' callstack

nomiddlename

Thanks for making the changes. I agree with you about the category level config being better - let's not bother with the global one. I made some comments about making logger.useCallStack a property following the same pattern as the logger.level property.

nomiddlename · 2019-05-16T22:49:08Z


 let enabled = false;
+let gCallStackEnabled = false;
+const loggers = new Map();


I don't think you need to cache loggers. They were designed to be lightweight. It's not a big deal to create lots of them.

nomiddlename · 2019-05-16T23:02:00Z

    this.context = {};
  }
+
+  enableCallStack(bool = true) {


enableCallStack with an argument seems confusing to me. This might be better as a property of the Logger class, with get and set functions which validate the boolean value (similar to how the level property is defined on Logger). This would also avoid having to cache Loggers by setting the useCallStack value at the category level (like how the log level is set at the category level).

But, that way will influence after new Logger(), not respect to the configuration

Take a look at how the level property is defined - it always looks up the level in the category configuration, so it applies to all loggers.

yup, applies to all same category loggers.

nomiddlename · 2019-05-16T23:08:57Z

  levels,
-  addLayout: layouts.addLayout
+  addLayout: layouts.addLayout,
+  useCallStack,


I don't think you need the global useCallStack - keeping it at the category level seems cleaner and simpler.

nomiddlename

This is great. Thanks for making the changes.

nomiddlename · 2019-05-22T22:36:21Z

Published to NPM in 4.3.0

lileilei · 2019-05-29T06:01:00Z

Missing fields need to be supplemented

s97712 · 2019-06-17T03:06:50Z

Would it be better to implement it with babel plugin?

rfox12 · 2020-05-24T21:56:02Z

It looks like these fields didn't make it into the type description for LoggingEvent (see log4js.d.ts)

jimmyolo added 5 commits May 16, 2019 00:45

feat: add line number, column position and filename support

5da824e

test: cover layout-test

daeb754

test: add callsite for devDeps & cover logger-test

ea0028b

test: cover LoggingEvent-test

c7bac4c

style: fix lint

cfb458c

nomiddlename reviewed May 15, 2019

View reviewed changes

jimmyolo added 2 commits May 16, 2019 10:44

chore: fix typo

911693f

feat: add category enableCallStack opt. (default: false)

9d1e325

jimmyolo added 2 commits May 16, 2019 20:04

chore: check category.enableCallStack when config

b5e7db2

feat: add log4js.useCallStack() to enable/disable all loggers' callStack

5342a8d

test: add & fix test

a564ce4

nomiddlename requested changes May 16, 2019

View reviewed changes

nomiddlename added this to the 4.3.0 milestone May 16, 2019

jimmyolo added 2 commits May 17, 2019 15:10

revert: remove global useCallStack

c49ea03

test: add output '' when callstack info not exist

ea6cbf0

nomiddlename approved these changes May 20, 2019

View reviewed changes

nomiddlename merged commit 475499b into log4js-node:master May 20, 2019

nomiddlename mentioned this pull request May 21, 2019

docs: line number feature documentation #884

Merged

lheberlie mentioned this pull request Oct 19, 2019

Pattern format for call stack using padding/truncation not working #955

Closed

Conversation

jimmyolo commented May 15, 2019

Uh oh!

codecov Bot commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nomiddlename left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmyolo commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmyolo commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmyolo commented May 16, 2019

Uh oh!

nomiddlename left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nomiddlename left a comment

Choose a reason for hiding this comment

Uh oh!

nomiddlename commented May 22, 2019

Uh oh!

lileilei commented May 29, 2019

Uh oh!

s97712 commented Jun 17, 2019

Uh oh!

rfox12 commented May 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented May 15, 2019 •

edited

Loading

jimmyolo commented May 16, 2019 •

edited

Loading

jimmyolo commented May 16, 2019 •

edited

Loading