Implement context.Database.Log for simple SQL logging #16196
Conversation
Fixes #1199 Very similar to EF6, except the log level can also be set. For example: ```C# context.Database.Log(Console.WriteLine); ``` or ```C# context.Database.Log(Console.WriteLine, LogLevel.Information); ``` Only logs command execution for now, since that's all we have an interceptor for so far. Uses the `EventData.ToString()` trick to get the same message formatting as we use for `ILogger`, but all the information is available to customize the output if needed.
|
/cc @divega |
|
Finally!! |
|
@ErikEJ I realized last night that now we have interceptors we have the stuff in place to finally do this cleanly! |
| Check.NotNull(interceptors, nameof(interceptors)); | ||
|
|
||
| _interceptors = interceptors; | ||
| _count = interceptors.Length; |
There was a problem hiding this comment.
Did you add this for perf? Is there a difference?
There was a problem hiding this comment.
Not only for perf, it was part of other refactoring too. I didn't measure.
There was a problem hiding this comment.
This is great!
I think we should brainstorm about how this gels with other things in the big picture. For example:
- When we talk about logging from now on, are we going to have to qualify it to specify what kind of logging (ILogger or Database.Log) we are talking about?
Only logs command execution for now, since that's all we have an interceptor for so far.
- So, we expect to start reporting more things in Database.Log if we add other interceptors, but I assume the fact that is hanging from Database means it should always filter to things that are about the interaction with the database. Then what is our model for thinking about other kinds of information, like model building? Just alway use ILogger or DiagnosticSource?
I realized last night that now we have interceptors we have the stuff in place to finally do this cleanly!
I suspect it would be useful for me to understand why interception (or what part of interception) was a necessary building block for this.
| /// <param name="action"> The delegate to which log messages will be written. </param> | ||
| /// <param name="level"> The log level to filter by. Only events at or more severe than this level will be logged. </param> | ||
| public virtual void Log([NotNull] Action<string> action, LogLevel level = LogLevel.Debug) | ||
| { |
There was a problem hiding this comment.
We can leave it for API review, but the name doesn’t quite work for me for several reasons. Something that starts with Set would probably be an improvement, but not sure about something like SetLogger because if the overlap between ILogger and this functionality (see my other comments).
I also want to touch on having something that is multicast, just in case. I see that simplicity is the high order bit, and multicast can be built externally, but given this is already a method (as opposed to a property as in EF6) I don’t think the delta in complexity would be too much.
|
Don't feel like debating things for now. We can revisit later. |
|
Some comments on this feature from my perspective. Like JimBobSquarePants (quoted by @divega in #15888) I believe that making the the SQL produced by EF Core is VERY important. An ORM hides the database so well that developers can write poor performing queries, but being able to see the SQL is a great antidote for those that want to learn. The other great thing about EF Core's SQL output is it is so readable - it looks like something a human would write. That again is VERY important (In EF6 I gave up trying to understand its SQL output because it was so hard to understand). An aside: Another aspect of EF Core producing very readable SQL is DBA are less negative when they see EF Core SQL. With EF6's SQL output it just confirmed their view that the ORM is rubbish (their view, not mine). The question I have is: now have two ways of getting the SQL code, ILogger and Database.Log, so which one should people use? I suspect that ILogger will be the main use as it so easy, and turned on by default in ASP.NET Core. Which means what does Database.Log provide over ILogger and where would you use it? As I understand Database.Log provides access to I hope my ramblings are useful in your deliberations. |
Fixes #1199
Very similar to EF6, except the log level can also be set. For example:
or
Only logs command execution for now, since that's all we have an interceptor for so far.
Uses the
EventData.ToString()trick to get the same message formatting as we use forILogger, but all the information is available to customize the output if needed.