Feature/enhance logging #51
Conversation
|
If we do want to add more detailed logging the correct approach is to use ILogger from Microsoft.Extensions.Logging.Abstractions, which provides a full set of log levels (info, warn, etc.) and allows clients to plug in any standardized logging mechanism, e.g. NLog. I originally didn't want to do that as I prefer to minimize dependencies, but I'm okay adding it if there's a need. I would prefer it to be separate PR-wise. |
|
Ironically I did use that package for my use but pulled that out for your PR. I’ll revisit that next week. I kinda liked this approach because it doesn’t force another dependency and because people may choose to not hook up trace.
|
|
And now I’m seeing there’s probably a reason why no one has yet used the ILogger interface in this project up to this point. There’s zero DI and zero container reference. Basically it needs next to no configuration (which is refreshing in its own way) and therefore the classes are not currently interfaced. What I had done locally when I brought in Microsoft.Extensions.Logger.Abstractions package was the minimal - which was to leverage the LogLevel enum as a second parameter to the log action so it could log at the appropriate level in whatever logging backend was actually being used. And arguably this would be a perfectly reasonable way to keep configuration simple and unopnionated. It doesn’t give us the filtering of ILogger - but that probably doesn’t matter here - curious your opinion on that. In fact if we are to go full ILogger it gets a little more complex, and IMO goofy. My usual approach is to go full on with constructor injection, interfaces for everything necessary, etc. I usually don’t have need to even accept an ILoggerFactory because of said interfaces, because I’m just using the abstraction, don’t mix dependencies and configuration in constructors, and use a totally different backend (usually Serilog-based). If we did that though, we would need a LibraryConfiguration class that would be required to be used to handle the IOC. And this feels like the wrong approach for a nuget package. Alternatively we can avoid all the interfacing and DI shuffling by receiving an optional ILoggerFactory at the client and server constructor entrypoints, and pass that thru the constructor call chains as needed (which feels like a SRP violation to put logger initialization down in the consumers). I am not sure the performance hit of calling CreateLogger in all the various places to filter appropriately, but its not going to be free. IMO just plain unfiltered ILogger would be clean and sufficient…and would not need but one factory call. Just requires consumer to pass a factory, which is standard although a little obnoxious if you didn’t need a factory until this point. Fourth option which I just stumbled across - I see Grpc.Core.Logging has an ILogger….with an ILogger ForType(); constructor… It seems to me any of these are valid approaches, each with their expectation put on the consumer of resolving dependencies, from none, to a full LoggerFactory. I want to hear your input before I proceed. |
|
@cyanfish I went ahead and hammered out a branch with the ILogger. If you want it I'll follow thru with a PR. Personally, I don't like it for the following reasons:
|
@cyanfish when debugging my load testing, I found that I needed to enhance the logging and make it accessible.
Please consider this PR, which separates a trace log action from an error log action as two separate targets that can be wired up as the caller needs. It also breaks out the four cases of error in your error handling to provide more specific information.
This as-seen builds up on the other work I did for FIFO and pool size options. If you are okay with this, I'd say merge this and abandon the other PR.