Skip to content
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

this.log should be replaced with req.log #74

Open
GuskiS opened this issue Nov 26, 2019 · 10 comments
Open

this.log should be replaced with req.log #74

GuskiS opened this issue Nov 26, 2019 · 10 comments

Comments

@GuskiS
Copy link

GuskiS commented Nov 26, 2019

I tried this:

req.log = req.log.child({ something: true });

but it didn't work. After digging into sourcecode I found this:

var log = this.log

this refers to res object, so that would mean res.log. After changing that code, something appeared in my output.

This is very misleading since all examples suggest to use req.log. Also there isn't res.log doesn't have typescript type definition.

@mcollina
Copy link
Member

Can you add an example to reproduce the problem you are facing?

@GuskiS
Copy link
Author

GuskiS commented Nov 27, 2019

I have something like this:

router.use(pinoHttp(), (req, res, next) => {
  req.log = req.log.child({  something: true })
  next();
});

router.get("/example", (req, res) => {
  req.log.info("some message")
  res.json({ ok: true });
});

req.log.info("some message") will have something: true in its output, however, the request completed will not have it, because that one relays on res.

I hope this make it more clear.

@davidmarkclements
Copy link
Member

if you're always binding the same value, this isn't a good pattern, instead do

const logger = require('pino')().child({ something: true })
const pinoHttp = require('pino-http')({ logger })
router.use(pinoHttp)

@davidmarkclements
Copy link
Member

davidmarkclements commented Nov 27, 2019

for injecting values on a per request basis use the serializers

@GuskiS
Copy link
Author

GuskiS commented Nov 27, 2019

Value changes per request. I tried setting that value on req object, however that didn't show up in serializers from req.raw.

@mcollina
Copy link
Member

I think it would be helpful to have a full example of what you are trying to do.

@GuskiS
Copy link
Author

GuskiS commented Nov 29, 2019

#74 (comment)
This is almost the same thing as I'm doing, but here goes:
1.

app.use(pinoHttp());

const originalRouteMiddleware = (originalRoute) => (req, res, next) => {
  req.log = req.log.child({ originalRoute });
  next();
});
app.use(pinoHttp({
  serializers: {
    req(req) {
      req.originalRoute = req.raw.originalRoute;
      return req;
    },
  },
}));

const originalRouteMiddleware = (originalRoute) => (req, res, next) => {
  req.originalRoute = originalRoute;
  next();
});

Route:

router.get("/user/:userId", originalRouteMiddleware("/user/:userId"), (req, res) => {
  req.log.info("some message");
  res.json({ ok: true });
});

So the issues I'm facing:

  1. I would need to do req.log = res.log = res.log.child({ originalRoute }); to make it work, which is misleading and not obvious if you don't take a look at sourcecode. Also, as suggested, not a good pattern.
  2. In this case req.raw doesn't contain my originalRoute, not sure why tho. Maybe .child creates copy of original object? 🤔

@mcollina
Copy link
Member

May I get a full example that I can run? Ideally without express (or maybe this is express-only).

@GuskiS
Copy link
Author

GuskiS commented Nov 29, 2019

@mcollina
Copy link
Member

In this case req.raw doesn't contain my originalRoute, not sure why tho. Maybe .child creates copy of original object?

Child serializes the request object once at the time the log line is created. In order for that property to show up, you'd have to add it in a middleware before the pino-http one.

I do not think there is any other way than req.log = res.log = res.log.child({ originalRoute }); tbh.
You could potentially disable autoLogging and log after you have set that value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants