Use LoggerMessage in HttpAbstractions #33927
Conversation
pranavkm
requested review from
BrennanConroy,
javiercn and
Tratcher
as code owners
ghost
added
the
pranavkm
force-pushed
the
prkrishn/use-logger-message
branch
from
631bf68 to
48d731b
Compare
| private static partial class Log | ||
| { | ||
| [LoggerMessage(1, LogLevel.Debug, | ||
| "Request successfully matched the route with name '{RouteName}' and template '{RouteTemplate}'", | ||
| EventName = "RequestMatchedRoute")] | ||
| public static partial void RequestMatchedRoute(ILogger logger, string? routeName, string? routeTemplate); | ||
| } |
There was a problem hiding this comment.
I don't think we should be duplicating private log types. If logging is shared by multiple types, then a non-nested internal static class is better.
There was a problem hiding this comment.
I realize I'm slightly contradicting what I said earlier here: https://github.com/dotnet/aspnetcore/pull/33927/files#r660733847, but if we were to write the code from scratch, it's very unlikely we'll decide to try and look for ways to commonize logging statements. I think it's much more intuitive to pick a pattern (use private log types) and use it consistently. We can definitely have outs for when the log has to perform a bunch of work, but for a simple one like this, it just seems like more work to try and de-dupe.
There was a problem hiding this comment.
I disagree. Splitting log types up for a single ILogger isn't a good idea. It makes it easy to unknowingly introduce duplicate IDs.
There was a problem hiding this comment.
Splitting log types up for a single ILogger isn't a good idea
Ok, I'm confused. I thought these were two different types that happened to be using the same logger message because they both wanted to do the same thing i.e. TreeRouter and RouterBase both happened to want to log the same message and so used the same LoggerMessage.
If that is the case (which I think it is), consider writing a new FooRouter that needs to log. Do we trawl thru all of the existing logger extensions to see if there's an existing method that's a good fit? The source generator will complain if you re-use event ids. Also what happens when FooRouter wants to log something that doesn't already exist? Do we put it in the shared static class so that the analyzer can ensure event-ids don't get duplicated?
There was a problem hiding this comment.
I took a look at what exactly is happening with these types and their loggers and saw they have distinct category names. In that case, I think splitting them up like this is fine. My concern was they shared a name but that isn't the case.
Here is my thinking on private Log types:
If the ILogger instance is private to a type (i.e. the type is passed an ILoggerFactory and creates its own logger using its type name) then it makes sense that its Log type is nested and private. If that isn't the case - the logger instance is shared by multiple classes - then a regular static class that can be shared by all the types that use the logger is better.
What do you think?
There was a problem hiding this comment.
What do you think?
Sure, that seems reasonable. Are there actual examples of this? I'm mostly familiar with MVC doing the former (re-using the same message across different logger categories), but not the latter.
pranavkm
force-pushed
the
prkrishn/use-logger-message
branch
from
48d731b to
7cfff88
Compare
| private readonly static Func<IFeatureCollection, IHttpResponseFeature?> _nullResponseFeature = f => null; | ||
| private readonly static Func<IFeatureCollection, IHttpResponseBodyFeature?> _nullResponseBodyFeature = f => null; | ||
| private readonly static Func<IFeatureCollection, IResponseCookiesFeature?> _newResponseCookiesFeature = f => new ResponseCookiesFeature(f); | ||
| private static readonly Func<IFeatureCollection, IHttpResponseFeature?> _nullResponseFeature = f => null; |
There was a problem hiding this comment.
That's pretty pedantic, I like it.
There was a problem hiding this comment.
|
This is updated. Could I have someone take another look? |
amcasey
added
area-networking
No description provided.