options.getChildBindings remove default value to avoid duplicate req …

[indreek]

https://github.com/pinojs/hapi-pino/issues/91

Didn't update version. Also tests should be reviewed because in my opinion they don't do what they are meant to do.

//test 
experiment('ignore request.log logs for paths in ignorePaths', () => {
  test('when path matches entry in ignorePaths, nothing should be logged', async () => {

server.route({
      path: '/foo',
      method: 'GET',
      handler: (req, h) => {
        req.log([level], 'foo')
        return 'foo'
      }
})

//It didn't test against route, but 2 log entries are created. One with req.log and second is default route logging.

-expect(data.data).to.equal('foo')
+expect(data.data).to.equal('foo from handler')

[coveralls]

Coverage decreased (-0.2%) to 98.841% when pulling 82c40e8a2ee22e2882a7afbf37bdfd82550cccd1 on indreek:master into 9b84037e3140c5d642774480a5d596c8addc21db on pinojs:master.

[mcollina]

I don't really understand this fix.

See https://github.com/pinojs/hapi-pino/pull/87 for the full discussion on that option.

[indreek]

Idea behind this fix is that element req gets added twice to each log entry. 1s time in getChildBindings with default options. 2nd time in server.ext('onRequest', OR tryAddEvent(server, options, 'on', 'response'.

In my opinion we shouldn't add it twice because 1) We don't know what is the next processor, and what it does actually. Is it getting the correct element? 2) For example if I modify remote address (reverse proxy, etc), first req element is with default api provided, but 2nd one is correct. 3) If whole log is 1483 chars, then req element of it is 521. Over 1/3 is rubbish. 4) If you enable logRequestStart. Req element is in both log entries. From readme: Note: when logRequestStart is enabled and getChildBindings is configured to omit the req field, then the req field will be omitted from the "request complete" log event. This behavior is useful if you want to separate requests from responses

Indrek

[mcollina]

Why cannot just pass a getChildBindings() function that returns an empty object?

[indreek]

You can do this also, but I think that it should be default behaviour then.

On Fri, 15 Nov 2019, 10:07 Matteo Collina, notifications@github.com wrote:

Why cannot just pass a getChildBindings() function that returns an empty object?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pinojs/hapi-pino/pull/92?email_source=notifications&email_token=ABJZCJ6HJJD4PRZG2ADCHBDQTZRFJA5CNFSM4JM3RWTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEEZEUY#issuecomment-554275411, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJZCJ7F6QB5DZFMG52YS5TQTZRFJANCNFSM4JM3RWTA .