-
Notifications
You must be signed in to change notification settings - Fork 1
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
IMN 473 - Replace express winston middleware #434
Conversation
dd7efdc
to
b0f66ab
Compare
94a4646
to
dc7a122
Compare
b0f66ab
to
623a280
Compare
loggerInstance.info(`Request ${req.method} ${req.url}`); | ||
res.on("finish", () => { | ||
loggerInstance.info(`Response ${res.statusCode} ${res.statusMessage}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we have to decide what we want to log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we log the same info that we were logging with the previous middleware?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old configuration was temporary so we could decide here the final version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few considerations:
- I like the idea of having one log before and one after the call, it's easier to read. On the other side, this would reflect in a 50% log increase for the GETs, which are the most frequent called endpoints. Since Cloudwatch costs are not negligible, and considering that the double log does not actually add information, I would keep only the log at the request completion
- considering that the logger is already adding all context details, we can keep the previous log format
Request {method} {urlWithParameters} - Response {code}
Notes:- we need to include query and path parameters but not elements in the body, which can be sensitive
- the previous log includes also the response status message. It does not actually add any information and we could remove it, but I've not strong opinions on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as long as we log the same info we were logging with the previous solution (or agree on a new set of info to log)
export function loggerMiddleware(serviceName: string) { | ||
return (req: any, res: any, next: express.NextFunction): void => { | ||
const loggerMetadata: LoggerMetadata = { | ||
serviceName, | ||
userId: req.ctx?.authData?.userId, | ||
organizationId: req.ctx?.authData?.organizationId, | ||
correlationId: req.ctx?.correlationId, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove the any
s?
My proposal:
export function loggerMiddleware(serviceName: string) { | |
return (req: any, res: any, next: express.NextFunction): void => { | |
const loggerMetadata: LoggerMetadata = { | |
serviceName, | |
userId: req.ctx?.authData?.userId, | |
organizationId: req.ctx?.authData?.organizationId, | |
correlationId: req.ctx?.correlationId, | |
}; | |
export function loggerMiddleware(serviceName: string): express.RequestHandler { | |
return (req, res, next): void => { | |
const context = (req as express.Request & { ctx?: AppContext }).ctx; | |
const loggerMetadata: LoggerMetadata = { | |
serviceName, | |
userId: context?.authData?.userId, | |
organizationId: context?.authData?.organizationId, | |
correlationId: context?.correlationId, | |
}; |
This way, if the AppContext
changes we would be "notified" by typescript at compile time
Re-add the logger middleware removed here #423
NOTE:
With this new logger middleware, we're unable to add a log to the /health request
NEW format