captains-log: When using a custom Winston logger, hide the prefix and use Winston's instead

[sailsbot]

From @felixmc on February 27, 2015 0:37

Right now if I use a Winston logger with Captain's Log, I get the following:

It would be nice if Captain's Log detected that I am using a custom Winston logger and did not print a prefix to logged messages. That way, if "showLevel" is true in Winston, only the Winston prefix shows, and if "showLevel" is false in Winston, no prefix shows at all.

Copied from original issue: balderdashy/captains-log#14

[sailsbot]

From @richdunajewski on February 27, 2015 16:11

I'm seeing the same result. It looks like lib/configure.js contains code to suppress all prefixes with options.prefix = false, but the intention in the comments does not follow through to the execution of the code.

If you do set options.prefix to false, configuredPrefix is set to an empty string, but the next line configuredPrefix = configuredPrefix || prefixes[logAt]; sets configuredPrefix to the default, and you get your double prefixes.

In my opinion, this code needs to be refactored to work as expected, and the implementing developer should set prefix: false in the options object when instantiating captains-log. Then we should have proper control over the inclusion of prefixes without forcing the change in behavior for other custom loggers that may not show the logging levels by default, and we'd want captains-log to handle.

[sailsbot]

From @felixmc on March 2, 2015 17:56

I forked the project and tried to fix the issue and I made the following changes starting around line 95:

      // If `prefix` is explicitly set to `false` or `null`,
      // disable prefixes altogether.
      if (options.prefix === false || options.prefix === null) {
        configuredPrefix = '';
      }

      // Else default prefix for each log level to whatever's in the `prefixes conf`
      // As long as a custom logger was not specified
      else if (!options.custom) {
        configuredPrefix = configuredPrefix || prefixes[logAt];
      }

      // Else if there is a custom logger and prefix is set to true
      // Set the prefix to the default from `prefixes.conf`
      else if (options.prefix === true) {
        configuredPrefix = prefixes[logAt];
      }

Actual source file can be found here: https://github.com/felixmc/captains-log/blob/master/lib/configure.js

This creates the following behavior (I made sure to test each scenario described below):

Without a custom logger, everything should work the same, except that it only sets the default prefix if options.prefix is not false or null.

With a custom logger and no prefix defined, no prefixes are set and relies on the custom logger to handle its own prefixes (probably preferred behavior with most custom loggers).

With a custom logger and options.prefix defined to true it adds the default prefixes before passing the message to the custom logger (this option might result in duplicate prefixes like above if the custom logger has its own prefixes, but it would be useful if the custom logger did not have it's own prefixes).

With a custom logger and options.prefix is defined as a string, the string prefix is prepended to the message sent to the custom logger.

Let me know what you think of my solution and if this seems like correct behavior based on the options, I would like to make it into a pull request if it seems stable enough.

[sailsbot]

From @richdunajewski on March 2, 2015 19:50

Looks good to me so far -- I dropped your file on top of mine and everything appears as expected.

[sailsbot]

Thanks for posting, @felixmc. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

• review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
• create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

[sailsbot]

From @mikermcneil on January 8, 2016 8:26

A workaround is to force the prefixes to go away using empty strings (see https://github.com/balderdashy/captains-log/issues/16#issuecomment-169928141). That said, it'd be great to have this be the automatic default if a valid custom logger is provided. On the other hand @richdunajewski, to your point, I'm also cool with that (needs inspect: false as well too in some cases).

And @felixmc: I think this is great, the best of both worlds. I'd love to merge that as a PR. I'll rephrase, just to make sure I'm 100% on the same page: so custom remains an optional config property. And prefix also remains an optional property-- but its default depends on the other config: if a custom logger is provided and prefix is unspecified, no prefixes will be added. If no custom logger is provided, then the current default prefixes will be used (and observe the other config accordingly). Regardless, if prefix is explicitly disabled using false or null, then no prefixes will be used (regardless of whether custom was provided or not). Thanks!

Also, I discussed this a bit in the other issue, but I think giving this page some love would be a big additional step we could take to help other people out in the future.

[sailsbot]

From @felixmc on January 11, 2016 0:40

@mikermcneil your explanation is accurate! I've already made a PR (balderdashy/captains-log#15), but I think I need to write some unit tests for it, and maybe while I'm at it I'll update docs you pointed out.

[sailsbot]

From @mikermcneil on January 14, 2016 7:37

@felixmc thanks for that! If you wouldn't mind adding some tests in that PR, and then linking to a PR in sails-docs with the changes there, I'll get her merged in. In case it saves some time to have the link, here's a change updating the relevant place in the docs which I just merged from @kevinob11.