-
Notifications
You must be signed in to change notification settings - Fork 435
Add Server logging #547
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
Add Server logging #547
Conversation
b987b3f to
1832cc3
Compare
MrMage
left a comment
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.
Looks good overall. A few notes:
- My recollection was that the client-side logging relied a bit more on "dependency injecting" loggers. This seems to be less the case on the server-side — any specific reasoning behind that?
- As with the client, would you mind providing a sample log dump?
- I have not checked the logs for completeness, i.e. whether all things that should be logged are logged.
| return | ||
| } | ||
|
|
||
| logger.info("received request head, configuring pipeline") |
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.
Should we try attaching , metadata: ["path": "\(requestHead.uri)"] to all log statements relevant to a specific call?
Also, should we have one dedicated canonical "access log" statement that serves a similar purpose as e.g. Apache/nginx access logs? Preferably including request path, response status, and the overall time the request handler took. (Let me know if I missed one you have for that.)
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.
Should we try attaching
, metadata: ["path": "\(requestHead.uri)"]to all log statements relevant to a specific call?
How about just a uuid so we can reuse the logger across different handlers?
It may be worth creating a CallHandlerContext and passing that to handleMethod and then the *CallHandlers (instead of the method name, request head, channel, error delegate, and logger) so we can avoid breaking the generated code so often.
Also, should we have one dedicated canonical "access log" statement that serves a similar purpose as e.g. Apache/nginx access logs? Preferably including request path, response status, and the overall time the request handler took. (Let me know if I missed one you have for that.)
We should!
Motivation: We only have client side logging at the moment, we should also have logs on the server. Modifications: Add logging for server components. Also add explicit 'self' in a number of places where it was missing. Result: More server logging.
1832cc3 to
f9d4710
Compare
|
|
MrMage
left a comment
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
Motivation:
We only have client side logging at the moment, we should also have logs
on the server.
Modifications:
Add logging for server components. Also add explicit 'self' in a number
of places where it was missing.
Result:
More server logging.