Connection logging: enhancements! #2019
Conversation
| } | ||
| } | ||
| } | ||
| public void WriteLine(string prefix, string message) |
There was a problem hiding this comment.
Consider an int indent instead of string prefix?
There was a problem hiding this comment.
We could, but overall it'd be doing a string creation and allocation that way so kept it simple :)
| foreach (var ex in aex.InnerExceptions) | ||
| { | ||
| log?.WriteLine($"{Format.ToString(server)}: Faulted: {ex.Message}"); | ||
| log?.WriteLine($" {Format.ToString(server)}: Faulted: {ex.Message}"); |
There was a problem hiding this comment.
Are these Format.ToString() calls necessary? It looks like ServerEndPoint already overrides ToString to do that
There was a problem hiding this comment.
Good question - I would like to change these everywhere but going consistent for this PR and want to do those all at once :)
|
|
||
| await Maintenance.ServerMaintenanceEvent.AddListenersAsync(muxer, logProxy).ForAwait(); | ||
|
|
||
| logProxy?.WriteLine($"Total connect time: {sw.ElapsedMilliseconds:n0} ms"); |
There was a problem hiding this comment.
Is there any value in knowing how long it took to fail to connect? If so, you could move these "Total connect time" lines into the finally
There was a problem hiding this comment.
I debated that as well - I feel like that might be confusing to give a total time when it just blew sky high for no reason - in those cases we care more about the log of what went sky high or what was the last thing logged, rather than the time...I think.
This tweaks logging a bit to make it easier to parse and switches timings to
ValueStopwatchwhile I'm in here adding one. Helps investigate things like #2017.