Skip to content

Middleware relies on static Log.Logger being set first. #182

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

Closed
dazinator opened this issue Mar 25, 2020 · 3 comments · Fixed by #183
Closed

Middleware relies on static Log.Logger being set first. #182

dazinator opened this issue Mar 25, 2020 · 3 comments · Fixed by #183

Comments

@dazinator
Copy link

dazinator commented Mar 25, 2020

I am trying to use only instances of ILogger. As such I wondered what would happen if I never set Log.Logger.

I found was now getting no request logging. Looking at the request logging middleware it looks like Log.Logger needs to be set first as it uses Log.ForContext: https://github.com/serilog/serilog-aspnetcore/blob/dev/src/Serilog.AspNetCore/AspNetCore/RequestLoggingMiddleware.cs#L76 and if Log.Logger isn't set this yields SilentLogger's. (To be honest I would think it would be better to throw an exception in such a case as logging not being initialised is pretty important)

I'd propose an enhancement to the middleware so that an instance of a logger can be provided and that used to get log context instead --> falling back to the static Log.Logger if no instance provided might be a good way to keep this a backwards compatible change?

@nblumhardt
Copy link
Member

Hi @dazinator - would accepting an ILogger through RequestLoggingMiddlewareOptions work with the startup sequence you're using? Just had a look at the effort involved, seems minimal, spiked a PR.

@dazinator
Copy link
Author

Yep that works for me!

@pnmcosta
Copy link

Me too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants