pinojs / pino-http

🌲 high-speed HTTP logger for Node.js
MIT License
539 stars 116 forks source link

The reqId type is overly-broad #268

Open jamiehodge opened 1 year ago

jamiehodge commented 1 year ago

The patching of the http IncomingMessage to add the id properly is arguably bad, but that discussion aside, reqId is too broad. The inclusion of number and object seems odd, considering that this value should be serialisable as a string in a header.

mcollina commented 1 year ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

marcbachmann commented 1 year ago

The default numbering could also use a change. As hint: crypto.randomUUID could be good enough on node 16 or newer.

Otherwise use https://www.npmjs.com/package/hyperid or apply the same concept with a custom function to have zero additional dependencies.

const {randomBytes} = require('crypto')

function createHyperId (bytes = 16) {
  let count = 1
  let id = randomBytes(bytes).toString('hex')

  function hyperid () {
    if (count < 1000000) return `${id}-${count++}`
    return `${id = randomBytes(bytes).toString('hex')}-${count = 1, count++}`
  }

  return hyperid
}

module.exports = createHyperId
marcbachmann commented 1 year ago

Here's a PR for the string based request id generation: https://github.com/pinojs/pino-http/issues/268 There's no type change in there.