Add support for connection scopes if logging is enabled #1953
Conversation
- Don't create a scope if logging isn't on - Copied the pattern we use in Hosting
|
Should we have a scope per request as well? |
|
Hosting creates one right? |
| return null; | ||
| } | ||
|
|
||
| private class ConnectionScope : IReadOnlyList<KeyValuePair<string, object>> |
There was a problem hiding this comment.
Why? It's a private class literally only used in this file.
There was a problem hiding this comment.
That's also true for all the dozens of what used to be private classes we moved to their own files in Kestrel. FrameConnection is already pretty big.
| var adaptedPipelineTask = Task.CompletedTask; | ||
| var input = _context.Input.Reader; | ||
| var output = _context.Output; | ||
| Log.ConnectionStart(ConnectionId); |
There was a problem hiding this comment.
Nit. No need for newline after try {
| { | ||
| _cachedToString = string.Format( | ||
| CultureInfo.InvariantCulture, | ||
| "ConnectionId:{0}", |
There was a problem hiding this comment.
Should we remove @"Connection id ""{ConnectionId}"" from KestrelTrace and LibuvTrace for logs we no we make from the async local scope? It seems a bit redundant if we don't remove it, but it could lead to a worse experience with loggers that don't support scopes.
There was a problem hiding this comment.
Also now that we have this should we log any extra metadata about the connection (e.g. remote IP) when a connection starts?
There was a problem hiding this comment.
How well do our own loggers support scopes? e.g. console, Azure app services, etc. If these don't easily display the name of the scope along with the message, we should keep the connection id in the message for now.
There was a problem hiding this comment.
@natemcmaster That's what I was concerned about. Assuming this is for 2.1, maybe we should still do this and add scope support to our loggers.
There was a problem hiding this comment.
I think we should keep it in the logs.
Questions:
Before:
After:
#640