Fix OmniSharp log filtering #1293
Conversation
When the logging packages were updated from 1.1 to 2.1, we took a breaking change (aspnet/Announcements#238) that caused the --loglevel switch to no longer work properly. This change uses the new APIs for configuring logging and centralizes the filter. Note that I didn't change the LSP implementation because it looks to me like LSP is using loggers differently.
| @@ -33,5 +34,30 @@ public void Dispose() | |||
| { | |||
| throw new NotImplementedException(); | |||
| } | |||
|
|
|||
| private static bool LogFilter(string category, LogLevel level, IOmniSharpEnvironment environment) | |||
There was a problem hiding this comment.
I moved this here because LSP is the only one using this method now. It might be that this is unneded, but LSP's logging looked a bit fancier, so I left it.
There was a problem hiding this comment.
That should be fine, I'll see if I can take a look at lsp logging later.
| var projectEventForwarder = typeof(ProjectEventForwarder).FullName; | ||
|
|
||
| builder.AddFilter( | ||
| (category, logLevel) => |
There was a problem hiding this comment.
Note that all of the logic below is inverted from the original HostHelpers.LogFilter(...) method.
| @@ -130,7 +135,20 @@ public static IServiceProvider CreateDefaultServiceProvider(IOmniSharpEnvironmen | |||
| services.Configure<OmniSharpOptions>(configuration); | |||
| services.AddSingleton(configuration); | |||
|
|
|||
| services.AddLogging(); | |||
There was a problem hiding this comment.
The code below is how logging is supposed to be configured these days. Filtering is a first-class feature for the logging APIs now.
|
@DustinCampbell How hard would it be to add some tests for this? |
|
It'd take a bunch more refactoring. The problem is that we use a different |
|
I think I see a place where I can add a test. I'll give it a shot. |
|
Added a couple of tests. I overestimated the amount of refactoring required. 😄 |
DustinCampbell
force-pushed
the
fix-logging
branch
from
c8875cd
to
aa00f4a
Compare
| @@ -33,5 +34,30 @@ public void Dispose() | |||
| { | |||
| throw new NotImplementedException(); | |||
| } | |||
|
|
|||
| private static bool LogFilter(string category, LogLevel level, IOmniSharpEnvironment environment) | |||
There was a problem hiding this comment.
That should be fine, I'll see if I can take a look at lsp logging later.
When the logging packages were updated from 1.1 to 2.1, we took a breaking change (aspnet/Announcements#238) that caused the
--loglevelswitch to no longer work properly. So, passing--loglevel debugdoesn't do anything.This change uses the new APIs for configuring logging and centralizes the filter. Note that I didn't change the LSP implementation because it looks to me like LSP is using loggers differently.