Connection logging: enhancements! by NickCraver · Pull Request #2019 · StackExchange/StackExchange.Redis
| } | ||
| } | ||
| } | ||
| public void WriteLine(string prefix, string message) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider an int indent instead of string prefix?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these Format.ToString() calls necessary? It looks like ServerEndPoint already overrides ToString to do that
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.