Console.log ??

[memelet]

I notice that all logging is hard-coded to use Console. Is there some way to redirect this modules console output to a application specific logger so it can be routed to eg, a file then ELK, like all the other logs in our api?

[jsdevel]

Hmm. I see what you mean. Have any ideas? Maybe a console provider option in the initialize args?

[memelet]

Since almost all logging modules support the basic api log.info,warn,error(string) take an logging instance in the initialize(). This would allow the use of bunyan or winston. And even if the logger did not support just that interface it would be trivial to map into it.

It's too bad that node does not have something like slf4j.

[jsdevel]

Yea. I'd love it if you could submit a PR with support for such an interface in .initialize(args) @memelet .

[memelet]

If we end up with swagger vs raml, I will do so. Thanks!

[mbsimonovic]

I'd like to give this one a crack. Any ideas on how/what it should look like? Guess a TypeScript Logger interface, and a couple of implementations for popular packages such as bunyan, winston, ..

[jsdevel]

Go for it! I would like to avoid adding interfaces from 3rd party packages if possible i.e. a simple interface with log & error methods would probably do for now

[mbsimonovic]

this again spans multiple packages, so i'd like to agree on a plan before making any changes.

I was thinking something along these lines (please feel free to change my typescript, it's pretty noob);

// in express-openapi

export interface GenericLogger { //there's already one called Logger
  error(message: string, ...extra: any[]): void;
  warn(message: string, ...extra: any[]): void;
  info(message: string, ...extra: any[]): void;
  debug(message: string, ...extra: any[]): void;
}

class NoopLogger implements GenericLogger {
  public debug(message: string, ...extra: any[]): void {
    // noop
  }

  public error(message: string, ...extra: any[]): void {
    // noop
  }

  public info(message: string, ...extra: any[]): void {
    // noop
  }

  public warn(message: string, ...extra: any[]): void {
    // noop
  }
}
export interface ExpressOpenAPIArgs extends OpenAPIFrameworkArgs {
  app: Application;
  consumesMiddleware?: { [mimeType: string]: RequestHandler };
  docsPath?: string;
  errorMiddleware?: ErrorRequestHandler;
  exposeApiDocs?: boolean;
  promiseMode?: boolean;
  securityFilter?: RequestHandler;
  logger?: GenericLogger;
}
export function initialize(args: ExpressOpenAPIArgs): OpenAPIFramework {
  const logger = args.logger ? args.logger : new NoopLogger();
// from now on it's always safe to call logger.info ...
}

the console object can also be passed as a logger. Winston and bunyan should also work, they all have those 4 methods (error, warn, info, debug).

Now the question is how to expose this logger instance to other packages? Seemed like the type should be defined in openapi-framework/src/types.ts?

[jsdevel]

Couple of suggestions:

Instead of (message: string, ...extra: any[]) how about just (data: any, ...args: any[]). This matches most loggers and more specifically nodejs's console methods.

Instead of adding logger to each of the frameworks' Args interfaces, I feel it should be defined in OpenAPIFrameworkArgs.

Now the question is how to expose this logger instance to other packages? Seemed like the type should be defined in openapi-framework/src/types.ts?

I'd agree with this.

Other than that, sounds like a good plan!

[mbsimonovic]

Instead of adding logger to each of the frameworks' Args interfaces, I feel it should be defined in OpenAPIFrameworkArgs.

I was going to add it there but then that crosses package boundary so just to test the idea it ended up in ExpressOpenAPIArgs.

Now the question is how to expose this logger instance to other packages? Seemed like the type should be defined in openapi-framework/src/types.ts?

I'd agree with this.

So how would each package receive the logger instance, request-validator for example?

1. It comes from express-openapi's initialize method, which accepts an instance of ExpressOpenAPIArgs (which extends OpenAPIFrameworkArgs).
2. express-openapi creates openapi_framework so it could pass along this logger instance thru the args.
3. openapi_framework sets up routes and creates request-validators, so at this point it could pass the logger thru the OpenAPIResponseValidatorArgs which would have to be modified. request-validator needs to know the type, so it has to import it, and I don't see that it imports anything from openapi-framework, so perhaps the Logger type should go to openapi-types?

To get this to work, I'd have to modify 4 packages: first PR for openapi-types, then for OpenAPIFrameworkArgs, then for request-validator and finally for express-openapi.

Or is there some other way?

[jsdevel]

I see your points @mbsimonovic. I see the following options as viable:

1. add an OpenAPIFrameworkLogger interface to openapi-types. Not sure if I like it because it would make that package no longer framework agnostic.
2. The low level packages could import openapi-framework if we wanted to expose the logger interface in openapi-framework, but that'd be circular and likely a dependency management nightmare.
3. We create a new openapi-framework-types package and add the logger interface there.
4. Each package redefines the interface. The duplication is obviously the downside the this approach and anytime we want to make a change to the interface we have to update n number of packages.

Not sure if I can think of any more options.

I feel option 1 is likely the most practical. Maybe not the purest. Is there a logger interface already defined in some other package?

[mbsimonovic]

1. add an OpenAPIFrameworkLogger interface to openapi-types. Not sure if I like it because it would make that package no longer framework agnostic. .. I feel option 1 is likely the most practical. Maybe not the purest. Is there a logger interface already defined in some other package?

you mean some other npm package, outside https://github.com/kogosoftwarellc/open-api?

what's this https://www.npmjs.com/package/@types/bunyan, or this https://github.com/winstonjs/winston/blob/master/test/typescript-definitions.ts?

Maybe some unrelated to these particular ones:

1. https://www.npmjs.com/package/ts-log (24 lines of code: https://github.com/kallaspriit/ts-log/blob/master/src/index.ts)
2. https://www.npmjs.com/package/typescript-log
3. https://www.npmjs.com/package/triviality-logger

[jsdevel]

thanks for doing some research @mbsimonovic !

I like ts-log's Logger interface. Seems to fit the bill just right (lightweight, focused, node compliant logger that's generic and not specific to any framework). I'd happily welcome that being a dependency in each of the packages and accepted as an optional arg.

[jsdevel]

in thinking about it further, we may run into issues supplying console for a ts-log Logger.

[mbsimonovic]

so you want to include it as a dependency, or just copy/paste the 24 lines index.ts?

[jsdevel]

i'd say include it as a dependency in each of the packages. you'll probably have to create a default logger in openapi-framework that implements the Logger interface though, as I don't believe it will work by just passing in console.log. Maybe it will.

[mbsimonovic]

no, seems to work fine with the console object passed in. so in which order should the PRs come?

[mbsimonovic]

linter fails when i install ts-log:

npm run lint
> @open-api/[email protected] lint 
> tslint './packages/**/*.[j,t]s'

Failed to load open-api/packages/openapi-framework/node_modules/ts-log/tslint.json: Invalid "extends" configuration value - could not require "typestrict". Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) for the approximate method TSLint uses to find the referenced configuration file.

ts-log/tslint.json:

{
  "extends": ["typestrict"],
  "rules": {
    "no-any": false,
    "max-classes-per-file": false,
    "no-magic-numbers": false
  }
}

i'm not sure why ts-log even gets checked when express-openapi's tslint.json has it excluded:

  "linterOptions": {
    "exclude": [
      "**/node_modules/**/*",

Perhaps this isn't correctly configured? @jsdevel could you please just install ts-log and run linter.

[mbsimonovic]

tried all these and none correctly excludes packages/openapi-framework/node_modules:

tslint --exclude 'node_modules' ./packages/**/*.[j,t]s
tslint --exclude 'node_modules/*' ./packages/**/*.[j,t]s
tslint --exclude 'node_modules'/**/* ./packages/**/*.[j,t]s
tslint --exclude '*/node_modules'/**/* ./packages/**/*.[j,t]s
tslint --exclude '**/node_modules'/**/* ./packages/**/*.[j,t]s
tslint --exclude '*/**/node_modules'/**/* ./packages/**/*.[j,t]s

On my Mac OS 13, and tslint v5.12.0 only this one correctly excludes:

tslint --exclude './packages/**/node_modules/**/*' ./packages/**/*.[j,t]s

As soon as this gets fixed, I can open the first PR.

[mbsimonovic]

ah, found the reason, tslint doesn't read tslint.json by default, it has to be specified via -c flag. i'll open a PR

[jsdevel]

Yea, that seems counterintuitive. Thanks for fixing that.

[mbsimonovic]

@jsdevel thanks for closing PR #330!

I'd like continue and add logging to other packages, fs-routes to start with, so here's a new PR #363

[elyobo]

Safe to close now that #61 is merged @mbsimonovic @jsdevel?

[jsdevel]

@elyobo almost. we have a few more packages that need to accept the logger. cc @mbsimonovic

[mbsimonovic]

ah sorry, didn't realise there was more work to do. so what's missing?

[elyobo]

A naive look doesn't show much

~/src/open-api(master)$ ag --ts --js console\\. packages/ | grep -v test
packages/fetch-openapi/bin/fetch-openapi.js:112:  console.error(chalk.red(LOG_PREFIX) + msg);
packages/fetch-openapi/bin/fetch-openapi.js:122:    console.error(chalk.gray(LOG_PREFIX) + msg);
packages/fetch-openapi/bin/fetch-openapi.js:127:  console.log(chalk.green(LOG_PREFIX) + msg);
packages/openapi-framework/src/types.ts:20:   * `console.debug` is just an alias for `.log()`, and we want debug logging to be optional.
packages/openapi-framework/src/types.ts:28:    console.error(message, ...optionalParams);
packages/openapi-framework/src/types.ts:32:    console.info(message, ...optionalParams);
packages/openapi-framework/src/types.ts:36:    console.trace(message, ...optionalParams);
packages/openapi-framework/src/types.ts:40:    console.warn(message, ...optionalParams);
packages/openapi-security-handler/index.ts:77:              console.warn(

The ones in types.ts are the adapter for the console, so it's appropriate there anyway. The fetch-openapi.js is a CLI tool, so also seems fine. That only leaves the openapi-security-handler/index.ts it seems?

Perhaps also reenable the noConsole rule in tslint.json and disable it for the few specific cases that it is known to be wanted to prevent it creeping back in.