Open zachrybaker opened 10 months ago
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
In fact if we are to go full ILogger
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
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
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.