Open theo-m opened 2 weeks ago
Please provide a minimal reproducible example. Doing so will help us diagnose your issue. It should be the bare minimum code needed to trigger the issue, and easily runnable without any changes or extra code.
You may use a GitHub repository to host the code if it is too much to fit in a code block (or two).
I'm not sure I understand, the first two line of the issue read like this:
Reproduction https://github.com/theo-m/repro-pino-http-error
You'll find a minimal reproducible example in that repo
You'll find a minimal reproducible example in that repo
I would hardly consider that minimal, given the build step, but I was able to reproduce anyway.
I've checked the example and pino-http behaves as expected. The "hacky" fix is indeed the correct one.
However, I think we could provide an express-specific variant that where the middleware intercept the first, error
argument:
https://github.com/pinojs/pino-http/blob/a4b581a0e1fe3b5ef9013c404bc9ceec66c04c46/logger.js#L89-L93
In this way we can intercept the error as it's passing through the middleware chain.
Would you like to send a PR?
I would hardly consider that minimal, given the build step, but I was able to reproduce anyway.
Not to stray from the issue, but having a build step seems inevitable since pino-http has typescript support? The docs do mention esbuild support too and suggest using the plugin that's setup in the repro.
It seems to me the repro is quite "portable" and has two commands that would run without additional setup, what would you guys expect?
I'd gladly help with the PR, but I'm not sure what's entailed by an express-specific variant
, what user API do you have in mind?
For me, I want a JavaScript reproduction. I have updated my saved reply to indicate as much as a result of this thread. But I also just didn't see the link originally. I had skimmed the report enough to recognize that we'd need one for clarity, but I missed the link. π€·ββοΈ
A middleware like:
const result = (req, res, next) => {
return loggingMiddleware(logger, req, res, next)
}
result.logger = logger
result.express = (err, req, res, next) => {
res.err = err
return loggingMiddleware(logger, req, res, next)
}
return result
Reproduction
https://github.com/theo-m/repro-pino-http-error
You'll observe that express does send the responses appropriately with the right errors - but, what's logged by Pino is entirely different.
What the client receives (π )
What the logger emits (π )
I had initially submitted this problem to Sentry as I had messed up my repro and the logger's stack trace pointed to Sentry/OTel - but even without these packages the problem persists.
Looking at this part of the lib, I can get what I want with this patch, but it seems hacky?
It seems this line expects an event, but it doesn't seem to exist on node's Response or OutgoingMessage objects?