issue #22 - added capability to provide known event id's based off log events #23
Conversation
There was a problem hiding this comment.
Looks great! Just a couple of thoughts; @GavinSutherland how does this match up to your expectations?
| /// </summary> | ||
| /// <param name="logEvent">The log event to compute the event id from.</param> | ||
| /// <returns>Computed event id based off the given log.</returns> | ||
| ushort Compute(LogEvent logEvent); |
There was a problem hiding this comment.
Agreed. Commit pushed.
| { | ||
| if (loggerConfiguration == null) throw new ArgumentNullException(nameof(loggerConfiguration)); | ||
|
|
||
| var formatter = new MessageTemplateTextFormatter(outputTemplate, formatProvider); | ||
|
|
||
| return loggerConfiguration.Sink(new EventLogSink(source, logName, formatter, machineName, manageEventSource), restrictedToMinimumLevel); | ||
| return loggerConfiguration.Sink(new EventLogSink(source, logName, formatter, machineName, manageEventSource, eventIdProvider), restrictedToMinimumLevel); |
There was a problem hiding this comment.
Perhaps this is where we do the ?? new EventIdHashProvider(), so that EventLogSink can require a non-null IEventIdProvider argument?
There was a problem hiding this comment.
I think that would be cleaner as it keeps configuration in LoggerConfigurationEventLogExtensions and it makes the EventLogSink more transparent on behavior. My only concern is introducing a new required parameter on the existing EventLogSink constructor. Is this something we should be concerned with? If so, I could make this change as requested and add a constructor overload for EventLogSink to support a custom IEventIdProvider.
Let me know what you think.
There was a problem hiding this comment.
Overloaded constructor sounds like it would be a winner 👍
There was a problem hiding this comment.
I like the idea of an overloaded constructor on EventLogSink to allow us to pass in a non-null IEventIdProvider.
There was a problem hiding this comment.
added overloaded constructor to EventLogSink. Attempting to keep default value for IEventIdProvider in one location so I've made the more generic constructor call the more complex constructor with the default value and have LoggerConfigurationEventLogExtensions smart enough to determine which constructor to utilize.
|
Yeah this looks good and meets my requirements. Other than creating the overloaded Thanks Chris! |
| @@ -68,6 +70,7 @@ public static class LoggerConfigurationEventLogExtensions | |||
| /// <param name="manageEventSource">If false does not check/create event source. Defaults to true i.e. allow sink to manage event source creation</param> | |||
| /// <param name="restrictedToMinimumLevel">The minimum log event level required in order to write an event to the sink.</param> | |||
| /// <param name="formatter">Formatter to control how events are rendered into the file. To control | |||
| /// <param name="eventIdProvider">Supplies event ids for emitted log events.</param> | |||
There was a problem hiding this comment.
The eventIdProvider param documentation has been inserted in the middle of the formatter param documentation.
Fixed documentaiton in LoggerConfigurationEventLogExtensions Updated LoggerConfigurationEventLogExtensions to honor the constructor overload
|
👍 thanks @mrbcmorris ! |
|
Odd, looks like CI somehow skipped the last commit - missing file: https://ci.appveyor.com/project/serilog/serilog-sinks-eventlog/build/46 @mrbcmorris do you have the missing appsettings.xml file handy? :-) Cheers! |
|
@nblumhardt interesting...it looks like the file is still here. Digging in a bit more, i was surprised to see the particular test in question using Generally I've relied on I still don't understand how this test was passing previous to this PR. Thoughts on how to move forward? |
|
@mrbcmorris thanks for the follow-up; I think I'd guess some change to the build image will have been responsible. New |
Just wanted to get the ball rolling for issue# 22 with a sample implementation.