[Suggestion] Leverage ILoggerFactory for non DI support

[pnmcosta]

Hi @hryz,

Excellent job with this package, it is precisely what we're looking for, that would allow us to diagnose Dapper connections/commands, and your package is the best from the ones we've also been evaluating.

However the only thing stopping us from using it, is that we also have classic .net projects without DI support. I'm aware of the hooks low level API in you package, and that would probably work, but would be very messy IMO, cause it would basically be the same as the existing IDbConnectionFactory implementations but would use a Serilog.ILogger instead.

But I believe I have a suggestion to slightly modify the internals of your package that would not be a breaking change, and would by default make your package support non DI leveraged projects, and potentially even other DI containers than MS's - but I'm not sure about that - yet :grin: .

The IDbConnectionFactory implementations should depend on a ILoggerFactory instead of ILogger<DbConnection> so that those that don't use DI can manually inject a SerilogLoggerFactory or even a custom one for example for testing.

I would also additionally suggest that the ILogger's created from the logger factory to have the context/category of the connection type instead, e.g. ILogger<WrappedConnectionFactory<T>>, because I believe ILogger<DbConnection> is too generic and if the project consuming this package uses other connection providers/types, it could create some confusion to the log stream.

For your consideration, if you agree, I'd happily contribute with a modified fork, however I'll have to do it after work as we don't have github permissions from here, i couldn't even clone the repo :sob:

Looking forward to hearing back from you,

Cheers, P.

[pnmcosta]

For further consideration, what I'm proposing is similar to what EF Core also does, see https://docs.microsoft.com/en-us/ef/core/miscellaneous/logging?tabs=v3#other-applications

When configuring the DbContext you set the logger factory to be used with UseLoggerFactory, see this for more information.

[hryz]

Hello @pnmcosta,

Thank you for your feedback. It's very much appreciated. I've looked through your suggestion and it totally makes sense. I'm having a few quite busy days. This weeken I'll check the EF links you shared and make sure the proposed approach won't hurt the project in the future. If everything is ok I'll create a new version.

[pnmcosta]

Pleasure @hryz thanks for looking into this too, really appreciate it.

If you want I can give it a head start (fork), and then you can review if you'd like to integrate it, would that work for you?

P.

[hryz]

Hey @pnmcosta,

Could you please elaborate on your scenario of using this lib without DI? Is it some sort of background processor or classical MVC/WebAPI on top of Full Framework? I'm trying to understand better the life-time of the objects in your scenario. In the world of AspNetCore DbContext is typically created per HTTP request. The scope is the request. In the case of Dapper, the DbConnection is typically also created per request. In both cases, the Loggers injected into controllers or services are also created per request. The ILoggerFactory, as well as ILoggerProviders, are singletons and created on bootstrap. However, sometimes people use the static logger created and configured at bootstrap.

If I understand you correctly, you would like to do the following:

1. Setup the Serilog static logger: Log.Logger = new LoggerConfiguration().CreateLogger();
2. Create an instance of SerilogLoggerFactory that will use the Serilog static logger internally
3. Create an instance of ContextfulLoggingFactory<T> or ContextlessLoggingFactory and pass the instance of SerilogLoggerFactory to it.
4. Use the shared ContextfulLoggingFactory<T> or ContextlessLoggingFactory instances to create a new DbConnection for Dapper whenever needed.

In the current implementation, you can do exactly the same with 1 extra step: var logger = new SerilogLoggerFactory().CreateLogger<DbConnection>(); and pass this ILogger<DbConnection> as argument to ContextfulLoggingFactory<T> This should be an equivalent to replacing the ILogger by ILoggerFactory in the lib.

However, this does not solve the other issue you mentioned. When logs have the context DbConnection it's too generic and sometimes can confuse people. It'd be nice to differentiate different types of DbConnection providers.

Please correct my assumptions about your scenario if they are incorrect. I might be missing something.

I'm trying to take into consideration everything: the possibility of breaking changes, future extension issues, performance issues, security concerns, etc. That's why I think so much about even tiny changes, like this. I'm sorry if it looks like I'm nitpicking 😀 I'm really not.

[pnmcosta]

Hi @hryz,

There's a mix of many projects, console's and also some MVC/WebAPI on top of Full Framework.

Your assumptions are all correct, and come to think of it var logger = new SerilogLoggerFactory().CreateLogger<DbConnection>(); would definitely work for this scenario, I guess I was tunnel visioned to inject a SerilogLoggerFactory instead 😂

But I definitely see the benefit of the log contexts to differentiate different types of DbConnection providers, so please do consider it.

Thank you for looking at this in such detail, I would never consider it nitpicking, but rather 2 engineers brainstorming solutions ;)

P.

[hryz]

Done

[pnmcosta]

Perfect, thank you :+1: