-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] Request context not working when using FastifyAdapter #546
Comments
@itheodoro hi, have you tried to play with routing params
looks like the problem is that nest adapters use different routing libs, and it doesn't provide any clean way to detect which adapter is used currently to set proper default routing params, so here some basic routing params are set that works for both express and fastify in tests |
Hi @iamolegga, thanks for your reply. I tried to configure routing params to match all available routes and got the same results as before. Is this what you suggest or I misunderstood something?
With the above code, this log is also displayed without |
interesting, will look deeper into this |
Thanks! Let me know if I can help with something. Just another behavior that may help is that |
Hi! Do you have any updates regarding the issue? |
Sorry, not yet. Not sure if will do in the nearest future, but anyone may feel free to investigate and fix this |
It's looks like there is a bug in latest versions of @nestjs/platform-fastify 8.1.2. The Middleware from module do not fire. in main.ts you can add global middleware and use pino from Fastify. import {storage, Store} from 'nestjs-pino/storage'
import pino from 'pino';
async function bootstrap() {
const logger = pino({
level: process.env.NODE_ENV !== 'production' ? 'debug' : 'info',
})
const app = await NestFactory.create<NestFastifyApplication>(ApplicationModule, new FastifyAdapter({
logger,
}), {
bufferLogs: true,
});
app.useLogger(app.get(Logger));
app.use((
req: any,
_:any,
next: ()=> void,
) => {
// @ts-ignore
storage.run(new Store(req.log), next);
});
await app.listen(PORT, '0.0.0.0');
}
bootstrap(); |
thanks @jasyes closing this as this is not a bug of the current lib, but will pin it in issues so everyone can see it. Please ping me here when this bug will be fixed. |
@jasyes have you created an issue in the nestjs repo? can you also post where the commit was reverted? |
The main issue was that different version of fastify installed as dependency of @nestjs/platform-fastify and as dependency for project. |
I just encountered this bug myself, thanks for pinning this issue! Looks like it's still not fixed upstream. |
Hey, you @tomtastico, @iamolegga, @SirReiva ! How to do that nestjs/nest#8809 (comment) Who undrestand how to reproduce that problem without any third-party libraries? |
@jmcdo29 thanks, but for that I needed to get a ban. These mens and womens can when want to do something. |
Based on @jmcdo29's investigation in this issue, it appears that the root cause may be because of how the FastifyRequest wraps the native Node IncomingRequest object. In order to access attach properties properly to request for Fastify, they should be retrieved from @iamolegga Does this make sense to you? Perhaps some changes need to be made to the middleware to adapt based on which adapters (Express vs Fastify) is in use? |
Another option, just as an FYI, would be to use an |
Thank you for the investigation. So, it looks like this is a bug in this library, not the framework. I'm a bit busy now (will have time next week) but if someone knows what to do, please open PR (with correct tests), I'll check it asap. |
Thanks, but I think it's enough of FYI's here, it won't help, only spam notifications, I'm already subscribed to all the related issues and watching the situation. It looks like we only need to change the |
If I get it right, Fastify v3.25.2 already solves the issue. Since we are using Fastify via platform-fastify, a new version of the package with updated Fastify version (>= v3.25.2) must solve the problem without the need of implementations here in nestjs-pino. Also, the need of a new version of platform-fastify was already pointed here. |
Closing this issue since |
Cool, @itheodoro have you tested this fix already? |
Yes, I've tested and it's working fine, @iamolegga 🎉 |
For anyone else who stumbles across this issue, I was still having issues with logs not having request context information, as shown in the OP. I finally tracked it down to having I was able to replicate this with a fresh NestJS project too, where simply having the Hope this helps someone else! |
@adworacz Literally life saver!!! Thanks |
[x] I've read the docs of nestjs-pino
[x] I've read the docs of pino
[x] I couldn't find the same open issue of nestjs-pino
What is the current behavior?
I'm using NestJS with Fastify.
nestjs-pino
works pretty well with HTTP GET requests and with other HTTP requests that doesn't have a body. When requests have a body, the logger does not brings context and only prints the basic log message, even when log is invoked inside the request context.What is the expected behavior?
When logging inside a request context, all log entries should have the correct context.
Please provide minimal example repo. Without it this issue will be closed
Example repo with log in app.controller.ts
Please mention other relevant information such as Node.js version and Operating System.
Node.js version: v14.17.6
OS: Ubuntu 20.04.3 LTS
nestjs-pino version: ^2.2.0
@nestjs/platform-fastify version: ^8.0.6
Important to mention that the wrong behavior doesn't occurs when using Nest with Express.
I also noticed a possible fix, but I don't know if it could cause some issue since I'm not very familiar with AsyncLocalStorage. The possible fix consists in replace AsyncLocalStorage
run
by AsyncLocalStorageenterWith
method in LoggerModule.Thanks for creating such a helpful package! 🙏
The text was updated successfully, but these errors were encountered: