-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data field different format #6
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for new feature PR.
But CI is failed. Could you support Node.js 4 to pass CI?
lib/index.js
Outdated
timestamp: loggingEvent.startTime.getTime(), | ||
category: loggingEvent.categoryName, | ||
levelInt: loggingEvent.level.level, | ||
levelStr: loggingEvent.level.levelStr, | ||
context: loggingEvent.context, | ||
data: data | ||
}; | ||
|
||
if (extra.length) { | ||
rec['extra'] = Object.assign(...extra); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use spread operator.
I want to support Node.js 4 until its EOL (2018-04-30).
.gitignore
Outdated
@@ -1 +1,2 @@ | |||
/node_modules/ | |||
.idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this change, please.
I don't want to add IDE specific configuration in this repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
I've used object.assign polyfill and get rid of spread operator. I've removed .idea from .gitignore. |
How do you think about log4js's context API? const log4js = require('log4js');
log4js.configure({
appenders: {
fluent: {
type: 'log4js-fluent-appender',
tag_prefix: 'tag_prefix',
options: {
levelTag: true,
host: 'localhost',
port: 24224
}
}
},
categories: {
default: {
appenders: ['fluent'],
level: 'info'
}
}
});
const logger = log4js.getLogger();
logger.info('This is info message! %d', 1);
logger.addContext("extra1", "1");
logger.info('This is info message! %d', 2);
logger.addContext("extra2", "2");
logger.info('This is info message! %d', 3);
logger.removeContext("extra1");
logger.info('This is info message! %d', 4);
logger.removeContext("extra2");
logger.info('This is info message! %d', 5);
setTimeout(() => {
log4js.shutdown(() => {});
}, 1000); |
I think context is OK but not for everything. For example if you just want to add request ID to every request then context is nice tool to use. But in other places you just want to log for example user logged event and add some information about what is this user id. So when you add user id to the context then this context is shared between everything else and you need to remove it as you wrote. For me this is unexpected behavior and I'm looking for something more close to the PHP's monolog usage. So check this example:
So if I request / then message "test with context" will be logger together with "number": 12341234 context. But just after that when I'll request /test then logger will log extra data which I don't want. As I said before, I think that .addContext methods should care about global logger data. |
You can remove extra data added by BTW You can add simple wrapper function for your purpose: pseudo code (not tested): const logger = log4js.getLogger('mylogger');
function logWithContext(level, message, context) {
context.forEach((value, key) => { logger.addContext(key, value)});
logger.log(level, message);
logger.clearContext();
} |
I've made some change in order to parse "data" field differently.
Right now .info, .warning etc methods accepts multiple arguments and they are collapsed into one string finally.
In this version: