Request time log level

[KieranSasseSolarGain]

It would be nice if it was possible to configure the log level based off of the total request time.

For example:

Request time < 300ms --> verbose Request time < 1000ms --> info Request time < 5000 ms --> warning etc.

These changes should achieve that.

[tsimbalar]

Hi,

Thank you for your PR 👍

I think the idea makes sense, but if we want to do something like this, I would maybe prefer something more "generic" which would be to allow passing a Function that would decide what ErrorLevel to log to based on all the info we have at the moment of emitting the log event.

For instance, I could imagine that client code would do something like :

SerilogWebClassic.Configure(cfg => cfg
  .UseLogger(myCustomLogger)
  .LogAtLevel((ctx, duration) => /* here we could have logic that looks at duration or context to determine logging level*/ LogEventLevel.Debug)
));

where`:

• ctx would be a HttpContextBase
• duration would be a TimeSpan (clearer than a long where you possibly don't remember if it's miliseconds, seconds or ticks etc)

This means we would have an override of LogAtLevel that accepts a Func<HttpContextBase, TimeSpan, LogEventLevel>. When such a "dynamic" rule is defined, we would then use it to determine the level at which to log in https://github.com/serilog-web/classic/blob/69cb8af00c4c04572893d4426a7b577d5511fcc1/src/SerilogWeb.Classic/Classic/WebRequestLoggingHandler.cs#L47

If you feel like trying such a PR, I could try and help you in the process. Be aware that I don't have a .NET dev environment set up currently, but I'll do my best :P

[KieranSasseSolarGain]

@tsimbalar

Hi,

Thank you for your reply.

I have modified my PR to what it is I believe you requested. Please let me know if you require further changes.

[KieranSasseSolarGain]

Hi again,

Thanks for your feedback. I have had a chance to look through all your suggestions now and they all make sense to me but I have run into one scenario where I do not know which path I should take:

ApplicationLifecycleModule.cs

By removing the RequestLoggingLevel property I am not sure what can be returned from the 'get' of this property as it could be called in places outside of a http request context right? I am not sure if it is safe to remove this property as I believe it will break existing client projects?

[tsimbalar]

Hi @KieranSasseSolarGain

I have run into one scenario where I do not know which path I should take:

Oh yes, good point I didn't think of this.

This property has been deprecated since v4.1 (almost 3 years ago) , and we are now in version v5.x , so I think it's probably ok to introduce a little bit of breaking changes.

In order to minimize the impact, I would maybe remove only the getter of the property RequestLoggingLevel , hoping that people is not relying it on it too much. I'm expecting that people is mostly using the setter anyway.

Does that unblock you ?

[KieranSasseSolarGain]

Hi again @tsimbalar,

I have now had a chance to revisit this PR and I believe I have made the changes you have requested.

I have assumed that the new LogLevelEvaluator function should only be set from the ConfigurationBuilder stuff as everything else on the ApplicationLifecycleModule has been marked deprecated.

I also just removed the getter on the RequestLoggingLevel property as you suggested.

Please let me know if anything else is required,

Cheers

[tsimbalar]

Hi Kieran !

This looks good, thanks so much for going until the end 💪

I haven't done it in a while, so let's see if I remember how it's done, but let me merge this and publish a release :)

[tsimbalar]

Published under v5.1.66 !

https://www.nuget.org/packages/SerilogWeb.Classic/5.1.66