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

Add support for non-bunyan loggers #88

Merged
merged 3 commits into from
Oct 25, 2018

Conversation

SerayaEryn
Copy link
Contributor

This adds support for non-bunyan loggers that do not have logger.child(), logger.addStream() or logger.level() like winston.

@basti1302 basti1302 self-assigned this Oct 15, 2018
@basti1302
Copy link
Contributor

basti1302 commented Oct 15, 2018

Hi Denis,

supporting other loggers is definitely a worthwile feature, thanks for bringing this up and submitting a PR!

I'm not 100% sure about the best configuration API to offer this. Here are some thoughts:

  1. Having a separate configuration property config.nonBunyanLogger for this feels a bit cumbersome. I think auto-detecting if config.logger is a Bunyan logger should be possible (for example, by checking for the existence of a child function) and might make using this a bit smoother. WDYT?
  2. If its not a Bunyan logger, we should probably validate if the usual logging functions (debug, info, warn, error) exist on the object received as config.logger.
  3. Whatever we decide on, I think we should add a test for it to logger_test.js.
  4. Finally, this new feature would need to be documented in CONFIGURATION.md.

It probably makes sense to come to a decision regarding (1.) before tackling (3.) and (4.). If you would rather not invest time in (3.) and (4.), I'm happy to take over those chores.

I'd like to hear your thoughts on (1.), though.

@@ -8,7 +8,10 @@ var parentLogger;

exports.init = function(config) {
if (config.logger) {
parentLogger = config.logger.child({module: 'instana-nodejs-logger-parent'});
parentLogger = config.logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand your change correctly, your intention would be to use config.nonBunyanLogger to pass in a logger object. Any particular reason to change the above line, handling the Bunyan case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was changed accidentally. i removed this change.

@SerayaEryn SerayaEryn force-pushed the support-non-bunyan-loggers branch from b6c23c1 to 75428db Compare October 18, 2018 11:43
@SerayaEryn
Copy link
Contributor Author

Hi @basti1302,

I agree, using the config.logger option is better.

I can add some test cases for the new feature and document it.

@basti1302
Copy link
Contributor

Cool, looking forward to it :-)

@SerayaEryn SerayaEryn force-pushed the support-non-bunyan-loggers branch from 75428db to ce83297 Compare October 19, 2018 09:05
@basti1302 basti1302 merged commit 9e5cec6 into instana:master Oct 25, 2018
@basti1302
Copy link
Contributor

Looks great, thanks! 🎉

This will go out with the next version.

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.

2 participants