graphile / migrate

Opinionated SQL-powered productive roll-forward migration tool for PostgreSQL.
MIT License
751 stars 58 forks source link

feat(log): integrate @graphile/logger to allow log overrides #109

Closed PizzaPartyInc closed 3 years ago

PizzaPartyInc commented 3 years ago

Description

Hi!

We are using graphile-migrate mainly in library/programmatic API mode instead of the CLI mode. There is a slight problem with graphile-migrate writing logs only using console. So, if the application uses a custom logger and logs messages in JSON format, there is no way to override default console logging in graphile-migrate and several types of logs are produced, which is not convenient when said logs are transported into log storage, having most logs using the same formatting is preferable. This PR adds an optional logFactory setting to be able to override console logs which can be used in library mode. If the value is not specified, console is used by default via newly added @graphile/logger integration.

console calls in CLI-related code parts were left without changes.

Please let me know if some more adjustments are expected :)

Performance impact

Should not be any, unless the user of graphile-migrate decides to add some complicated logger...

Security impact

Unknown, but since this is a configuration change, it's up to the user of the library. Considering that users will have more control over writing logs - it's potentially more beneficial to security since a custom logger would be able to not log potentially sensitive data.

Checklist

benjie commented 3 years ago

You've given me the impetus to do what I've been meaning to do for a while - break out the Graphile Worker logger into it's own module:

https://github.com/graphile/logger

I've just re-integrated that back into Graphile Worker:

https://github.com/graphile/worker/pull/181

Would you be interested in reworking this PR to utilise this logging framework instead? I'd like to go with consistency across the Graphile stack.

PizzaPartyInc commented 3 years ago

Adjusted to use @graphile/logger internally. Also had to export some @graphile/logger internal types explicitly, in case @graphile/logger is not installed in the target project where graphile-migrate is used. Example usage:

import { Logger} from 'some-custom-logger-class';
import {
  MigrateLogFactory,
  MigrateLogLevel,
  MigrateLogMeta,
} from 'graphile-migrate';

//....

const logger = new Logger('Migrations');
const logFactory: MigrateLogFactory = () => {
  return (
    level: MigrateLogLevel,
    message: string,
    meta?: MigrateLogMeta,
  ): void => {
    switch (level) {
      case MigrateLogLevel.ERROR:
        meta?.error
          ? logger.error(meta?.error, message)
          : logger.error(message);
        break;
      case MigrateLogLevel.WARNING:
        logger.warn(message);
        break;
      case MigrateLogLevel.INFO:
      default:
        logger.log(message);
    }
  };
};

Had to adjust a few cases where errors are logged, but overall tried to keep the logged messages the same as before.

benjie commented 3 years ago

Ping me when you’re ready for re-review 👍

PizzaPartyInc commented 3 years ago

Adjustments are made, PR ready for re-review. Integration would now look like this:

import { Logger} from 'some-custom-logger-class';
import {
  MigrateLogFactory,
  MigrateLogger,
  MigrateLogLevel,
  MigrateLogMeta,
} from 'graphile-migrate';

//....
const logger = new Logger('Migrations');
const logFactory: MigrateLogFactory<Record<string, unknown>> = () => {
  return (
    level: MigrateLogLevel,
    message: string,
    meta?: MigrateLogMeta,
  ): void => {
    switch (level) {
      case MigrateLogLevel.ERROR:
        meta?.error
          ? logger.error(meta?.error, message)
          : logger.error(message);
        break;
      case MigrateLogLevel.WARNING:
        logger.warn(message);
        break;
      case MigrateLogLevel.INFO:
      default:
        logger.log(message);
    }
  };
};
const migrateLogger = new MigrateLogger(logFactory); // passed to settings
benjie commented 3 years ago

This went out in graphile-migrate@1.2.0-0 (graphile-migrate@next) :+1: