feat(middleware-correlationid): added ability for custom logger

[lkhari]

What did you implement:

I've added the ability to add custom loggers to the correlationId middleware Closes #98 Blocked by https://github.com/getndazn/dazn-lambda-powertools/pull/220

How did you implement it:

I've added an optional argument for the middleware library to allow you to write your own correlation logger, with it defaulting to the powertool logger. There for not causing a breaking change.

How can we verify it:

Use the correlationId middleware library and add the constructLoggerFn with your own custom logger.

Todos:

• [x] Write tests
• [x] Write documentation
• [ ] Fix linting errors
• [x] Make sure code coverage hasn't dropped
• [ ] Provide verification config / projects / resources
• [x] Enable "Allow edits from maintainers" for this PR
• [x] Update the messages below

Is this ready for review?: Yes Is it a breaking change?: NO

[theburningmonk]

@lkhari can you explain the use case here? why only add custom logger constructor in the correlation IDs middleware, that seems like a really odd thing to do. If you want to support a customer logger, wouldn't it be better to add that support directly to the logger module so that it can proxy all the requests to your customer logger?

[lkhari]

To be honest, the use case came from https://github.com/getndazn/dazn-lambda-powertools/pull/220 PR, when I wanted to expose the log library. And I interpreted an issues that middleware-correlationId was a little too coupled to the logger.

This PR is really just a first stepping stone in decoupling the two libraries and allowing other people to use their own Logger.

[theburningmonk]

The fact that they are tightly coupled is by design - in the same way that iPhone, mac and airpods are design to work great together even if they're not necessary so good with other devices. It's even in our readme An integrated suite of powertools for Lambda functions that .... The philosophy for this project was always "use our modules together and everything just works" and while some extensibility would be nice, extensibility should not impede on that.

[simontabor]

I quite like this approach as it will allow us to attach our own loggers to the context (which is where I want them), without touching the internal logs written by powertools (which would be a right pain to modify)

At DAZN, we use our own logger (not powertools) and currently have a middy middleware which effectively reproduces the logger functionality of middleware-correlation-ids by looping over all records and attaching the correct logger to them, and to the context. Being able to avoid this would be a quick win for us.

I need to think about this some more, but I think trying to patch in the whole powertools-logger with a custom logger would be annoyingly complex, so limiting it to the loggers which are exposed by powertools to the application (ie, this middleware) is a good middle-ground

[theburningmonk]

@simontabor why do you think injecting a custom logger through the powertools logger would be annoyingly complex? Making that the injection point would make it work for all the middleware in this suite and not just correlation IDs.

[lkhari]

I can see the benefits of both sides. I like that the idea of proxy to encourage these good practices, but it could be problematic when a dev wants to use a function not exposed by our proxy-logger, which could then lead to this repo getting a lot more request for more functionality.

Turning the logger into a proxy, seems a bit wrong, since it's title is powertool-logger not a proxy for another logger. Although you can say it's already a proxy for console.log, other loggers like Winston just classifies console.log as a type of transport.

Regardless of the idea to make powertool-logger a proxy or not. Encouraging modularity is usually a positive if done well. This current PR doesn't dissuade people to using the powertool-logger, because it's there by default, and adds the ability to change/remove the logger which is optional.

It could be argued, from another point of view, that adding the logger to the Records, is extra functionality from the automatically extract correlation IDs from the invocation event. i.e. not really single responsibility and maybe shouldn't be there at all, since people already using their own logger may think it's just wasting memory. To be honest with the point I've just written, I could see a comment of improvement in this code would be to actually lazy load the default logger.

[theburningmonk]

@lkhari but it could be problematic when a dev wants to use a function not exposed by our proxy-logger - based on the change you're making here, the goal is to allow the middleware to call the custom logger, right? If that's the case, those calls won't be using any custom logic in your custom logger anyway. And outside of the middlewares in this suite of tools, you can still use your own logger however you'd like.

Turning the logger into a proxy, seems a bit wrong - as you said, console.log is just a transport, it just so happens to be the only one right now. If the semantic of proxying calls is the problem, then we can call them transports instead and create a well-defined interface, but ultimately it's just some API that our logger calls when it needs to log a message.

[lkhari]

based on the change you're making here, the goal is to allow the middleware to call the custom logger, right? If that's the case, those calls won't be using any custom logic in your custom logger anyway. And outside of the middlewares in this suite of tools, you can still use your own logger however you'd like.

Sorry, my PR description was a bit lacking. The PR goal is to allow the middleware to attach a custom logger to the Records coming in from SQS, Kinesis, etc. It doesn't allow the middleware to call the customer logger but it takes in a function which allows the dev to write the way their logger is constructed.

A good example is wanting to use Winston over powertool-logger, for it's format capablilties or extra log levels like verbose and http. Allowing me to do:

middy((event)=>{
   event.Records[0].logger.profile('test'); // unsupported by powertool
   event.Records[0].logger.verbose('This is a log level not supported by powertool logger');
   event.Records[0].logger.profile('test')
})
.use(captureCorrelationIds({
      sampleDebugLogRate: 0, (correlationId)=>new LoggerThatTakesInCorrelationID(correlationId)
    }))

Once #220 is merged this PR would be a lot more clear.

You are right, we can still use our own logger however we'd like, without this PR. But we are also enforcing that the users also has our logger attached to their records. Which seems like unnecessary bloat.

[simontabor]

@simontabor why do you think injecting a custom logger through the powertools logger would be annoyingly complex? Making that the injection point would make it work for all the middleware in this suite and not just correlation IDs.

Because whatever logging interface is defined by powertools will leak across the entire user's application. Basically, what @lkhari said!

Let's say, for example, I want to use Winston as my logger. Powertools then decides "ok I'll let you set a logger, just give me a function which accepts (level, message, params)". Great!

However, now I'm restricted to using the proxied interface, especially when using record.logger attached by the correlationIds middleware. I can't use any of the additional features which Winston, or any other logger, provides.

The only real limitation that this introduces is that powertools can't write any logs internally (it doesn't do this at the moment anyway) without having a proxied interface defined to ensure compatibility.

EDIT: powertools does write some logs internally: https://github.com/getndazn/dazn-lambda-powertools/blob/e9a35c45f10d58364c3bd3d2b89a63ccebc752a0/packages/lambda-powertools-middleware-correlation-ids/event-sources/firehose.js#L51

I think we should attach the "root" logger to the main Lambda context to further support this, ie context.logger = constructLoggerFn(correlationIds)