Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

piotrkochan
Copy link

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:

  • first argument from .info is passed to data
  • all others are passed to additional field "extra" and merged together via Object.assign

Copy link
Owner

@okkez okkez left a 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);
Copy link
Owner

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
Copy link
Owner

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@piotrkochan
Copy link
Author

I've used object.assign polyfill and get rid of spread operator. I've removed .idea from .gitignore.

@okkez
Copy link
Owner

okkez commented Feb 2, 2018

How do you think about log4js's context API?
logger.info() can accept multiple arguments and format log string using format string like following.

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);

@piotrkochan
Copy link
Author

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:

const log4js = require('log4js');
const requestLogger = log4js.getLogger('request');

router.get('/', function(req, res, next) {
  requestLogger.addContext('number', 12341234);
  requestLogger.info('test with context');

  res.render('index', { title: 'This log will contain context' });
});

router.get('/test', function(req,res,next) {
  requestLogger.info('test without context');
  
  res.render('index', { title: 'There will be no context in this log' });
});

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.

@okkez
Copy link
Owner

okkez commented Feb 5, 2018

You can remove extra data added by .addContext using .removeContext or .clearContext.

BTW .addContext is log4js API.
So I don't want to add extended API to this library for this purpose.

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();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants