-
Notifications
You must be signed in to change notification settings - Fork 209
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Allow an ILogger to be specified #183
Conversation
/// The logger through which request completion events will be logged. The default is to use the | ||
/// static <see cref="Log"/> class. | ||
/// </summary> | ||
public ILogger Logger { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth to use DI provided features. Why to manually specify logger if it is already exists ? Just add new ctor param:
public RequestLoggingMiddleware(RequestDelegate next, DiagnosticContext diagnosticContext, RequestLoggingOptions options, ILogger logger)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sungam3r - what about if serilog ILogger is not registered for DI and the developer is only using Log.Logger
everywhere - which i may be wrong, but is a valid style for using serilog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, then we can use second RequestLoggingMiddleware ctor with ILogger property inside options to satisfy both categories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @sungam3r - will give it some thought 👍
Any prediction when this might be merged? Thanks. |
@pnmcosta still needs some thought, sorry. I like the idea of accepting the logger as a constructor parameter, but I'm not sure whether this covers all scenarios 🤔 |
@nblumhardt my scenario is that we built a library that extends Serilog with our own conventions, and we expose our own singleton for the root logger, not relying on Serilog's. We also only leverage DI for MS's ILogger's, i'm not even sure if internally Serilog.Ilogger's are registered in the container, but in our case we don't need the Serilog.ILogger in the container. From a consuming point of view, I'd definitely be more inclined to set the logger on the options, unless HTH |
Resolves #182