Better align with expected behaviors when used with ASPNET Core

[nickalbrecht]

Settings for Exceptional should be obtainable by requesting an instance of IOptions<Settings> from dependency injection. At the moment they exist as a static property on the Settings class called Current. This is likely needed for backwards compatibility, but making it available through dependency injection is the typical expectation when working with ASP.NET Core. Need to verify that if the reloadOnChange: true setting is set when loading the appsettings.json file, that Exceptional handles the changes to the configuration as expected.

Exception logging should be possible by calling one of the ILogger<>.Log() methods (if the Exception parameter is not null). Will likely need to adjust the way the ILogger instance is used inside of the error handler to not recursively call itself should there be a problem with the error handler itself.

[nickalbrecht]

I don't think the dynamic reload configuration behavior is easily achieved/realistic with ASP.NET 1.1. However the Options system seems to have been fleshed out a lot in 2.0, including adding a concept of IOptionsFactory to facilitate creation. Seems like a good place to start.

I'm not completely sold on my original idea of exceptions logged to a Logger cascading through to Exceptional because of recursion. Or at least to prevent it, there may need to be a caveat that any implementation of ILogger<> in Exceptional ignore errors from the Exceptional namespace itself.

[nickalbrecht]

On a side note, I was thinking about this last night. It'd be nice if the Settings used for Exceptional was a POCO class, and things that were more complex/stateful/delegates/etc were in some sort of current configuration instance. I feel like currently the ConfigSettings class used to get information from the web.config/appsettings.json is more along the lines of a POCO settings class, but that the full Settings class used in Exceptional currently an amalgam of settings, stateful instances, custom accessors/getters, and generic lists of complex objects. Though I realize this may be more of a wishlist item than anything, since refactoring it would introduce breaking changes for existing usages of Exceptional. Though 2.0 is a major version number revision. It would greatly simplify obtaining/refreshing the settings from whatever text file holds the information. </brainstorm>

[NickCraver]

@nickalbrecht this stuff may indeed change a lot. The problem is primitive deserialziation for things like regexes. I'm working on Stack Overflow search at the moment but tonight I'll get back to MiniProfiler. Both it and Exceptional are making the ASP.NET Core leap at the same time, so I'm iterating on both to see what works best. Check out current MiniProfiler progress for ideas on settings: https://github.com/MiniProfiler/dotnet/pull/210 Note that it doesn't load any from config yet because only a few things are simple primitives, but I'll get to adding that soon.

I'd appreciate any thoughts there, since you're very familiar with the -isms of ASP.NET Core as they've proven out so far this feedback is very useful. Thanks for all your time helping improve here! I'll get back to Exceptional and make a lot of updates in responses to the above after the next MiniProfiler alpha. Hopefully I can find a settings layout that works better, though existing web.config compatibility (otherwise brutal even for a major version change) is a concern here where it isn't with MiniProfiler.

[nickalbrecht]

I'll do that. I wasn't sure how much you were hoping/expecting to change with the revisions to get Exceptional (and also MiniProfiler) working in ASPNET Core. Knowing that a fair amount of the changes may flow back through to the ASPNET versions. However I literally laughed out loud at the first line in that issue on MiniProfiler. Hahaha

Let's be honest: this will break everyone.

[NickCraver]

@nickalbrecht I've opened #108 aiming to address most of this. IOptions<ExceptionalSettings> is in there.

Exception logging should be possible by calling one of the ILogger<>.Log() methods (if the Exception parameter is not null). Will likely need to adjust the way the ILogger instance is used inside of the error handler to not recursively call itself should there be a problem with the error handler itself.

Can you elaborate a bit here? I'm not against it, just unsure what the ask is with ILogger.

The settings need not be a POCO IMO, since it's programmatically accessible. I'm going to eliminate ConfigSettings shortly and replace it with a binder loop (which it's effectively doing in an inefficient way already) to simplify things. I'll do that on the #108 branch.

[nickalbrecht]

My assumption when working with ASPNET Core, after looking at all of the examples and templates, is that they are pushing the developer towards using ILogger abstraction for logging purposes. There are a bunch of extension methods and overloads for doing Warnings, Trace, Information, Debug, etc. as well as the root Log() method. All of these overloads have a signature where you can pass in an Exception. It made sense when looking at exceptional to try and take advantage of this, and potentially any existing code developers may have for hooking up logging. Not only would it abstract Exceptional out of the developer's code so that installation would simply be including the package and adding a line to the Startup class, but having exceptional as a logger means when an error it thrown, it could also display any other logged messages from the same request to help provide additional context for debugging.

I only recommend that the settings be POCO for ease of serialization, and facilitate easier refreshing of settings should the source of the config change. The way I saw the classes used with IOptions is that they are just strongly typed instances of the source, and are meant to be consumed by more stateful instances elsewhere in an application.

EDIT: That and by treating it as a POCO, you can keep things separate. Otherwise you end up putting a lot of other logic like Func<> and setup code inside of the ConfigureServices() method. Which for transparent reasons, should be just for setting up your ServiceCollection. The rest of that I'd rather have inside of the Configure method where you're actually dictating how requests should be handled.

[NickCraver]

@nickalbrecht I'm not sure I agree that ILogger makes sense, but perhaps I'm misunderstanding. Exceptional is 100% geared (and intended) to only log exceptions, not general messages. While I think hooking up to Error and Critical levels (if an exception is present) would be good, I'm not sure how you'd do that. It's my understanding the use resolves one logger via DI, and having one that only logs exceptions seems not all that useful, as it'd replace rather than compliment any general purpose ILogger.

Am I correctly understanding the intent and impact here, or missing what you meant entirely? I'm not discarding the idea of having such a an implementation, but I want to know what the actual usefulness is.

[nickalbrecht]

Oh gosh no. My understanding is that it's more along the lines of an event type system. Hence why when you creating a new ASPNET core app, It automatically adds a Console logger, and a Debug logger, you can also add others, and they each get the event. Hence the utility of an Exceptional logger. At minimum, I was thinking an Exceptional logger would only interact with calls to log that actually have an Exception, the rest aren't really relevant. Unless you want to buffer them, and then include them when if an Exception is logged, else discard the buffered information information from other calls to the logger in the same request/scope.

[nickalbrecht]

I tried to get something similar to this working on my end, but ran into a problem with trying to provide the HttpContext. I haven't had a chance to play with it further, but I wonder if DI can help on the Logger.

[nickalbrecht]

Just checking up on old issues. I've been using the updated version of Exceptional for a while and I don't think I have anything more to add to this issue. I can imagine there being value in being able to attach all logged messages when an exception occurs (per request), but the implementation/practicality of it is a totally different matter. Where would they live for requests that complete successfully? Memcache? DistributedMemCache? What memory implications are there if they are just recorded internally, and discarded when the DI container gets disposed? Does it scale well? Could it be a shared memory pool? Also there's a bit of overlap in this sort of idea with other services like Application Insights. So while I think it'd be a neat experiment, it's sort of outside of my the initial issue, which was making Exceptional more ASPNETCore friendly, and use the mechanism it exposes, which I believe has been addressed already.