EventLog Scopes #741
EventLog Scopes #741
Conversation
| @@ -9,10 +9,12 @@ namespace Microsoft.Extensions.Logging.EventLog | |||
| /// The provider for the <see cref="EventLogLogger"/>. | |||
| /// </summary> | |||
| [ProviderAlias("EventLog")] | |||
| public class EventLogLoggerProvider : ILoggerProvider | |||
| public class EventLogLoggerProvider : ILoggerProvider, ISupportExternalScope | |||
There was a problem hiding this comment.
breaking change technically?
There was a problem hiding this comment.
While this is technically a breaking and adding interface can change overload resolution ISupportExternalScope would be new in the release so no one would be using it.
There was a problem hiding this comment.
Yeah not worried about this type of breaking change in this particular case.
| // Arrange | ||
| var loggerName = "Test"; | ||
| var maxMessageSize = 50 + loggerName.Length + Environment.NewLine.Length; | ||
| var expectedMessage = loggerName + Environment.NewLine + |
There was a problem hiding this comment.
This is how we want messages to look?
Test
Message
Outer Scope
Inner Scope
|
I'm curious about this change. When the logging provider (EventLogLoggerProvider) implements But the EventLogLoggerProvider.GetLogger() doesn't pass the IExternalScopeProvider as parameter to the newly created EventLogLogger. Instead the EventLogLogger creates its own internal version, that is only exercised when calling BeginScope directly, but in the "real world" that will never happen. Is it the intention that EventLogLogger should not perform logging of current scope? |
|
Good catch. That was a bug that was fixed in the new versions: https://github.com/aspnet/Extensions/blob/master/src/Logging/Logging.EventLog/src/EventLogLoggerProvider.cs#L36 |
|
Ahh it was hidden in the refactoring commit from dotnet/extensions#502. Sorry about the noise |
|
Any reasons for having the stale copy around here? when the correct version now resides at new location? |
|
Repository title says |
#713