search

NickCraver / StackExchange.Exceptional

Error handler used for the Stack Exchange network
https://nickcraver.com/StackExchange.Exceptional/
Apache License 2.0
862 stars 170 forks source link

V2 Planning - Ideas and Feedback Needed! #85

Closed NickCraver closed 5 years ago

NickCraver commented 7 years ago

This is a planning post for V2, which I've already started into in the v2 branch (needs VS 2017 15.3 Preview to work with). I've made pretty sweeping changes to settings locations to centralize them on the code side (old web.config sections still work!), and will continue to do so as I find a good solution for ASP.NET and ASP.NET Core both. I've also been working on the UI, code sharing etc. Let's break it down.

Here's a list of what I'm planning for V2:

That's just an overview of the stuff I already have in queue, for example here are what the new stack traces look like to a user (you can confiure it to looke like C#, VB, or F# syntax):

screen shot 2017-07-16 at 19 24 12

...and here's what the list page currently looks like:

screen shot 2017-07-16 at 21 26 15

...so that's what I've been working on. Now...

What functionality would you like to see in V2 for exceptional?

I'm not promising to implement all ideas, but I want to at least (hopefully) make all breaking changes necessary in a major release where they belong. I'm sure there are a lot of other great ideas (I know some co-workers have some that I'm forgetting right now) and I wanted to collect them all here so that none are forgotten. Please, share your ideas!

Also, as always, feedback on any of the above is welcome. I'm just trying to get a release out as personal time allows!

NickCraver commented 7 years ago

@alfeg There is now a Ignore.Header in settings that is respected for header replacements the same as Cookie and Form behave, added in 55345c8c58280bf3b6711e2d67fadc87b4db02f8. It'll be in the next NuGet release (and available on MyGet in a moment).

ctorx commented 7 years ago

BAM. Thanks for the heads up.

On Sep 8, 2017 7:00 PM, "Nick Craver" notifications@github.com wrote:

@ctorx https://github.com/ctorx Merged into master :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NickCraver/StackExchange.Exceptional/issues/85#issuecomment-328247477, or mute the thread https://github.com/notifications/unsubscribe-auth/AAU-59lFYi8YTpOIYGzvOOzirHWO6rbmks5sgfE2gaJpZM4OZe7z .

NickCraver commented 6 years ago

I added a bunch of testing in #111 (still a long ways to go, but way better than zero) and have pushed a new alpha release to NuGet. The 2.0.0-alpha2-00171 packages are now available for testing. For anyone willing to do so, thank you!

nickalbrecht commented 6 years ago

Ok, so I had to fight with this a bit. I'm not sure if this was new to alpha2, or if I broke this previously with the alpha1 bits and just didn't notice. But it's worth mentioning that when the docs get updated that the call to app.UseExceptional(); must appear after app.UseExceptionHandler("/Error"); (if you are using the ExceptionHandler). Otherwise, it appears that the exception handler will swallow the exception, and it never makes it up to Exceptional.

It's a little counter intuitive that if you call app.UseExceptional(); on it's own, or app.UseExceptional(); and services.AddExceptional();, that Exceptional still starts up correctly in both cases, and starts to log errors using all of the default settings. There doesn't seem to be any observable difference. It also doesn't read anything from appsettings by default. I can't find any example of the Microsoft project templates looking to a specific config section by default however. So, I'm not sure if there's a reason for this and it's a convention they don't want to condone? But if that's the case, and Exceptional will work fine without calling services.AddExceptional(); Then I would expect the only signatures that should be available would be these

services.AddExceptional(Configuration.GetSection("Exceptional")); // pull from settings only
services.AddExceptional(settings => { /* Manipulate settings here */ }); // pull from code settings only
services.AddExceptional(Configuration.GetSection("Exceptional"), settings => { /* Manipulate settings here */ }); //pull from app settings first, then apply code settings

Calling services.AddExceptional(); with no arguments shouldn't be allowed. Not because of any errors or unexpected behavior but to prevent people from thinking it's needed, or will somehow affect the default behavior.

Also, I noticed it's impossible to set the ApplicationName via code? Is this by design? Is it possible to use Exceptional in a multitenant app where the tenant name is used as the application name? I notice that StackExchange.Exceptional.Extensions.Log() will accept an ApplicationName programmatically, but setup/settings will not.

services.AddExceptional(null, settings => {settings.ApplicationName = "MyCoolApp"}); //Not possible
NickCraver commented 6 years ago

@nickalbrecht I'll look at the settings overloads with a clearer head in the AM - but a quick note: .ApplicationName was always specific to the store, so it's in the Store, e.g. .Store.ApplicationName in alpha2. Just wanted to unblock you there.

nickalbrecht commented 6 years ago

Can't get the SQL error store to work. Keeps using the default Memory Error Store (according to the footer of the Error Log page).

services.AddExceptional(Configuration.GetSection("Exceptional"));
app.UseExceptional();
{
  "Exceptional": {
      "Store": {
        "ApplicationName": "Uber Application",
        "Type": "SQL",
        "ConnectionString": "**MyConnectionString**"
      }
  }
}

The application name is picked up, but it seems to ignore the store type?

NickCraver commented 6 years ago

@nickalbrecht Are you able to try the latest on MyGet? I've just added a lot of fixes and tests for this in the latest commit. TL;DR: It's very likely an assembly in the domain blew up when loading types, causing no stores to get loaded at all, which explains why SQL in the core of .Shared isn't even included. The loading code is now resilient to this as of df45337bf7d34a537760d9d2cda9bda2c42e8336 (2.0.0-alpha2-00174 in packages).

nickalbrecht commented 6 years ago

Ok, so I'm using the package from MyGet. It all seems to be working fine at first glance. Will continue to play with it and see how it goes.

I stepped away from my desk for a bit however, and when I came back I noticed around 3 errors spread out in the output window of VS (all with the same details)

System.ArgumentNullException: Value cannot be null.
Parameter name: provider
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at StackExchange.Exceptional.Extensions.Log(Exception ex, HttpContext context, String category, Boolean rollupPerServer, Dictionary`2 customData, String applicationName)

I can't get them to happen again however, so I'm not confident it's an issue.

All of the links for Protecting/Clearing an error, and clearing all unprotected errors - seem to work fine. Though they do still give JS errors in the browser console, it doesn't seem to interfere with their primary function.

It's worth noting that in most cases the default for adding json files to the Configuration is to specify true for the value of the reloadOnChange parameter. However Exceptional doesn't support this as it uses IOptions<> for accessing it's settings. If you wanted to make it support dynamically reloading of the settings you'd have to use IOptionsSnapshot<> which will cause it to check if it needs to build a new instance at the start of each request. I don't imagine that the the settings for this would change very often, if ever, in a production environment, so I'm not sure the investment is needed? I wanted to mention that IOptionsSnapshot<> also exposes getting options instances by name, which allows you to request unique instances that are cached separately. However this is a problem because the last step in the internals of creating a settings instance for use in DI, is to call StackExchange.Exceptional.Exceptional.Configure which saves the resulting settings to a static instance needed for background error handling. This breaks the implementation for named instances of options, since the last instance requested would always overwrite the background settings. This isn't a big deal the way Exceptional is written now because it doesn't generate different settings by name (might be interesting for multi tenant usage of Exceptional at a future date). I don't see anyway currently to put in a check that if someone requests a named instance (as opposed to the default instance which has a name equal to string.Empty) to raise an error that it's not supported. But I wanted to mention it since the ability to request an instance of IOptionsSnapshot<ExceptionalSettings> from DI is possible by the developer using Exceptional even though it's not done internally. The implications of this is that if changes are saved to the AppSettings file, and an instance of the IOptionsSnapshot<ExceptionalSettings> class is requested (even if it's not used), the instance of the settings used for handling background exceptions would be updated and any background exceptions would use these new settings, while the singleton of the settings created at the start of the application lifecycle would be used for exceptions during a request. Changing the internal code to use IOptionsSnapshot<> would mean the behavior between the two scenarios stay the same, but you'd have to run some additional tests to make sure that it behaves as expected when this happens. Changes mid request from a different request/thread, etc.

Or just use Singletons registered to IServiceCollection directly, instead of using IOptions<>, since the main reason to use them, is for handling change notifications. You can still build up strongly typed objects from IConfiguration without using Configure<>() and IOptions<>() just by using var settings = ConfigurationBinder.Bind(config, new ExceptionalSettings());.

I had to reword that a few times, hopefully my final explanation still makes sense :-P

nickalbrecht commented 6 years ago

On a brainstorming note regarding my comment of expected overloads of AddExceptional(); After looking over the way the options work natively, letting you effectively compose the resulting options over multiple calls to Configure You could do that behavior and let them compose them over separate calls. But most of the examples regarding ASPNET Core don't do that once you're hiding the calls to Configure() with a central one-stop function for setup. They do tend to return a custom interface and chain further setup off of that however. Ultimately it seems to be more preference of the package author?

Option 1

services.AddExceptional(Configuration.GetSection("Exceptional"), settings => { /* Manipulate settings here */ }); //pull from app settings first, then apply code settings

Option 2

services.Configure<ExceptionalSettings>(Configuration.GetSection("Exceptional")); //start with configuration sources first
services.Configure<ExceptionalSettings>(settings => { /* Manipulate settings here */ }); //then apply code changes

Option 3

services.AddExceptional()
    .FromConfig(Configuration.GetSection("Exceptional"))
    .FromFunc(settings => { /* Manipulate settings here */ });

Option 4

services.AddExceptional(svcs =>{
    svcs.Configure<ExceptionalSettings>(Configuration.GetSection("Exceptional"));
    svcs.Configure<ExceptionalSettings>(settings => { /* Manipulate settings here */ });
});

I realize part of the reason it's done inside of the encapsulated method AddExceptional() is to ensure the order they are executed, and simplification. But I wanted to mention the other options because of my comments about named options and multi-tenant implications. For example... being able to do something with named options isn't possible when they are all behind a self contained AddExeptional() method. Granted there's nothing to say the developer can't NOT use the AddExceptional() and do it themselves, but after playing with the innards of ExceptionalServiceExtensions, ExceptionalSettingsExtensions and StackExchange.Exceptional.Exceptional.Configure, not everything needed is public, so this would not be possible

forgive any typos I'm just typing it and copy/pasting parts, but you get the idea

services.ConfigureAll<ExceptionalSettings>(Configuration.GetSection("Exceptional"));
services.Configure<ExceptionalSettings>("AdminPortal", Configuration.GetSection("AdminExceptional"));
services.Configure<ExceptionalSettings>("PublicPortal", Configuration.GetSection("PublicExceptional"));
services.PostConfigureAll<ExceptionalSettings>(settings => {settings.Email.ToAddress += "IT@example.com";});
//Edit: On second though, I'm not sure the sequential bindings to Configuration.GetSection works as expected in my head, but the idea is still there :-P
//Usage
var options = HttpContext.RequestServices.GetService<IOptionsSnapshot<ExceptionalSettings>>();
var adminOptions = options.Get("AdminPortal");

Might be better for 2.1, but it's worth thinking about to get an API surface/accessibility modifiers that doesn't need to change between versions

At some point I'm sure you're going to be thinking "Damnit Nick, stop poking holes already!" :-P

NickCraver commented 6 years ago

@nickalbrecht I appreciate the thoughts - all of that makes sense as stated, thanks! The tradeoffs there are complexity and in the snapshot case, allocations - both of which we want to avoid. I think I'm most comfortable with the current setup and let's see id we need to add additional options later.

I'm currently considering registering the HttpContextAccessor for users if they use the ILogger as this would be needed for context (and possibly or even likely registered already). I'd hate to do this globally though, since there's still some performance tradeoff across async boundaries due to AsyncLocal backing.

I'm thinking .AddExceptionalLogger(LogLevel minLogLevel) as an extension method to register the accessor singleton and the Exceptional logger itself, which only logs when there's an exception present. What do you think?

nickalbrecht commented 6 years ago

Sounds good. Just wanted to make sure you were aware of the potential hurdles so that it's easier to triage should something come up.

I had actually added that to my app in the past but it's commented out now and it all still works. So now I'm trying to figure out if it's included by default. Created a brand new ASPNET Core 2 web app, and doing HttpContext.RequestServices.GetService<Microsoft.AspNetCore.Http.IHttpContextAccessor>() works out of the box. So I don't think you'll have to add it.

The Logger method sounds good. The only catch would be making sure the implementation would ignore calls to log errors from within the StackExchange.Exceptional namespace to prevent recursive calls. It might also be worth seeing if using AddExceptionalLogger() is as good at capturing errors as the current method of adding it to the middleware pipeline is. If they are about the same, you could get away with not adding it to the pipeline and using the logger instead, as I imagine the two methods in tandem may result in duplicates being recorded.

NickCraver commented 6 years ago

Created a brand new ASPNET Core 2 web app, and doing HttpContext.RequestServices.GetService<Microsoft.AspNetCore.Http.IHttpContextAccessor>() works out of the box. So I don't think you'll have to add it.

Were you using Identity? That's the big often-used item that adds the context accessor. Some or most will have it, but not all. Those after extreme perf (e.g. here at Stack Overflow) won't have it registered, so can't assume :)

nickalbrecht commented 6 years ago

You're right, it's not resolving when I do it in a new project with no authentication. So it'd have to be either added in the setup of Exceptional using the services.TryAddSingleton<>(), or indicate in the documentation that it must be done by those consuming the Exceptional packages. On the plus side, I believe the performance hit is only when it's used, simply having it in the DI container doesn't implicitly cause any performance side-effects. Since it'd only be needed when it's trying to log an exception (which ideally is rare), then it shouldn't be too bad.

How much of an impact does it have performance wise?

I think there's a bug on the list of Exceptions in the current alpha version. The URL column is always empty, even though in the details the value is there?

NickCraver commented 6 years ago

@nickalbrecht The weight is from registering it, regardless of usage. The weight is all in the AsyncLocal context switch actions in every state switch. There have been great performance improvements around this in 2.0, though.

I'm going to assume you're using SQL if hitting the Url column bug. I pushed a fix for this to MyGet several hours ago if you want to give it a go :)

nickalbrecht commented 6 years ago

Really? Wow, ok. Well that means I get to start refactoring some code to not rely on it so much since it was so easy to use.

Yup I am. Sweet! I'll grab the update :-)

mmillican commented 6 years ago

Finally had a chance to upgrade our projects to ASPNET Core 2.0 and installed Exceptional in one of them so far. Using the In Memory store, there don't seem to be any errors. Thanks @NickCraver!

mmillican commented 6 years ago

Using the latest version on Nuget (2.0.0-alpha2-00171) and setting store to SQL, it's still saying it's using the memory store.

Here's my config:

appsettings.json

"Exceptional": {
    "Store": {
      "ApplicationName": "APP NAME",
      "Type": "SQL",
      "ConnectionString": "Server=***;Database=***;User ID=***;Password=***;"
    },
  },

Startup.ConfigureServices()

services.AddExceptional(Configuration.GetSection("Exceptional"), settings =>
{
    settings.UseExceptionalPageOnThrow = HostingEnvironment.IsDevelopment();
});
nickalbrecht commented 6 years ago

That's exactly how mine is configured, and it's working correctly. I'm not sure what else you might be missing? It's worth noting you might want to upgrade to 2.0.0-alpha3-00180 and see if that resolves your issue. It might be related to an issue @NickCraver's already fixed.

NickCraver commented 6 years ago

Alpha 3 is now on NuGet. @mmillican the latest libs should resolve the loading error that @nickalbrecht is referencing above.

nickalbrecht commented 6 years ago

I noticed today that emails aren't working with Exceptional in my ASP Net Core project, and haven't for a while by the looks of it? I'm not sure. To try and debug this, I'm using the most recent version of the source from here <658888e>. I went digging and it looks like when ProcessNotifications() runs, the notifiers collection is empty when it checks at ErrorStore.cs:180. So I went looking for what populates it and found that there's a Register() method that supposed to populates it at ExceptionalSettingsBase.cs:68, but it's not used in any of the code, only in the sample project for Console. I'm trying to get it to work with the AspNetCore sample project. Am I missing something or was this a bug introduced somewhere along the line?

NickCraver commented 6 years ago

@nickalbrecht ...oops? Yeah this never worked in ASP.NET Core but does in ASP.NET non-Core. I've just pushed fixes up for that and new packages should land on MyGet shortly. Thanks for the heads up!

nickalbrecht commented 6 years ago

Awesome. I updated to build 00185 from MyGet and it looks like it's works as expected now. Thanks :-)

nickalbrecht commented 6 years ago

Also, did something change with the recorded server variables? It used to record the Name of the current user from the HttpContaxt.User.Identity property, but it doesn't seem to be recorded any more? Not sure if this is unique to ASP.NET Core or not, I haven't tested the non-Core version.

Update: I was able to get the same functionality working with ASP.NET Core however using an ExceptionFilter and some logic to record Exception.Data into the custom exception info. Not sure if it's as easy on ASP.NET though?

public class UserNameExceptionFilterAttribute : IExceptionFilter
{
    public void OnException(ExceptionContext context)
    {
        context.Exception.Data.Add("UserName", context.HttpContext.User.Identity.Name);
    }
}
public void ConfigureServices(IServiceCollection services)
{
    services.AddExceptional(Configuration.GetSection("Exceptional"), x =>
    {
        x.GetCustomData = (exception, dictionary) =>
        {
            foreach (var key in exception.Data.Keys)
            {
                dictionary.Add(key.ToString(), exception.Data[key].ToString());
            }
        };
    });
}
NickCraver commented 6 years ago

You can also include any keys easily in the regex, for example to log any key containing "UserName", it'd be:

public void ConfigureServices(IServiceCollection services)
{
    services.AddExceptional(Configuration.GetSection("Exceptional"), x =>
    {
        x.DataIncludeRegex = new RegEx("UserName", RegexOptions.Compiled);
    });
}
nickalbrecht commented 6 years ago

Would this work to simply specify a regex that should match any Data item on the exception?

x.DataIncludeRegex = new RegEx(".*", RegexOptions.Compiled);
NickCraver commented 6 years ago

@nickalbrecht yep, it would indeed. We've been running with no problems at Stack Overflow now for a few weeks, so I'm promoting the packages to beta at this point. I want to revisit the ILogger stuff as soon as time allows, but since that's not a breaking change, we can just add at any time which is nice. I'm trying to catch up on so many OSS projects, so just doing what I can as time allows.

nickalbrecht commented 6 years ago

Haha, yeah I can only imagine.

Some brainstorming on ILogger I'm not 100% positive on how it would work to use ILogger to monitor for exceptions. Just because of trying to prevent recursive calls to the logging method by using an ILogger instance inside of Exceptional? That being said, there is a Category on the ILogger, which is typically the fully qualified namespace. So you could use that to prevent Exceptional from trying record the details into the store when it's an exception from inside of Exceptional. That would combat the recursive calls. Would you then use an object hashcode of the exception to determine if the error has been recorded already outside of the ILogger to prevent duplicates?

However I can see if being useful to introduce a way to call Exceptional as part of the middleware pipeline, and create a logging scope for the request. Or maybe have it implicitly done with a LoggingProvider that logs to a backing collection on the HttpContext? Not sure which is better, LoggingProvider backed by the HttpContext, or a LoggingScope, or both? Then when an exception is written into the store, the logged messages up until that point could be retrieved from the scope/context, and included. Maybe a new section along side Server Variables, Cookies, Form, etc. Sort of like recording the old trace messages from classic asp.net? Be nice way to include extra debugging info that would be recorded in Exceptional, as well as any other logging providers that might also be connected, like console, or Azure. Though It'd be worth noting that I BELIEVE the logger process is asynchronous. So while unlikely, it's possible to have pending logging calls relevant to the exception, that are not yet recorded to the backing store of the ILogger. Unless the recording of the exception into the exceptional store could be deferred/delayed a little, maybe triggered by disposing of the loggingscope if that is possible. Would it make sense to defer writing to the backing store until after the HttpContext lifecycle ends? (IF it has an HttpContext anyway). Is Exceptional asynchronous?

mmillican commented 6 years ago

OK, I'm missing something here I think. I just upgraded to the latest Nuget version (2.0.0-rc1-00187 for Core) and it solved my SQL store issues, which is great.

However, if my environment is Production, nothing gets logged. Is it because of this?

settings.UseExceptionalPageOnThrow = HostingEnvironment.IsDevelopment();?

Note, when in production, I'm still seeing the standard .NET error page, which I would expect, but not sure if that's somehow swallowing the exception before it gets logged?

stijnherreman commented 6 years ago

@NickCraver Any timeframe for RC2? No commits for a while now, so I assume the changes after RC1 are stable?

Edit: whoops, only just found out about the MyGet feed.

KiarashS commented 6 years ago

@NickCraver, These features can be very useful:

nickalbrecht commented 6 years ago

The first one I can see being useful, but while I think you can override it to use your own page if you want to customize it, It'll take a bit of work to do. I'm not sure your second suggestion is needed. Sounds an awful lot like a browser back button :-P

DaniilSokolyuk commented 6 years ago

@NickCraver When i configure Exceptional with https://github.com/NickCraver/StackExchange.Exceptional/blob/0529002f90806f0215e11d88cba3a6b51354c64a/src/StackExchange.Exceptional.AspNetCore/ExceptionalServiceExtensions.cs#L29 values from action is not setted in Statics.Settings and LogNoContext using InMemoryStorage :(

KiarashS commented 6 years ago

@nickalbrecht @NickCraver Although the second suggestion is more user-friendly. The first one is very useful. That's awesome to rapidly and easily add a customisable link in top of Errors listing and Error Detail pages , for example add a Back to Admin Dashboard link.

Two options must be add to Exceptional options with overridable values, for example href and text options. Also default values can be set to: href: root address (/) text: Back to Home

NickCraver commented 6 years ago

@DaniilSokolyuk do you have an example? (A separate issue will be best) - I missed it in this thread but happy to help and want to be sure that's working correctly.

KiarashS commented 6 years ago

@NickCraver Add support for capturing logs of ASP.NET Core logging infrastructure with configurable log levels. As a suggestion, Error and Critical levels can be considered as default.

129

NickCraver commented 5 years ago

Since v2.0 is live and #129 is captured separately, I'm going to close out this issue.

Thanks again to everyone who gave feedback and help to make this release happen. It's very much appreciated!