-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Supply services when configuring Serilog inline #20
Conversation
|
||
ConfigureServices(collection, logger); | ||
ConfigureServices(collection, null); |
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.
Why is there now null
?
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 note, @sungam3r - it's because ILogger
is already registered lazily above (line 156); I've added a comment 👍
@@ -26,16 +26,27 @@ namespace Serilog | |||
/// </summary> | |||
public static class SerilogHostBuilderExtensions | |||
{ | |||
// Used internally to pass information through the container. | |||
class RegisteredLogger |
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 don’t understand why this hack is needed. Could you explain please?
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,
ILogger
needs to be registered as singleton. The registration method on MEDI is factory (to be able to implement the title feature). Registering singleton with factory method is subject to automatic disposal of the instantiated object that is undesirable since the disposing of ILogger
handled by SerilogLoggerFactory
(passing dispose=true
to it constructor).
AFAIC, all this ceremony needed to correctly handle preserverStaticLogger=false
(to do Log.CloseAndFlush()
) otherwise the disposal could be handled by the DI container itself..
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.
Quite hard to understand to one who has no some background knowledge about all this stuff. I guessed that the library should consider all combinations of use. Maybe we should add your comment to the code? It’s hard to understand what’s going on without it.
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.
Added a comment paraphrasing Sergey's - thanks for the suggestion 👍
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.
👍
This change adds an overload to
UseSerilog()
that passes anIServiceProvider
to the logger configuration callback:There's a lot we could do on top of this to improve UX, but this is the fundamental capability needed to get the ball rolling.
The initialization of Serilog in the "inline" scenario will now have a slightly different timing, but I don't think this will be noticeable for most consumers.
I've tested this manually in a couple of different configurations; I'd love for someone with the Hosting familiarity to help set up some better unit tests for it, but I think the need for this is such that we should get it out there and start iterating :-)
ILogger
for DI #16