Remove MS.Ext.Logging and Abstractions from most of our repo #10199
Conversation
ie, no BeginScope, no EventIds. Message formatter could go in future too.
This is mostly mechanical fixing of namespace, plus some test mock changes
Copies from MS.Ext.Logging :) In future we should use a proper interpolated string handler
davidwengier
changed the title
| public bool IsEnabled(LogLevel logLevel) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter) | ||
| public void Log<TState>(LogLevel logLevel, TState state, Exception exception, Func<TState, Exception, string> formatter) |
There was a problem hiding this comment.
Can state be removed here?
There was a problem hiding this comment.
This is how the message gets passed currently. Will fix in a follow up PR.
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxTreeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Logging/AbstractRazorLoggerFactory.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Logging/AbstractRazorLoggerFactory.cs
Outdated
Show resolved
Hide resolved
| using var _ = StringBuilderPool.GetPooledObject(out var vsb); | ||
| var scanIndex = 0; | ||
| var endIndex = format.Length; | ||
|
|
||
| var count = 0; | ||
|
|
||
| while (scanIndex < endIndex) | ||
| { | ||
| var openBraceIndex = FindBraceIndex(format, '{', scanIndex, endIndex); | ||
| if (scanIndex == 0 && openBraceIndex == endIndex) | ||
| { | ||
| // No holes found. | ||
| return format; | ||
| } | ||
|
|
||
| var closeBraceIndex = FindBraceIndex(format, '}', openBraceIndex, endIndex); | ||
|
|
||
| if (closeBraceIndex == endIndex) | ||
| { | ||
| vsb.Append(format, scanIndex, endIndex - scanIndex); | ||
| scanIndex = endIndex; | ||
| } | ||
| else | ||
| { | ||
| // Format item syntax : { index[,alignment][ :formatString] }. | ||
| var formatDelimiterIndex = format.AsSpan(openBraceIndex, closeBraceIndex - openBraceIndex).IndexOfAny(',', ':'); | ||
| formatDelimiterIndex = formatDelimiterIndex < 0 ? closeBraceIndex : formatDelimiterIndex + openBraceIndex; | ||
|
|
||
| vsb.Append(format, scanIndex, openBraceIndex - scanIndex + 1); | ||
| vsb.Append(count++); | ||
| vsb.Append(format, formatDelimiterIndex, closeBraceIndex - formatDelimiterIndex + 1); | ||
|
|
||
| scanIndex = closeBraceIndex + 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need this? Do we want to keep using the MS.Ext.Logging string formatting? Or, would it be better to just use AppendFormat and add use interpolated strings (with InterpolatedStringHandler methods)?
There was a problem hiding this comment.
Will follow up with an interpolated string handler in another PR.
| } | ||
| } | ||
|
|
||
| return string.Format(vsb.ToString(), args); |
There was a problem hiding this comment.
It feels strange to call string.Format here, which will just use another StringBuilder to call AppendFormat.
|
|
||
| internal interface ILogger | ||
| { | ||
| void Log<TState>(LogLevel logLevel, TState state, Exception? exception, Func<TState, Exception?, string> formatter); |
There was a problem hiding this comment.
Do we need all of the extreme flexibility this signature provides?
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Logging/ILogger.cs
Show resolved
Hide resolved
| { | ||
| foreach (var logger in _loggers) | ||
| { | ||
| if (logger.IsEnabled(logLevel)) |
There was a problem hiding this comment.
I'm assuming this was always the case, but some of our loggers also check IsEnabled before logging. Should we remove that check there?
There was a problem hiding this comment.
Will do something about this in a follow up PR. With an interpolated string handler, hopefully the Log call can be avoided entirely in the right circumstances
|
The answer to a bunch of the feedback is "I deliberately left the implementation unchanged to reduce the churn, and make the review easier", so I can either follow up in this PR, or another. I've always thought the log method has a weird signature :) |
|
Will do the renames in this PR, since it already touches so many files, and will follow up with a nicer API and use of an interpolated string handler in a future PR. |
We don't get much from using MS.Ext.Logging for our logging, and we pay for it with
twothree (?) DLL loads in VS. This fixes that by implementing the logging bits outselves, and removing the abstraction for options monitoring which we weren't really using.I also removed the notion of "scopes" from the logging because we never actually implemented them, so this also closes #8232. We don't actually do any structured logging so we're really not missing much.
The only place that does still use MS.Ext.Logging is our one DLL that is referenced by Roslyn in VS Code, since they pass us an ILogger, so we don't really have a choice about that bit :)
I still need to do a little more testing of the options bit,
because I'm seeing some oddities, but I think the issue is really one with the unified settings experience and/or our options updating code.Okay yeah, this is happening in VS main too. Logged #10200 but will make sure this doesn't get any worse :)