Meowhal / osu-ahr

irc bot for osu multi lobby auto host rotation
MIT License
129 stars 37 forks source link

Single logger instance to each type of logger #82

Closed xayanide closed 2 years ago

xayanide commented 2 years ago

Resolved by #99

Requesting an idea. Have only one instance of each type of logger. e.g cli, discord, irc, PMLogger, etc instead of importing log4js and configuring the path everytime they are going to be used.

Doesn't have to be done right away. Can also take a different but similar approach from what I will show below.

It might look something like this:

Before:

// file where cli logger is used
import log4js from 'log4js';
const logger = log4js.getLogger('cli');
log4js.configure('./config/log_cli_prod.json');

logger.info('Hello world.');

After:

// file where cli logger is used
import { cliLogger } from '../cliLogger';

cliLogger.info('Hello world.');

Less lines used just by using log4js's logger, and straightforward than it already is.

// cliLogger.ts
import log4js from 'log4js';

// directly putting the configuration
log4js.configure({
// the whole config here
});

// or by using the json config path
// log4js.configure(configPathHere);

export const cliLogger = log4js.getLogger('cli');

// -------------------------------
// another way of implementation
import log4js from 'log4js';
export const cliLogger = log4js.configure({configHere}).getLogger('cli');
// ------------------------------

In the case for discord appender, I think it is possible to do it something like this

// discordLogger.ts
import log4js from 'log4js';
import path from 'path';

// example only, this is not the whole config, might not work as intended
log4js.configure({
  appenders: {
    discord: {
      type: `${path.join(__dirname, 'discord', 'DiscordAppender')}`,
      layout: {
        type: 'pattern',
        pattern: '%m',
      },
    },
  },
  categories: {
    discord: {
      appenders: [
        'console_f',
        'file_app',
      ],
      level: 'all',
    },
  },
});

export const discordLogger = log4js.getLogger('discord');

// -------------------------------
// another way of implementation
import log4js from 'log4js';
import path from 'path';
export const discordLogger = log4js.configure({configHere}).getLogger('discord');
// ------------------------------
// file where discord logger is used
import { discordLogger } from '../discordLogger';

discordLogger.info('Hello world.');
Meowhal commented 2 years ago

log4js.configure is to be run only once for the entire application. It should be considered separately from the instance.

Oh, my God. I have found a bug. I have instanced the cli logger before running log4js.configure.

import log4js from 'log4js';
// const logger = log4js.getLogger('cli');
log4js.configure('./config/log_cli_prod.json');
const logger = log4js.getLogger('cli'); // <- Should be instantiated here!
logger.info('Hello world.');

I am sorry this mistake has caused you concern.

In addition, I noticed another possible bug. Loggers that are instantiated in the top-level scope of each file may not be finished configuring depending on the import order. This should be fixed.

Meowhal commented 2 years ago

OSU-AHR has two types of loggers: one logger that has only one instance in the entire application. The other is a logger that may have multiple instances per lobby. Singleton loggers are managed on a file basis, which is awkward for classification purposes. It would be more reasonable to organize the logs by defining instances in a module and importing them, as you suggest.