Optional parameters for the middleware

[erickhun]

Allow to pass optional parameters to the middleware.
For now, only allow logger and customLogLevel option

// eg: a custom logger: 
var pino = require('pino');
 app.use(BuffLog.middleware({'logger': pino()}))

// customLevels
var myLevels = function (res: any, err: any) {
            if (res.statusCode >= 400 && res.statusCode < 500) {
                return 'notice'
            } else if (res.statusCode >= 500 || err) {
                return 'critical'
            }
            return 'debug'
        }

 app.use(BuffLog.middleware({'customLogLevel': myLevels )}))

// or both : 
 app.use(BuffLog.middleware({'customLogLevel': myLevels, 'logger': pino() )}))

What I'm unsure:

Here my non-js expertise that talks :

For the others options, I didn't do it (yet) since it feels like copy pasting the code of the pino-http itself .

I'm wondering if there is not a way to not copy paste, but get the pino-http defaults and then forward them ?

Thoughts @esclapes @colinscape ?

[colinscape]

I'm not entirely clear what problem we're trying to solve here with this passing of arguments to the logger. From my (naive!) perspective, the less customisation for the logger the better - that way we can be confident in our logs having a standard format. If one service starts logging 4xx as error and another as info, it becomes difficult to reason about.

I like having the middleware be as simple as something like app.use(bufflog.middleware) or however it might look, and this provides a standard, opinionated, set of logs for all our node services. There's nothing to stop the service adding additional info or debug logs in addition to the standard middleware, using the 'regular' BuffLog interface.

Maybe I'm missing a conversation or some requirement though?

[esclapes]

Hey @erickhun, thanks for this PR 🌮

For the others options, I didn't do it (yet) since it feels like copy pasting the code of the pino-http itself.

I would go with the simplest approach and just do { ...defaults, ...options }, without worrying about defaults or checking what is passed. I would expect the usage of custom options to follow docs and be tested properly so I wouldn't add too much logic to handle it.

I like having the middleware be as simple as something like app.use(bufflog.middleware) or however it might look, and this provides a standard, opinionated, set of logs for all our node services.

@colinscape I fully agree and this PR wouldn't change that. It would still be simple and opinionated by default. However, I feel that exposing the underlying options gives us some extra features for free when/if we need them.

If one service starts logging 4xx as error and another as info, it becomes difficult to reason about.

Yeah, I agree that it could lead to some inconsistencies, which I'd expect to be justified. For instance, it may be useful to log the 403 responses (rate limited) for the buffer-login service.

I wouldn't ask for custom features to the logger before they are needed, but I wouldn't hide existing features either.

[colinscape]

exposing the underlying options gives us some extra features for free when/if we need them

I feel this starts to tie us to the implementation, which I'd rather we didn't do. I like how simple the log library is 'out of the box' and I'm loathe to add additional complexity for the sake of potential future need. YAGNI and all that.

it may be useful to log the 403 responses (rate limited) for the buffer-login service

For sure, and I think logging these kind of special cases explicitly with a bufflog.notice() call should work, without having to create a special custom logging level function to achieve it.

From my point of view, the interface of the Buffer Logger is this:

bufflog.debug('hello critical', {"some":"stuff"});
bufflog.info('hello info');
bufflog.notice('hello notice with context', {"foo":"bar"});
bufflog.error('hello error');
bufflog.critical('hello critical');

plus the helper method bufflog.middleware().

I now realise we added exposing the internal logger recently which I'm reconsidering - I don't think I really understood exactly what was happening there since while it does add flexibility it now means we can't really change the implementation and as a result we haven't abstracted the interface sufficiently.

[esclapes]

Thanks a lot for sharing your thoughts @colinscape

I feel this starts to tie us to the implementation, which I'd rather we didn't do.

I guess it's sort of a YAGNI vs NIH in this case 😄

I am probably biased here because I proposed moving away from our own logger wrappers a year ago 😅 Back then, after adding a few custom features to buffer-js-logger, I felt we were still missing features that were common in open source options. I still lean towards not re-inventing something like a logger library.

However, I did not have the focus and bandwidth to move forward with that proposal. @erickhun and you are doing a much better job at standardizing our logging approach and the last thing I want is to provide any kind stop energy.

This project clearly owned by Infrastructure, so I am happy with whatever direction feels better to you. Let me know how I can help 🙌

[colinscape]

@esclapes - thanks so much! I realise we (well, me!) are being quite opinionated here, and that's a deliberate focus in order to strive for standardisation and a simple, generic interface for all our logs across multiple languages & frameworks. That isn't to say we won't revisit this later on as we learn what works and what doesn't! You have such a breadth of knowledge on all this, I'm sure we'll come knocking to your door once more!

You've contributed a lot to this project already - with your work last year and introducing pino to us all, which by all accounts is working very well with all the work we are giving it! Not to mention the time and effort on the reviews over the past few months! 🌮 🙌