feature: request logging middleware for express

[ofrobots]

Similar to logging-bunyan, this PR implements a request correlating middleware for express. This is WIP, but opening now to get early feedback.

[ofrobots]

Tests are expected to fail until a new version of winston@3 is released that includes the fix winstonjs/winston#1483.

[ofrobots]

I've extracted much of the logic to googleapis/nodejs-logging#248.

[ofrobots]

New version of winston is out now. This is ready to review.

[JustinBeckwith]

Do you have a bug open somewhere to track that?

[JustinBeckwith]

👋 @ofrobots is this ready to go?

[ofrobots]

Actually it turns out I still need to do a bit more work on this.

[ofrobots]

Currently blocked on googleapis/nodejs-logging#376 being released.

[ofrobots]

Okay, this is ready for review for realz this time.

stale

[kjin]

LGTM w/ q

[kjin]

Would this always replace both the user and default option for logName?

[ofrobots]

Yes. The middleware is opinionated. It may need to create two different log streams: one for the app logs and potentially one for the request logs (when not running on GAE/GCF). The app log will always uniformly have the _applog suffix. We want to 1) distinguish the app log from the request log and 2) have uniform naming regardless of the deployment environment.

It is conceivable that we might want to make ${APP_LOG_SUFFIX} a user-configurable option, but I don't see the need for it yet.

[shaunc869]

Is there any documentation on how to enable this or does it just magically starting working inside express? Thank you for doing this!

[ofrobots]

Hi @shaunc869 this is not publicly exposed at this point unfortunately 😞. This is currently blocked by issues related to TypeScript and our transparent support for both winston2 and winston3 out of the same module: #278. I would suggest subscribing to that issue.

[shaunc869]

I just saw the merge come through, looks like we're close on this maybe?

[ofrobots]

@shaunc869 yes, this should get released around the end of the month. If you wanted to kick the tires, you can install the module from the master branch and play around: npm i googleapis/nodejs-logging-winston. Note that you'll be the first 'real' user of this feature, so this may be rough around the edges. If you have any feedback, that would be most welcome.

[shaunc869]

Sounds great I'll do that tonight any special instructions for this to work with express?

[ofrobots]

The readme on the master branch has some instructions.

[shaunc869]

Dumb question, now my import is failing and I can't see why, can these not be mixed?

import { LoggingWinston } from '@google-cloud/logging-winston';

[DominicKramer]

The way you are importing LoggingWinston should work. What specific error are you seeing?

[shaunc869]

Sorry I rebuilt and now I'm down to:

TypeError: LoggingWinston is not a constructor
    at Object.<anonymous> (/Users/hackintosh/PycharmProjects/spm-appengine/lib/util/logger.js:10:24)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/hackintosh/PycharmProjects/spm-appengine/lib/models/appointment_request.js:5:18)
    at Module._compile (module.js:653:30)
Waiting for the debugger to disconnect...

For the code:

import * as LoggingWinston from '@google-cloud/logging-winston';

const loggingWinston = new LoggingWinston({
	projectId: 'highlevel-backend',
	keyFilename: './lib/highlevel-backend.json',
	serviceContext: {
		service: process.env.GAE_SERVICE,
		version: process.env.GAE_VERSION,
	},
});

[DominicKramer]

Either

import * as LoggingWinston from '@google-cloud/logging-winston';

const loggingWinston = new LoggingWinston.LoggingWinston({
	projectId: 'highlevel-backend',
	keyFilename: './lib/highlevel-backend.json',
	serviceContext: {
		service: process.env.GAE_SERVICE,
		version: process.env.GAE_VERSION,
	},
});

or

import {LoggingWinston} from '@google-cloud/logging-winston';

const loggingWinston = new LoggingWinston({
	projectId: 'highlevel-backend',
	keyFilename: './lib/highlevel-backend.json',
	serviceContext: {
		service: process.env.GAE_SERVICE,
		version: process.env.GAE_VERSION,
	},
});

should work. Let me know if it doesn't.

[shaunc869]

Awesome first one totally worked, now I threw in a test method:

export async function logTest(req, res) {
	req.log.info('this is an info log message');
	res.status(200).send('Hello');
}

It runs fine, but nothing prints to the console on my localhost, should it?

[ofrobots]

@shaunc869 Yeah, that is a problem – the express middleware configures only a stackdriver transport for winston. This is a bug.

However, in order to keep the discussion focussed, I am going to open a new bug rather than continuing to work on this closed issue: #303