AEB-labs / cruddl

Create a GraphQL API for your database, using the GraphQL SDL to model your schema.
https://aeb-labs.github.io/cruddl/
MIT License
131 stars 17 forks source link

Logging configuration #116

Open deinspanjer opened 5 years ago

deinspanjer commented 5 years ago

What is the right way to handle configuration of the logger? Based on notes in the source code, I tried bringing in log4js as a dependency and then changing the log level like this:

const log4js = require('log4js');
log4js.getlogger().level = 'warn';
log4js.getLogger('ArangoDBAdapter').level = 'warn';

const project = new Project({
  loggerProvider: log4js,
        sources: [
...

But even with that, I still get a log message from the ArangoDBAdapter: ArangoDBAdapter: Executing AQL: @v_result1 = execute(

Is it right for me to be bringing in log4js as a dependency? it doesn't look like the built-in ConsoleLoggerProvider is exported, nor is it actually configurable, so I'm thinking that part is right.

henkesn commented 5 years ago

You can have a look at LoggerProvider. You can either have your own logger or you can pass log4js directly as you do in your code. Implementing your own provider also lets you add specific data like a request id or a username (delegator style). Regarding the log level: I don't think you change the level like this. You only set the level for the logger object retrieved by getLogger() which remains unused in your code. Please have a look at the log4js manual how to properly configure log levels.

henkesn commented 5 years ago

Regarding the ConsoleLoggerProvider, this is only for internal test purposes. We don't want to have the log4js in the cruddl package.json. That's why there is an unexported minimalistic console logger implementation in the code.

deinspanjer commented 5 years ago

So the way that I am setting the level works for most things inside cruddl, everything except for that one message that is logged inside ArangoDBAdapter. I believe it works because I am configuring the default logging provider that is returned whenever .getLogger() is called, so cruddl sees that new value.

I can't pass an instance to loggingProvider because the instances don't have the constructor or getLogger() function that the interface expects.

I'll keep looking in the log4js docs, but their minimalist configuration and usage just looks like this:


var logger = log4js.getLogger();
logger.level = 'debug'; // default level is OFF - which means no logs at all.
logger.debug("Some debug messages");```
deinspanjer commented 5 years ago

Okay, I found the more appropriate way to configure the default logger, but I'm still getting the logging from ArangoDBAdapter. Here is my new code snippet:

        const loggerProvider = log4js.configure({
            appenders: { out: { type: 'stdout'}},
            categories: {default: {appenders: ['out'], level: env.logging.LEVEL}}
        });

        const project = new Project({
            loggerProvider,
            sources: [
henkesn commented 5 years ago

Hm that sounds strange. Is it just this Executing AQL... log which you get? In the code there is even a condition around this log because the log generation is expensive. Perhaps you can debug into that line and check if the log4js level is correct. Or do you perhaps redefine the default log levels?

deinspanjer commented 5 years ago

Yes, it is only that one message I'm getting. I was wondering if maybe the fact that the adapter seems to try to set up its own logger was preventing the one I was passing in from being used.

henkesn commented 5 years ago

Yes, but you are not passing a logger but a LoggerProvider. The Logger itself is created when needed, specifically for a category. The ArangoDBAdapter still uses your provided LoggerProvider.

deinspanjer commented 5 years ago

Could you see if you can replicate the behavior?