Skip to content
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

SQL should be logged at debug level #14523

Closed
rgamage opened this issue Jan 25, 2019 · 10 comments
Closed

SQL should be logged at debug level #14523

rgamage opened this issue Jan 25, 2019 · 10 comments
Assignees
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@rgamage
Copy link

rgamage commented Jan 25, 2019

Note that this change was reverted before the final release of 3.0. Instead we made it easy to configure any event to log at any level. See #15888


Currently, if we implement logging filtered at the Information level, we get details of every EF SQL query. This is clearly debug or trace level data, not Information. It clutters everyone's logs, and is useless because the data is hiding the interesting things like id numbers, field values, etc.

What I'd like to see is these EF SQL details to be down-graded to Debug level.

@ajcvickers ajcvickers transferred this issue from dotnet/extensions Jan 25, 2019
@ajcvickers
Copy link
Member

@DamianEdwards @davidfowl TL;DR Please let us know if we should not change the default log level for SQL to debug.

We've seen this feedback (SQL should be logged at debug) fairly consistently, and I think the general consensus on the team is we should change the level to debug. However, the decision to log SQL at info level was made by Project K at a higher level, so checking in with you guys before pulling the trigger.

@ajcvickers ajcvickers changed the title Logging Entity Framework query details - should be hidden at Information level SQL should be logged at debug level Jan 28, 2019
@ajcvickers ajcvickers self-assigned this Jan 28, 2019
@ajcvickers ajcvickers added this to the 3.0.0 milestone Jan 28, 2019
@davidfowl
Copy link
Member

I think its fine to make it debug by default. It would be nice if it was a different provider name (one that said something like "Microsoft.EntityFramework.Queries") that can be dialed up or down in configuration.

ajcvickers added a commit that referenced this issue Feb 10, 2019
Issue #14523

I looked at making it easy to change the level back, but the ASP.NET delegate compiler expects the LogLevel to come in pre-baked--i.e. its parameterized on the level, which means we would have to regenerate the delegate to do this.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Feb 10, 2019
ajcvickers added a commit that referenced this issue Feb 11, 2019
Issue #14523

I looked at making it easy to change the level back, but the ASP.NET delegate compiler expects the LogLevel to come in pre-baked--i.e. its parameterized on the level, which means we would have to regenerate the delegate to do this.
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview3 Feb 22, 2019
ajcvickers added a commit that referenced this issue Mar 10, 2019
Issue #14970

Example API:
```C#
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    => optionsBuilder
        .UseInMemoryDatabase("MyDb")
        .ConfigureWarnings(c => c.Log((CoreEventId.CascadeDelete, LogLevel.Warning)));

```

Mitigates the breaking change of #14523.

Also, makes initialization of logging definitions lazy. We have quite a lot and eagerly initializing all of them the first time any string resource is used was bothering me.

Also, stopped including Exception in logging definition since this causes the stack trace to be included in the log even if the exception has been handled. The exception is still available in diagnostics.
ajcvickers added a commit that referenced this issue Mar 11, 2019
Issue #14970

Example API:
```C#
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    => optionsBuilder
        .UseInMemoryDatabase("MyDb")
        .ConfigureWarnings(c => c.Log((CoreEventId.CascadeDelete, LogLevel.Warning)));

```

Mitigates the breaking change of #14523.

Also, makes initialization of logging definitions lazy. We have quite a lot and eagerly initializing all of them the first time any string resource is used was bothering me.

Also, stopped including Exception in logging definition since this causes the stack trace to be included in the log even if the exception has been handled. The exception is still available in diagnostics.
ajcvickers added a commit that referenced this issue Mar 12, 2019
Issue #14970

Example API:
```C#
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    => optionsBuilder
        .UseInMemoryDatabase("MyDb")
        .ConfigureWarnings(c => c.Log((CoreEventId.CascadeDelete, LogLevel.Warning)));

```

Mitigates the breaking change of #14523.

Also, makes initialization of logging definitions lazy. We have quite a lot and eagerly initializing all of them the first time any string resource is used was bothering me.

Also, stopped including Exception in logging definition since this causes the stack trace to be included in the log even if the exception has been handled. The exception is still available in diagnostics.
@ajcvickers
Copy link
Member

@rgamage The default experience when running an ASP.NET app in production is to log at Warning level. Logging at Info level is the default only for development environments.

With the implementation of #14970 it is now possible to easily configure any EF event to log at any level. This is making us revisit again whether changing the default level for SQL is the correct thing to do. Can you provide some more details of when and why you are using Info level logging?

/cc @divega @Eilon @DamianEdwards

@fschlaef
Copy link

fschlaef commented May 9, 2019

For anyone wanting to re-enable query logging without embedding configuration in DbContext (with Serilog for example, which is what I'm doing), the override you're looking for is Microsoft.EntityFrameworkCore.Database.Command

Excerpt from my Serilog conf :

"Serilog": {
    "MinimumLevel": {
      "Default": "Debug",
      "Override": {
        "System": "Information",
        "Microsoft": "Information",
        "Microsoft.EntityFrameworkCore.Database.Command": "Debug"
      }
    }
}

This pretty much restores the old behavior, while maintaining control over what is logged in config files.

Edit : big downside compared to old behavior : all queries are now logged twice (once for Executing DbCommand and once again for Executed DbCommand)

@ajcvickers how would you use CoreEventId in json config files for use with Serilog ?

@JonPSmith
Copy link

I agree with reducing the logging load because I have seen the impact of looging on performance. However I do think that having access to the database commands (typically SQL) that EF Core produces is very important. This is a great way to understand a database access, especially if you are looking to performance tune things. I recommend this to everyone who is concerned with EF Core performance.

The problems I see is the simple solution of setting the LogLevel to Debug would include a whole load of other Debug information (well, it does when I tried it). I am therefore looking at ways to isolate the database commands. Here are some solutions I came up with:

  1. Use @fschlaef solution, i.e. filter by the EventId/Name. That works but needs a more complex setup.
  2. My favourite would be all existing Debug level logging, apart from Executing DbCommand to the Trace level. I realise this might be too bigger change inside EF Core, but it would make life easier for EF Core users.
  3. The final approach is to publicise the DiagnosticSource for getting database commands. I have no experience of using DiagnosticSource so I don't know if this is difficult for developers to use, but it was recommended to me in issue Request for better parameter infromation in CommandExecuted log output #15085.

@ajcvickers
Copy link
Member

@JonPSmith As documented, you can change the level of only this event to whatever you want with something like this:

protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    => optionsBuilder
        .UseSqlServer(connectionString)
        .ConfigureWarnings(c => c.Log((RelationalEventId.CommandExecuting, LogLevel.Info)));

@JonPSmith
Copy link

@ajcvickers, Sorry, I read the headline and didn't read further - my bad.

That is an acceptable fix and I'll try to publicise that when I'm writing about EF Core 3.

@ajcvickers
Copy link
Member

Note: we are reverting this breaking change. See #15888

@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 16, 2019

Also update breaking changes docs?

@ajcvickers
Copy link
Member

@ErikEJ Done, but haven't sent the PR yet.

@ajcvickers ajcvickers removed this from the 3.0.0-preview3 milestone Nov 11, 2019
@ajcvickers ajcvickers added this to the 3.0.0 milestone Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

No branches or pull requests

6 participants