marcelog / Nami

Asterisk manager interface (ami) client for nodejs
http://marcelog.github.com/Nami
Apache License 2.0
99 stars 59 forks source link

Make nami a bit more configurable; Add option to disable logging #10

Closed mbrevda closed 11 years ago

mbrevda commented 11 years ago

Logging will be automatically disabled if log4js is not found.

marcelog commented 11 years ago

Hey! Thanks for your contribution :)

So logging is useful, for example to log errors and warnings, so I would personally keep at least one way of logging stuff, even if it is on stdout and stderr. And if the issue is the number of debug messages, the log level can be easily configurable in the nami config.

On the other hand, we could avoid having all the if's around the code with a small abstraction over the logging feature, that will know if log4js is present and either log to console or use it.

On the other-other hand, log4js is a requirement easily installable by running npm install.. what issues did you have with that? Just curious, because the deps are locally to a particular installation.

Cheers :)

mbrevda commented 11 years ago

I should have explained. I was trying to accomplish as follows:

  1. A way to turn down the debug noise
  2. Not to force any particular logging mechanism/enhancer

I don't have an issue with logs per se, and agree that errors and warnings are important to keep around. I was going to just replace logger with console but log4js has methods that console does not have. This would also not turn down the noise.

the log level can be easily configurable in the nami config

Is that currently implemented, or were you making a suggestion? If it's not, I would suggest abstracting the logging to a Nami-provided object, which can then pass the calls to console. This would allow the log level to be configured globally without if statements.

It seems that log4js can replace console, so that you don't actually have to hardcode ANY referenced to log4js in nami, the user can do this in their app of they chose so. This would reduce the need for dependencies.

Thoughts?

marcelog commented 11 years ago

Cool, so configuring the loglevel is already implemented in log4js. You can check their repo and their wiki. Also, there's an example in AsterTrace-Node, in this file. You can just initialize log4js in your own app and nami will pick it up from there (when requesting the predefined loggers for it).

About replacing the console.. well, it's an option, I would personally prefer to use something more powerful like log4js from nami, and let the end user decide in its own app whether to replace the console or actually use the log4js capabilities by just tweaking the log4js options and initialization code (nami does not interfere with this, so you get total freedom on that). I might agree, however, that you might want to use other logging frameworks. In that case it might be useful to have the abstraction over the logging feature (like commons-logging in the java world, although i don't know if there's something like that available for node, but I wouldn't go for anything else other than that, otherwise you would be tied to another framework, ending up in the same situation).

Also is pretty common to depend on a logger (in almost any language and app), and having a dependency in a logger is actually (imho, of course) less harmful than reinventing the wheel of having your own logger in every application distributed in the wild, and you can (optionally) take advantage of features like different appenders, log formats, log rotation, different logging levels, etc. And having this installed locally reduces the chance of collitions.

Don't get me wrong, I do like to depend on as few things as possible, in the case of nami, log4js is the only dependency needed, and it's there just to give the end user the freedom to have a console logging, or something more complex, because applications can vary a lot depending on the use case.

Does that make sense? Just curious: how do you handle these situations in your own applications? You just log to the console? Can you handle different loglevels, split appender configurations (like having certaing stuff go to a file and other stuff to another)? etc?

mbrevda commented 11 years ago

In general, the least amount of dependencies is usually the simplest. That doesn't negate your points, nor minimize the fact that nami has exactly one. It seems we both agree that abstracting the logging mechanism is not a bad idea.

I generally log to stdout/syslog for easier log collection across systems. I was looking for how to disable log4js on their site before I started the patch, but I must have missed it. Can you point it out to me please?

marcelog commented 11 years ago

Hey :)

Well I agree in that it would be cool to have something like commons-logging, although it escapes nami scope (it deserves to be a project on its own, and would end up being a dependency anyway).

Check out the links I paste above, you should have everything you need. I'm going to close the issue now, since log4js is a required dependency and can already be configured to do what you need.

Cheers!

mbrevda commented 11 years ago

Thanks marcelog. For the googlers that may reach this, here is how I'm limiting the logging:

var namiLib = require('nami');

var nami = new namiLib.Nami({
    host: 'server',
    port: '5038',
    username: 'user',
    secret: 'secret'
});

nami.logger.setLevel('WARN');

Please note: nami, via log4js, will take over console.