-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: add trace to logs #405
Conversation
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
@@ -1,5 +1,14 @@ | |||
import { Request } from 'express' | |||
|
|||
export const getRequestIp = (req: Request) => { | |||
interface IRequestWithId extends Request { |
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.
nit: Can we extend the namespace globally (following suggestions in this SO post, some may be outdated) so it's consistent throughout the application?
Can also be done in a different PR, since I think we need to extend request to also have req.session.user
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.
Ok! Will create issue for this and work on it
@@ -577,6 +577,7 @@ exports.getExampleFormsUsingAggregateCollection = function (req, res) { | |||
meta: { | |||
action: 'getExampleFormsUsingAggregateCollection', | |||
ip: getRequestIp(req), | |||
trace: getTrace(req), | |||
url: req.url, |
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.
Out of scope for this PR, but can you help create an issue for an utility function that returns the relevant log info from a req
? Seems like the ip, trace, url, header combo is repeated throughout the application :P
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.
Ok!
Problem
Implements tracing for (https://github.com/datagovsg/formsg-private/issues/25)
Solution
Add trace to logs using cloudflare cf-ray header. If cf-ray does not exist, trace using x-request-id header
Tests
On staging
On localhost
logging.ts
, replacewith
Access a public form. Check that the value of x-request-id header is logged under trace in the server logs for the get request.
New dependencies