ILogger input #18
ILogger input #18
Conversation
| @@ -245,6 +245,48 @@ namespace SerilogEventFlow | |||
| } | |||
| ``` | |||
|
|
|||
| #### ILogger | |||
|
|
|||
| *Nuget package:* **Microsoft.Diagnostics.EventFlow.Inputs.ILogger** | |||
There was a problem hiding this comment.
I is likely that a lot of different libraries will use the ILogger concept and define their own ILogger implementation. For example Serilog has one: https://github.com/serilog/serilog/blob/dev/src/Serilog/ILogger.cs
So I suggest that we name the package and the input so that it conveys it is about Microsoft.Extensions.Logging package and its implementation of ILogger. For example
Microsoft.Diagnostics.EventFlow.Inputs.MicrosoftLogging
There was a problem hiding this comment.
will rename to Microsoft.Diagnostics.EventFlow.Inputs.MicrosoftLogging
| The ILogger input has no configuration, other than the "type" property that specifies the type of the input (must be "ILogger"): | ||
| ```json | ||
| { | ||
| "type": "ILogger" |
There was a problem hiding this comment.
How about using "Microsoft.Extensions.Logging" as the input type?
| using (var pipeline = DiagnosticPipelineFactory.CreatePipeline(".\\eventFlowConfig.json")) | ||
| { | ||
| var factory = new LoggerFactory() | ||
| .AddEventFlow(pipeline); |
There was a problem hiding this comment.
I like this. Very clean!
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Logging.Internal; | ||
|
|
||
| namespace Microsoft.Diagnostics.EventFlow.Inputs.ILogger.Logger |
There was a problem hiding this comment.
We use just Microsoft.Diagnostics.EventFlow.Inputs namespace for all input code
There was a problem hiding this comment.
(applies to other files too)
|
|
||
| public EventFlowLogger(string categoryName, LoggerInput loggerInput) | ||
| { | ||
| Debug.Assert(categoryName != null); |
There was a problem hiding this comment.
Use Validation.NotNull() for parameter checking
There was a problem hiding this comment.
ok (EventFlowLogger is internal, so it's not public API)
There was a problem hiding this comment.
Understood, but even for internal types, I like to do parameter checking and throw if the checks fail. I like to think that public methods & properties form a contract with consumers and parameter checking is part of that contract. OK to just assert the correctness of parameters of private methods.
| } | ||
| else | ||
| { | ||
| properties.Add("Scope", scope.State); |
There was a problem hiding this comment.
Same concern about duplicate property names here
|
|
||
| public bool IsEnabled(Extensions.Logging.LogLevel logLevel) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
Hmm. Should we allow to drop events based on the level inside this Logger? Seems like a common requirement--during routine execution you probably want to capture only warnings or worse.
There was a problem hiding this comment.
Level based event filtering could be done without EventFlow when factory is being created:
factory
.WithFilter(new FilterLoggerSettings { { "ConsoleApp1.Program", LogLevel.Warning } })
.WithEventFlow();
If we additionally configure level-based filtering on EventFlow side it may conflict with MS.Extenstion.Logging approach.
There was a problem hiding this comment.
Sounds good, let's not have this extra configuration parameter
| if (payload != null) | ||
| { | ||
| foreach (var kv in payload) | ||
| eventPayload.Add(kv); |
There was a problem hiding this comment.
What if the payload already has properties named "Message", "EventId", "EventName" etc?
| } | ||
| catch (Exception ex) | ||
| { | ||
| this.healthReporter?.ReportProblem($"Failed to write message. Error: {ex}", TraceTag); |
There was a problem hiding this comment.
- Health reporter will never be null (given constructor check)
- Reporting a warning is probably more appropriate
- Use ex.ToString() for extra clarity
| @@ -0,0 +1,232 @@ | |||
| using System; | |||
There was a problem hiding this comment.
Nice job with the tests!!
| return new AddResult(); | ||
| } | ||
|
|
||
| Random random = new Random(DateTime.Now.Millisecond); |
There was a problem hiding this comment.
- No need to use Random(seed) constructor; the parameter-less constructor does what your code does
- How about creating a static instance of the Random class and caching it?
|
One more thing: in Warsaw.sln make sure the new MicrosoftLogging project is listed as a dependency of the signing project |
This change implements ILogger input