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

allow for custom logging #108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

allow for custom logging #108

wants to merge 1 commit into from

Conversation

RXminuS
Copy link

@RXminuS RXminuS commented Aug 25, 2016

We use bunyan for our logging needs but no-kafka only allowed us to send in 1 logFunction. This meant that we couldn't use log levels properly.

I've tried to remedy this by allowing you to pass in any object which overrides the standard logging functions. It's not as pretty as I would like it to be but I favoured backwards compatibility over my OCD ;-) Test could probably be expanded a bit more but think this covers the basics. README updated accordingly.

I haven't tested all the logging libraries, just bunyan but now my output looks beautiful :-) What do you think...worth merging?

image

@RXminuS
Copy link
Author

RXminuS commented Aug 25, 2016

@archie Chime in?

@oleksiyk
Copy link
Owner

Why can't you use that one single logFunction provided by no-kafka and split it into several functions by the log level which is the first parameter passed to it?

@RXminuS
Copy link
Author

RXminuS commented Aug 25, 2016

Didn't know that's how the default logger worked, documentation isn't great. If that works and we provided some examples in the README that might be an option.

But it still get's really ugly because of the bind that you do when you create the client...I need to bind it back to bunyan for each function I provide so I end up with a bunch of manual functions I need to send in all with a .bind(log). Given that this solution seems to be backwards compatible it just makes life a lot easier.

@RXminuS
Copy link
Author

RXminuS commented Aug 25, 2016

I think nice-simple-logger is a great default to have. But the last update was 8 months ago and it seems quite geared towards Logstash; for our production code I much rather have the possibility of bypassing it than trying to put square pegs in round holes.

But that's just my 💭

@memelet
Copy link

memelet commented Aug 26, 2016

Real pluggable logging with the string parsing would be a joy.

@archie
Copy link

archie commented Aug 26, 2016

+1 for replaceable logger instance. If not at least it'd be great if we could add documentation on how to replace each of the log functions. Let me know if I can help with anything.

@ghermeto
Copy link

ghermeto commented Oct 18, 2016

+1

BTW, @RXminuS there is a small conflict

@wdullaer
Copy link
Contributor

wdullaer commented Dec 2, 2016

I wrote a small dedicated logging function which you can pass in with the current options and plays nice with kafka (logger is a bunyan instance):

/**
 * A log function that can be used by no-kafka
 * @param  {string} messageLevel The level at which the message is logged
 * @param  {string} message      The actual message
 * @return {undefined}           Returns void
 */
function kafkaLogger(messageLevel, message) {
  // The no-kafka logging library uses a log function that can take
  // a variable number of arguments
  // We concatenate all extra arguments and use this as the bunyan message
  let args = [...arguments];
  message = args.slice(1).join(' ');
  switch (messageLevel) {
    case 'TRACE':
      logger.trace(message);
      break;
    case 'DEBUG':
      logger.debug(message);
      break;
    case 'WARN':
      logger.warn(message);
      break;
    case 'ERROR':
      logger.error(message);
      break;
    case 'LOG':
    default:
      logger.info(message);
      break;
  }
}

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.

6 participants