Make it easy to install plugins per-logger, not just globally

[pimterry]

cc @shellscape - pulling out the conversation from #82 (comment) as separate issue.

[pimterry]

To repeat the current problem from the previous thread:

Take a look at the master branch of webpack-dev-middleware, lib/log.js and the same location in webpack-dev-server. Both are using loglevel and both want to apply a plugin to loglevel (the prefix plugin). Since webpack-dev-server uses webpack-dev-middleware, and node caches modules, we end up with the same base instance being shared between the two. And well, the plugin doesn't like that, for good reason since it can't apply itself to the same instance twice with different settings. It's the lack of true different instances that create the scenario. If loglevel returned a class that we instantiated, the scenario wouldn't exist.

This is probably slightly edge-casey since the probability of this is pretty low. But these are two rather high-profile modules, so maybe the edge case could be forgiven. I'd be happy to crank out the needed changes on a fork for Node 6+, but it'd require someone on your end setting up the right babel env to compile it back down for browser use. That is to say, if you feel that direction is even right for the project.

[pimterry]

This totally makes sense, and that is a use case I'd like to support properly. I'll take a proper look at it this weekend, but I definitely think there's a route through there.

As a first impression, I think the right approach is to make it easy to scope plugins to a specific logger instance. We've already got getLogger to build the separate logger instances you're talking about (they're initially based on the root instance, but they're otherwise independent once created), so it seems like a reasonable approach would look something like:

const log = require('loglevel');

// In the webpack-dev-middleware:
const middlewareLogger = log.getLogger('middleware');
// In webpack-dev-server:
const serverLogger = log.getLogger('dev-server');
// [etc]

applyPlugin(middlewareLogger);
// ^^ exact setup here is plugin specific 

middlewareLogger.warn(...); // plugin should have wrapped this method
serverLogger.warn(...); // plugin should not have wrapped this method

Would that work for you?

I think almost all the plumbing is here to make this approach work, and it might even already work out of the box (though I need to find some time to thoroughly test that), and the real problem is that loglevel-plugin-prefix specifically checks to block this: https://github.com/kutuluk/loglevel-plugin-prefix/blob/master/lib/loglevel-plugin-prefix.js#L44.

@kutuluk is there a specific reason you need to check this, and enforce that the plugin is applied to the root logger?

[shellscape]

The code in the comment preceding this one is what we had in mind, yes, thank you. In looking at the prefix plugin, it appears to be proxying the method factory, which doesn't seem to be doable on anything but the root logger.

[shellscape]

@pimterry yeah in taking another look at the code, defaultMethodFactory is outside of the Logger class and a static export. It'd need to be moved within the instance for the prefix plugin to be able to operate on it successfully.

[kutuluk]

@pimterry
I made this lock because the child logger can not override the plugin in the parent logger when assigning the plugin.

This leads to the fact that if the plugin is assigned to both the root logger and the child logger, then the plugin is executed twice.

Therefore, the plugin can be applied either only to the root logger, or only to the child loggers. I chose the root logger.

[pimterry]

@shellscape I've done some investigation. That explanation above isn't correct - defaultMethodFactory isn't an export at all, static or otherwise, and isn't used in the plugin code either. I've added a test that covers this exact case, and separate child loggers already independently handle separate plugins just fine, see: f9bf1e3

@kutuluk That's explanation is very useful, thanks. I see what's going on here I think - you're saying that it would work (i.e. you can apply plugins independently to different loggers), but that doing so breaks the prefix plugin's code because it would have to be run repeatedly, and that will break things. Is that about right?

Looking at that code, you've got a static loglevel, originalFactory and pluginFactory, which looks like the key problem - any way you cut it, you can only totally safely apply that plugin once in a JS process.

This is your underlying issue @shellscape. You don't need changes in loglevel at all to fix this, as far as I can tell - the tricky global state is on the plugin side.

If the above's all correct, we can fix this by disabling the root logger check in the plugin and making the state linked to the logger somehow, rather than being global (e.g. you could use logger.name as a key to build a map for this state, if you do need it globally, which should work, or you could just attach properties to loggers I guess).

The semantics of disable will need thought though: it needs to decide whether it's disabling the plugin for all loggers, or it could take a specific single logger instead (or you could overload and do both).

[kutuluk]

In the current implementation, loglevel, originalFactory and pluginFactory are static because I allow the plugin to be applied exclusively to the root logger. I can easily rewrite the code so that these values ​​are stored in the apply scope and the plugin can be applied to any number of child loggers. But then I will be forced to prohibit the use of a plugin with a root logger.

The key problem is that you assign a plugin to the root logger, and then to the child logger, then when you call the method, the plugin with the settings of the child logger will be executed first, and then the plugin with the settings of the root logger will be executed. The code does not break, the plugin will work fine in both cases, but the user will get a behavior that he obviously does not expect.

The current loglevel api does not provide tools that could fix this problem, so I'm forced to implement the plugin as it is now.

[pimterry]

Sorry, I've been off for Christmas - I'm now back looking at this!

There is indeed some tricky plugin UX there. It's not super clear to me how to have both namespaced plugins and default 'global' plugin configuration, without loglevel providing a much more complex api to manage the currently applied plugins, which is both more complex and less flexible than the current 'just provide a function' approach.

Alternatively, perhaps child loggers could be linked to the root logger, rather than copying it on setup? I.e. if you haven't added a plugin to a child, it should behave like the root logger (even if the root has had plugins added after the child was created), but when/if you do add a plugin to a child, then it should use that plugin and ignore any of the root logger's plugins entirely. Does that make sense? It'd be fairly easy to implement, but it would need a breaking change in behaviour, and maybe the plugin api itself (though I think that's optional), and I'm not sure if it is the most intuitive or useful model. Thoughts?

Alternatively, for now, I think you can handle this in the plugin itself, by detecting this specific case. You can allow people to set plugins for either child loggers or the root logger, but record whether they've done so (e.g. hasBeenAppliedToRoot/hasBeenAppliedToChild), and then throw an error only if somebody tries to do both. I.e. fail when they apply the plugin to a child, after having done the root, or vice versa, but don't block anything initially.

That's not perfect, since there are niche cases where you could want to separate manage root and child plugins, but it's fairly simple to do I think, and it does give users a lot more flexibility than they have now.

[shellscape]

I was able to solve this in my fork without pain. In my implementation, getLogger can simply be proxied and a new "plugin" ("plugins" in my implementation are nothing more than a Factory) applied to each logger created. But that relies upon each logger being a wholly unique instance.

[pimterry]

Ah, interesting. Yeah, that already works here - you should be able to proxy getLogger right now, and control how child loggers get built & how the method factory inheritance works in any way you like.

The problem, as you've discovered, is that that only works if all your loggers are totally independent. That's a bit limiting, and changing it would certainly be a breaking change. Right now, using the shared ready-out-of-the-box root logger is how loglevel is primarily used afaik (child loggers are relatively new), and it's a simpler model for the standard easy use case, where you don't really need separate independent loggers.

My suggestion above I think lets you bridge this, a bit: you'd have a root logger, plus child loggers that share plugins by default, but which can become totally independent if you try to add a plugin to any of them individually. That lets you do global or local logging totally however you like, in a composable way (separate libraries can decide to do either, within one codebase) My main concern is whether that's nice & intuitive, or really just confusing...

[shellscape]

That's a bit limiting, and changing it would certainly be a breaking change. Right now, using the shared ready-out-of-the-box root logger is how loglevel is primarily used afaik

We haven't found it remotely limiting. In fact, we've found the exact opposite. The root logger is still present in our implementation - it's simply an instance, like any "child" logger (we don't call them that), extended with sugar for ease-of-use. Insisting on a "master logger to rule them all" and implementing a master-child relationship is actually limiting, and I believe this thread/topic exposes the reasons why.

I wasn't suggesting that you implement our paradigm - the differences of opinion on that are clear -
only sharing how we solved this successfully.

[pimterry]

We haven't found it remotely limiting.

I'm talking about the limitation you described above: 'that relies upon each logger being a wholly unique instance'. I'm not necessarily against going in that direction, especially if this necessitates a breaking change regardless, but it is a new constraint.

I wasn't suggesting that you implement our paradigm

Ok, but to be clear: I am quite open to implementing that same paradigm.

There are short-term workarounds, and other options to discuss too, but this in general seems like somewhere where there's clear value in improvements, and those are likely to be breaking regardless. Keeping plugin compatibility with your fork would be a nice free benefit to this approach, so you could benefit from the existing plugin ecosystem too, once they upgrade.

[kutuluk]

kutuluk/loglevel-plugin-prefix@c7c9749

Solved =)

[pimterry]

@kutuluk neat, ok. Just so I understand, your solution there is:

• Remove disable() entirely.
• Allow applying to child loggers
• Automatically skip duplicate application (by checking logger names)
• Wrap the methods only if it was applied directly to a child, or its been inherited from the root, not applied (i.e. it's running on a named child logger, but has never been applied to that named child logger)
• If you do apply to a child that's inherited from the root, merge the plugin configuration.

Is that right? Implementation is a little complicated, but pretty neat, and it works very nicely & fairly intuitively from an end user POV imo.

Obviously it'd be better if we could make it simpler for plugin authors to do this, and I think I'd like the get the built-in approach closer to this model out of the box (and in a perfect world, support some kind of disable), but it's cool that it's possible to build it on top directly like this in the current model anyway. Good stuff!

[kutuluk]

Wrap the methods only if it was applied directly to a child, or its been inherited from the root, not applied (i.e. it's running on a named child logger, but has never been applied to that named child logger)

Methods are wrapped if the plugin is applied to the logger for the first time. Each time only the configuration changes.

[kutuluk]

Simplified plugin:

const defaults = {
  prefix: 'prefix',
};

const configs = {};

const apply = (logger, config) => {
  if (!logger || !logger.setLevel) {
    throw new TypeError('Argument is not a logger');
  }

  const originalFactory = logger.methodFactory;
  const name = logger.name || '';

  function methodFactory(methodName, logLevel, loggerName) {
    const originalMethod = originalFactory(methodName, logLevel, loggerName);
    const options = configs[loggerName] || configs[''];

    return function (...args) {
      // skip the root method for child loggers to prevent duplicate logic
      if (name || !configs[loggerName]) {
        args.unshift(options.prefix);
      }

      originalMethod(...args);
    };
  }

  if (!configs[name]) {
    logger.methodFactory = methodFactory;
  }

  const parent = configs[name] || configs[''] || defaults;
  configs[name] = Object.assign({}, parent, config);

  logger.setLevel(logger.getLevel());
  return logger;
};

export default { apply };