apply default serializer.req before using configured serializer.req

[benib]

This serializes the req using the internal asReqValue before passing the result to any configured serializer.req. This is to make it work in combination with pino-noir and a configuration redacting a value in the req object. e.g.

{
    plugin: require('hapi-pino'),
    options: {
      serializers: noir(['req.headers.authorization'])
    }
  }

It is inspired by the discussion in this issue: pinojs/express-pino-logger#14

This change is BC breaking as before any configured serializer got the original req object without being passed through asReqValue. One test case is changed to reflect this.

I am not sure if this makes sense. Another solution would be to do this within a function passed to
options.serializers.req directly. Although this would slightly complicate the usage of pino-noir together with this plugin.

I am also not sure if this should be done for res as well and if you would like to see this as copied code (just for res instead of req) or if you prefer to have it in a separate function called with res or req returning the function.

Let me know if I should change anything.

[coveralls]

Coverage decreased (-0.08%) to 98.229% when pulling 1fc78ce on benib:overriding-req-serializer into 4d67abf on pinojs:master.

[coveralls]

Coverage decreased (-0.08%) to 98.229% when pulling 1fc78ce on benib:overriding-req-serializer into 4d67abf on pinojs:master.

[jsumners]

LGTM

[coveralls]

Coverage increased (+0.06%) to 98.368% when pulling e4c3ece on benib:overriding-req-serializer into 4d67abf on pinojs:master.

[coveralls]

Coverage increased (+0.06%) to 98.368% when pulling e4c3ece on benib:overriding-req-serializer into 4d67abf on pinojs:master.

[felixheck]

Mhh..I don't know. This results in an issue that the serializes doesn't get the whole information which breaks existing serializers and doesn't feel like best practice.

Perhaps it's better so pass req and additionally asReqValue to the serializer so the dev can decide if he wants the whole request object or just the simplified:

options.serializers.req = (req, asReqValue) => (
  (options.serializers.req | asReqValue)(req, asReqValue)
)

[benib]

@felixheck Yes, this would break existing serializers that are implemented against the complete request object. This is a problem. I agree.

I'd rather export asReqValue from hapi-pino instead of passing it in though. pino-http does this here: https://github.com/pinojs/pino-http/blob/master/logger.js#L107-L110

As far as I know me as a dev has to apply asReqValue at some point though to stay compatible with https://github.com/pinojs/pino-toke#supports and probably others.
I do not have a lot of experience with pino, so I am not sure what the strategy about keeping these serializers "compatible" as in producing the same req log object. But I guess this should be synced with pinojs/express-pino-logger#14 and others somehow.

I am happy to help with any implementation but would rather leave the decision to people who have more experience with pino and these http-style serializers as it can have quite some implications for the future.

[felixheck]

Mhh..yeah, exposing asReqValue seems to be the smartest solution. At least in my point of view.

[benib]

@felixheck so should we go for not overriding but exposing asReqValue and documenting that one should use asReqValue in any custom serializer passed to options.serializers.req? I would send another PR in favor of this one then.
Any opinion from @davidmarkclements? The implementation in this PR is based on your comment here: pinojs/express-pino-logger#14 and handling this in the same way would make a lot of sense imo.

[felixheck]

@mcollina has the last word ;-)

[jsumners]

oriqReqSerializer => origReqSerializer

[jsumners]

How about attaching req.raw as a non-enumerable property when invoking the original serializer?

[coveralls]

Coverage increased (+0.06%) to 98.368% when pulling d5fdeef on benib:overriding-req-serializer into 4d67abf on pinojs:master.

[coveralls]

Coverage increased (+0.06%) to 98.368% when pulling d5fdeef on benib:overriding-req-serializer into 4d67abf on pinojs:master.

[jsumners]

See pinojs/pino-http#43

[benib]

@jsumners the plan is that hapi-pino could or should wrap around pino-http?

[jsumners]

@benib unfortunately I don't think so. But I believe whatever solution we settle upon there should be used here.

[davidmarkclements]

hey guys thanks for all the input here!

@benib - pino-http is the basis for express and koa loggers, but Hapi is too different to integrate with efficiently so it has it's own implementation

@felixheck - the full req serialization is a bug that occurs when adding multiple serializers, which is unfortunate. However it doesn't happen in the main case. To retain the full request behaviour we simply provide a new serializer, let's not change the serializer format as this is more disruptive.

Regarding how we collaborate on Pino and Pino's ecosystem:

@mcollina and I wrote pino core together, @mcollina wrote hapi-pino, I wrote pino-http (express-pino-logger, koa-pino-logger, ...) and as in pretty much all cases when there's a hot topic, the "last word" tends to come from agreement between @mcollina, @jsumners and myself. In this particular case I trust @jsumners judgement (as does @mcollina) on which way to take pino-http, which should tease out a direction for this issue.

[benib]

thanks for all the explanations. I will leave this PR open and close it once I come up with one fixing the issue along the lines of pino-http once pinojs/pino-http#43 is settled. Or anyone else picks this up. No hard feelings here.

[davidmarkclements]

thank you @benib !

[jsumners]

pino-http has been updated to v3.0.0 which solves this same problem. I recommend looking at how it was solved there and applying that solution to this PR.

[mcollina]

@benib would you mind updating this PR?

[benib]

@mcollina sorry was pretty overwhelmed with work the last days. will try to get to that before the end of the year. If someone has the resources to take over, feel free.

[benib]

closed in favor of: #42