Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V2 Planning - Ideas and Feedback Needed! #85

Closed
55 of 57 tasks
NickCraver opened this issue Jul 17, 2017 · 86 comments
Closed
55 of 57 tasks

V2 Planning - Ideas and Feedback Needed! #85

NickCraver opened this issue Jul 17, 2017 · 86 comments
Milestone

Comments

@NickCraver
Copy link
Owner

NickCraver commented Jul 17, 2017

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:

  • netstandard2.0 build
    • StackExchange.Exceptional.Shared
    • StackExchange.Exceptional.AspNetCore
  • ASP.NET Core support (it'll be middleware, via a .UseExceptional() setup call)
  • Move shared code to .Shared library that StackExchange.Exceptional and StackExchange.Exceptional.AspNetCore depend on
  • Move logging methods from static to extension (this approach works in ASP.NET and Core, since they have different primitives)
  • Consolidate settings
    • Move all shared settings to StackExchange.Exceptional.Shared
    • Figure out GetCustomData and where it lives
    • Move IP parsing to IPNet (a utility class I created for Opserver)
      • Support IPv6 parsing/logging
  • All pages
    • Remove all images (replace with SVGs and CSS3)
    • Simplify pathing for module relative renders
    • Add SRI to CSS & JS includes
    • Embed jQuery (remove Google CDN dependency)
    • Styling overhaul (.less files with simpler, more consistent, and purpose-based styles)
    • Combine .js (via Web Essentials) to Bundle.js
    • Move all embedded resources to .Shared for consistency and DRY between libs
  • Error detail page (meta item for below)
    • More readable stack traces
      • Colorization of stack traces
      • Make generic arguments more readable (e.g. <T1,T2> instead of `2
      • Collapse async stack traces
      • Make this reusable (Utils.StackTrace.PrettyHtml(string stackTrace) method)
      • Linkify SourceLink source (configurable!) with GitHub links supported in the default options (e.g. click to go to source for the exact commit)
    • Add Google's Prettify for code blocks
    • Remove RazorGenerator dependency (uglier, but far simpler)
    • Remove System.Web dependency
  • Async all the things (since most stores of any cost will consume I/O)
    • Operational methods: Delete, Protect, etc.
    • Get/GetAll
    • Entire IHttpModule
    • Rendering pipeline for pages
    • Log()/LogAsync() equivalents
  • Add a .AddLogData() extension for easily adding custom data
  • Remove *List suffixes from Delete/Protect methods. Just make them overloads that take IEnumerable<Guid>
  • Remove "SQL" logging, and replace it with configurable many "Commands" logging (.Data["SQL"] will be recognized by default) - for example Redis commands, Elastic queries and responses, etc.
    • Make sure SQL still acts as a deserializer set for viewing old exceptions
    • Remove SQL column for SQL store scripts
    • Provide upgrade script instructions in an upgrade notes section
  • Add a LastLoggedDate column/property to fix rollup issues, e.g. roll up every 10 minutes (as before) but show the actual last date it occured in the UI, rather than the start of the rollup
  • Add a Category (string) field to the exception for a variety of use cases (categorization, severity, etc.)
  • Create /docs (GitHub pages, like miniprofiler.com/dotnet (source)
    • Add .NET section (e.g. console app)
    • Add .NET Core section
    • Add ASP.NET section
    • Add ASP.NET Core section
  • Providers
    • In-Memory
    • JSON
    • SQL Server
    • MySQL
    • Postgres
    • MongoDB
    • Redis
    • Elastic

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!

@captncraig
Copy link

captncraig commented Jul 17, 2017

I have an odd perspective on this. My desire is to ship go errors straight to the db, and view them via opserver. A few things I'd love to see for that, recognizing I'm not going to be a top-level supported use case:

  1. Good documentation on database schema, what columns are optional vs required, shema for the json blob if that stille exists, and so forth.
  2. Are the ui bits fairly standalone? Is there a documented api it uses? In an optimal case I could serve some html, replicate some json endpoints (with the same sql queries used on the .net side), and have a working ui in my go app for little effort. If the ui is more .net centric cshtml type stuff though, maybe that's not so easy.
  3. Is the stack trace rendering / source linking general enough that it could handle a go stack trace? If it were extensible enough, I'd certainly be willing to work on a go dialect parser that could handle it, assuming it could handle plugins in that kind of way.

@CptRobby
Copy link

CptRobby commented Jul 17, 2017

I like what you're doing, @NickCraver

For the LastLoggedDate, I've got a version of that done already. I could port it over to work with your v2 setup if you like.

One other thing that I've done for my own use (though I have thought about releasing it as an independent add-on package) is I created an ErrorStore that doesn't actually store the error, but relies on a user created object to provide the ability to store and retrieve the errors. That probably sounds confusing but my use case for this is that I wanted to be able to store the errors from within my EntityFramework context (which is why I called my custom ErrorStore "EntityErrorStore", even though it doesn't actually use or depend on EntityFramework and could instead be used to store Errors using whatever method the user could think of.) Besides the EntityErrorStore class, it also defines two interfaces: IPersistenceProvider and IPersistedError. IPersistenceProvider defines the methods that need to be implemented by the user created class that handles storing/retrieving the errors to/from the ultimate store (an EF context, in my case). The methods largely imitate the abstract methods of ErrorStore, except that instead of returning Error objects, they return IPersistedError objects. IPersistedError is a slimmed down version of Error (GUID, FullJson, ErrorHash, ApplicationName, DuplicateCount, LastLoggedDate, DeletionDate and IsProtected, with only the GUID and FullJson properties being absolutely required). IPersistedError is then implemented by my POCO EF CodeFirst representation of the error in the database. At runtime, the EntityErrorStore is configured as the default store and is given a reference to the implementation of IPersistenceProvider (though it will also try to find a suitable type in the ExecutingAssembly if it hasn't been given one before it needs to use it). For any method on the IPersistenceProvider called by EntityErrorStore where it gets back an IPersistedError, it will create an Error object using the FullJson property and then it sets the other properties based off of the other properties of the IPersistedError (IE DuplicateCount, IsProtected, etc. that are stored outside of the Json).

Sorry for the above wall of text. I just couldn't think of a simpler way of explaining it. Let me know if you're interested in it though. 😉

@NickCraver
Copy link
Owner Author

NickCraver commented Jul 17, 2017

@captncraig I think the story there would have to be go sending an error to an endpoint (I'd be happy to package an ASP.NET Core endpoint in the Exceptional repo) that logs to the DB, accepting JSON (or Opserver could do this). In short, it doesn't make sense not to have .NET in the flow if we're using it to view errors anyway. That and Go's packaging story is a complete trainwreck, so there's no "update the library" when the schema changes, it's a lot more work, a lot more practical skew, and just more problems overall. But throwing JSON fields that rarely change: that we can work with. It's actually how we supposed Node.js back in the day.

There's a related issue here: .NET will have n storage providers. There already exists SQL, MySQL, Postgres, and I'll likely add Elastic and Redis in v2. Aiming directly at one of these doesn't make Exceptional multi-platform, but having an HTTP API would.

The stack trace aspect is an interesting one, as that'll need some handling. The prettification is very much for .NET format stack traces, conventions, classes, etc. It's not pluggable nor could it be. The overall pluggability of which prettifier to use is a thing maybe, but the error would have to indicate which platform it was from somehow. I'd hate to record ".NET" a billion times in a table for most shops, but an enum doesn't work. Recognizing stacks or conventions seems brittle, thoughts?

As for the individual questions:

  1. Covered above - these will change and it's the change itself that makes this a problem, especially in Go until packaging gets a good story.
  2. They're very much embedded in the lib. A .NET platform can make use of them, but others cannot.
  3. Also above - we'd have to put in a different parser for each platform, which has it's own set of issues. We'd need to be able to identify the platform first, though.

@NickCraver
Copy link
Owner Author

NickCraver commented Jul 17, 2017

@CptRobby I've got the date bits locally, it's pretty simple - not worried about that piece. Thanks though!

I admit I'm not following the rest - the abstraction is what ErrorStore should already provide, what can't be done with just an ErrorStore? It controls the serialization, de-serialization, storage, etc. Why couldn't all that be done just implementing a new store? I'm sure I'm missing something...help me out here?

@CptRobby
Copy link

@NickCraver No problem. I just thought I'd offer to help on that part first since it was a low hanging branch for me. Got any other pieces I could try my hand at? 😄

Regarding the rest, sorry about the confusion. The key point that I used was the IPersistedError interface. That allowed me to create a POCO class (which I just called PersistedError) in my collection of Entities that simply implemented that interface (inheriting from Error wouldn't work very well because there are a lot of other properties that I would have to get EntityFramework to ignore). This provides for an abstraction layer where my class that implements IPersistenceProvider handles all the communication with the database through my DbContext (EntityFramework starting point) while the EntityErrorStore provides all of the ErrorStore functionality by communicating with my IPersistenceProvider.

I probably could have just done it all from within my website's code without the abstraction layer. Then I would just have a class that inherited from ErrorStore that knows how to use my DbContext to get PersistedError objects and then convert them to Error objects and all of that. It would be very tightly coupled, obviously. And there would be the downside that I couldn't configure it in web.config because the ErrorStore.GetErrorStores method only finds classes that are in assemblies that match the file name pattern "StackExchange.Exceptional*.dll". The way I did it, they were in a separate assembly called "StackExchange.Exceptional.Entity.dll", and I could just configure it in web.config with <ErrorStore type="Entity" />.

The reason I wanted to use EntityFramework for storing the errors was twofold. First, I wanted to tweak the structure of the table a bit without having to modify the SQL strings in the SqlErrorStore. And secondly, I just prefered having a single pathway to the database, controlled by a single connection string.

Hopefully that helps explain why I did it the way I did. I'm not expecting you to take this and do the same thing, that would probably be pointless. If you did want to take anything away from this though, you could change the ErrorStore methods (GetError, GetAllErrors, etc) to use an interface instead of the Error class. And when you need to make sure you have an Error object, you can just pass it to a method that would check to see if it is an Error then just return it as a cast, otherwise create a new Error from the FullJson property and set the other properties based off of the passed in IError. This would remove some of the need for the abstraction layer mentioned above.

I know though that it would largely be pointless for the majority of users who just use the built in error stores, so I'm not really expecting anything. Just sharing with you how I've been using it. 😉

NickCraver added a commit that referenced this issue Jul 18, 2017
Addresses comments in #85 and adds flexibility.
NickCraver added a commit that referenced this issue Jul 18, 2017
Rleated to #85, this adds the last date an exception occured. While CreationDate is needed for de-dupe rollups, we want to show the actual last time it happened in the dashboard...this does that.

Schema scripts for SQL and MySQL also made to be re-runnable and update schemas to v2 (more changes incoming).

INFORMATION_SCHEMA is used in SQL to make it more portable for more platforms later.
@NickCraver
Copy link
Owner Author

@CptRobby Understood! I've just push a commit that changes things up. The original reasoning was based on current plans for ASP.NET Core and AppDomain which have pulled a 180 since. I changed the implementation to look at all assemblies in the domain for any inheritors of ErrorStore. I hope that'll make life much easier for your scenario in v2 :)

@CptRobby
Copy link

CptRobby commented Jul 19, 2017

@NickCraver I saw the commit. My only question regarding the change is how it would behave with the MySqlErrorStore (for example). If there are no references to StackExchange.Exceptional.MySql.dll from a website project, would it still be loaded in the AppDomain just because of it being in the website's bin folder?

The situation I'm thinking about (convoluted yes, but not unreasonably so) is a user who has their website all set up and running. It currently is using the SqlErrorStore and is just configured in web.config. The user decides to switch to using a MySql database for storing the errors and isn't interested in changing anything else. So the MySql database is set up, StackExchange.Exceptional.MySql.dll is dropped into the bin folder and web.config is edited. With the way the stores are currently loaded, I know that this would work. Would it work with the change that you just made?

I honestly don't know the answer. I tried searching to see how assemblies are loaded in ASP.NET, but got conflicting answers. Obviously if the AppDomain just loads all assemblies from the bin folder when it starts up, then it would start working as soon as the AppDomain is reloaded, which would probably happen as a result of editing web.config.

Also, this question needs to be considered for native .NET applications. One of the results that I found said that assemblies are loaded differently in ASP.NET from the way they're loaded in native .NET applications. If in either case the assemblies aren't just loaded automatically without any code references, then maybe just add a section before the point where the loaded assemblies are searched to make sure that any files in the current folder that match "StackExchange.Exceptional.*.dll" are loaded.

@CptRobby
Copy link

CptRobby commented Jul 19, 2017

Just found a really well written blog post by Rick Strahl that seems to answer the question definitively: https://weblog.west-wind.com/posts/2012/Nov/03/Back-to-Basics-When-does-a-NET-Assembly-Dependency-get-loaded

In that article, it says that "ASP.NET pre-loads all assemblies referenced in the GAC assembly list and the /bin folder." But for .NET applications, the JIT compiler avoids loading any assemblies until the very moment they are needed. This means that to enable the easy drop-in approach for .NET applications, we should probably insert the following code:

var path = Path.GetDirectoryName(typeof(ErrorStore).Assembly.Location);
//Note that I added another . before the *, since we know that we already have
//StackExchange.Exceptional.dll loaded
foreach (var filename in Directory.GetFiles(path, "StackExchange.Exceptional.*.dll"))
{
    Assembly.LoadFrom(filename);
}

This will ensure that StackExchange.Exceptional.MySql.dll (or any other assembly following the naming convention) is loaded even though it isn't referenced in code.

@mmillican
Copy link
Contributor

I really like the idea of having an API that we could use to log from other platforms (ie mobile apps). Our devs have been asking for this for a while to centralize our error logging.

One question though, where would this "API" (or single endpoint rather?) live? In an application that uses Exceptional? Or would we essentially spin up a new instance of it to act as just an API?

I'd be happy to help contribute to this if it's a route you'd like to pursue.

While not straightforward, depending on how you were planning on the Category field being implemented, I could see that possibly being used for the "source" (as referenced in this comment but that doesn't really fix the issue of having it recorded millions of times.

@NickCraver
Copy link
Owner Author

@CptRobby I concur, addressed in 346bbde - thanks!

@minhhungit
Copy link

Can we implement a custom module or a simple module which we can add it into dashboard page with simple information

@CptRobby
Copy link

@minhhungit What type of module are you talking about exactly? Just saying that you could add it into a dashboard page doesn't help since there are numerous types of dashboards (including StackExchange's Opserver).

And are you asking for an API to allow you to build a custom module or are you asking to be given a module ready to go?

@minhhungit
Copy link

minhhungit commented Jul 27, 2017

@CptRobby some APIs that we can create a simple panel to contain a list top error as table, and link it to exception main page when we click on them (we also should have setting to disable the link)
Dashboard is just a simple html page, lol

@NickCraver
Copy link
Owner Author

Thanks for the help everyone (especially contributors!), 2.0 alpha packages are now up on NuGet: https://www.nuget.org/packages?q=StackExchange.Exceptional

Special props to @StefanKert on lots of work for ASP.NET Core configuration and @CptRobby for fixes in 2.0 as well.

@nickalbrecht
Copy link

Just a heads up, your example and documents claim you can call app.UseExceptional(Configuration.GetSection("Exceptional")); without the second parameter to further apply configuration in code, but the method actually throws an error if the second parameter is null.

In order to exclusively pull the configuration from the appsettings file , you have to use
app.UseExceptional(Configuration.GetSection("Exceptional"), config => { });

I'm actually trying to re-add Exceptional to my project, since I was unable to keep it when I ported an existing project from ASP.NET to ASP.NET Core 1.1. It'll be nice to have it back again.

@nickalbrecht
Copy link

Also, this works.

app.UseExceptional(Configuration.GetSection("Exceptional"), config => { config.UseExceptionalPageOnThrow = true; });

but this does not
app.UseExceptional(Configuration.GetSection("Exceptional"), config => { });

"Exceptional": {
    "UseExceptionalPageOnThrow": true
}

NickCraver added a commit that referenced this issue Aug 15, 2017
@NickCraver
Copy link
Owner Author

@nickalbrecht Thanks! I fixed both of those just now, they'll appear on NuGet shortly. Please keep sending any issues you hit!

@nickalbrecht
Copy link

On a more feedback/suggestion note, versus bug - is it possible to use a Func<HttpRequest, bool> for a UseExceptionalPageOnThrow check (or some variation of it), to allow the possibility of letting certain users view the Exceptional error page, otherwise it falls through to the standard friendly error handler (or whatever else is setup)? It'd be awesome to be able to call IAuthorizationService.AuthorizeAsync() and pass the Exception as the Resource parameter to see if they are are allowed to view details for that type of error. Though that might require the signature to be something like Func<HttpRequest, Exception, bool> unless Exceptional could just take the Policy name as the parameter and it calls AuthorizeAsync() (which would add a dependency of Microsoft.AspNetCore.Authorization.

I know the MiniProfiler uses Func<HttpRequest, bool> for checking things like ResultsListAuthorize, ShouldProfile, ResultsAuthorize, etc.

The use case for me is that I have razor views for certain tasks stored in a database that an end user can alter (email templates for example), and it'd be nice to let them see their own errors when the error is due to the dynamic compilation, but not let them see exceptions core to the application we provide to them.

@ctorx
Copy link

ctorx commented Aug 15, 2017 via email

@nickalbrecht
Copy link

nickalbrecht commented Aug 15, 2017

I've got the Exceptions list working but nothing else. Any of the child 'KnownRoutes', I was getting a 404 error on.

Here's what I have, since it contributed to my problem, and might help others.

I have an ErrorsController that handles feedback for BadRequest (400), Forbidden (403), and NotFound (404), etc. I've added a new action for Exceptional as suggested

[AllowAnonymous]
[Route("~/errors/[action]")]
public class ErrorsController : Controller
{
    public async Task Exceptions() => await StackExchange.Exceptional.ExceptionalMiddleware.HandleRequestAsync(HttpContext);

    [Route("~/NotFound", Name = "NotFound")]
    [Route("~/errors/404", Name = "NotFoundCode")]
    public new IActionResult NotFound()
    {
        return View("Error_404");
    }

    /* All my other actions here */
}

And this is wired up in the Startup.cs under Configure()

app.UseExceptional(Configuration.GetSection("Exceptional"), config => { config.UseExceptionalPageOnThrow = true; });
app.UseStatusCodePagesWithReExecute("/errors/{0}");

I can view the exceptions at ~/errors/exceptions/, but since CSS and JS failed, there's no styling on the page and no JS works. Clicking on any of the errors redirects me to a url that looks like ~/errors/exceptions/info?guid={ID HERE} but that's a 404 as well.

Not sure what's wrong here?
UPDATE:
Because I had a route attribute on my ErrorController that customized the route, I essentially broke the routing for Exceptional beyond the root Exception list. If you want to do the same as what I did above, be sure to decorate your Exceptions that Exceptional asks for, with a catch all at the end. For example, mine now looks like this.

[Route("~/errors/exceptions/{*resource}")]
public async Task Exceptions() => await StackExchange.Exceptional.ExceptionalMiddleware.HandleRequestAsync(HttpContext);

Using the word 'resource' in the route is not important, it just needs to have something there. It's the * that is important to mark anything after the action name as optional. Just using {*} without a name is not valid.

@nickalbrecht
Copy link

nickalbrecht commented Aug 15, 2017

@ctorx The problem is that introduces a dependency on Bootstrap and jQuery (already used). If you need to customize the look of these pages you're better off to implement your own handler for the call to view exceptions to replace the call to public async Task Exceptions() => await StackExchange.Exceptional.ExceptionalMiddleware.HandleRequestAsync(HttpContext); you would place in your controller.

Since the logic for rendering the page's content is in StackExchange.Exceptional.Shared and needed to be generic, you'd even be able to customize it more by overriding it since you be able to more tightly couple it to ASP.NET or ASP.NET Core depending on what you're using. Like using RazorPages.

UPDATE:
This got me curious, so I went and implemented this with ASPNET Core, it's RIDICULOUSLY easy to do with razor pages. I'm actually really surprised.

It is not PERFECT. I didn't place a version number at the bottom of the page, and the ellipse character is messed up. It also ONLY replaces the main List page, Info would be a separate page, and all of the other known routes for JS are still handled internally to Exceptional. But still.

https://gist.github.com/nickalbrecht/8f0c8f8cf82906049eb1e266f81e6f59

Note: This is not a new list. I basically translated @NickCraver's StringBuilder implementation out to a cshtml file

@nickalbrecht
Copy link

It's also worth pointing out that while this is denoted as 2.0, a lot of the changes are only refactorings to get a reusable Shared package, and have the AspNet/AspNetCore packages take care of wiring up to their own respective pipelines. It doesn't use some of the features you might expect to be available when using AspNetCore. For example, there is no instance information registered with DI, settings are still stored on a static property on the Settings class called Current. Also, be aware that the AspNet Core implementation is a middleware. It will not record exception information recorded by calling the methods on an instance of Microsoft.Extensions.Logging. ILogger<>. Instead there are extension methods that hang off of Exception instances, Log().

This is not to criticize, more to point out to others who have been using AspNet Core for a while, and might expect these aspects to be available.

Would it be possible to look at exposing the ConfigSettings type to not be internal? Or provide a way to populate an instance of Settings without having to call the builder extension methods that also wireup the middleware? I tried to implement my own take on wiring up the middle ware to try and test getting the UseExceptionalPageOnThrow logic to use more than just a boolean, but it's made more complicated by the class being internal, and no way to populate the settings without it. As it stands, all I can think of is to call UseExceptional, remove the ExceptionalMiddleware immediately afterwards, and then add my own just so that I can override the Invoke() method to flesh out that if statement. Or alternatively (and likely a better separation of concerns) separate the behavior of wiring up the middleware for recording exceptions from the Exceptional variation of the developer exception page so they are distinct? Then we could do something like this.

app.UseExceptional(Configuration.GetSection("Exceptional"));
app.UseExceptionalPageOnThrow();

@nickalbrecht
Copy link

nickalbrecht commented Aug 16, 2017

I'm trying all of this on AspNetCore, not sure if its a contributing factor. Wanted to mention a few other bugs before I head home.

Clicking on the lock icon to protect an exception and clicking the delete icon both work, but the response results in a Javascript error. Error details are from Chrome. IE gives an error too though it complains about expecting ';'

VM146:1 Uncaught SyntaxError: Unexpected token :
    at ir (js?v=bqZI4yofKeBK6AnXz7kIBjLmjYvI+Ywn4rLXWJPjJb6r7hqqKEyE2XgzVKLAO64cyzfdMoVjHL0l3nWHkF08lQ==:2)
    at Function.globalEval (js?v=bqZI4yofKeBK6AnXz7kIBjLmjYvI+Ywn4rLXWJPjJb6r7hqqKEyE2XgzVKLAO64cyzfdMoVjHL0l3nWHkF08lQ==:2)
    at text script (js?v=bqZI4yofKeBK6AnXz7kIBjLmjYvI+Ywn4rLXWJPjJb6r7hqqKEyE2XgzVKLAO64cyzfdMoVjHL0l3nWHkF08lQ==:2)
    at ho (js?v=bqZI4yofKeBK6AnXz7kIBjLmjYvI+Ywn4rLXWJPjJb6r7hqqKEyE2XgzVKLAO64cyzfdMoVjHL0l3nWHkF08lQ==:2)
    at b (js?v=bqZI4yofKeBK6AnXz7kIBjLmjYvI+Ywn4rLXWJPjJb6r7hqqKEyE2XgzVKLAO64cyzfdMoVjHL0l3nWHkF08lQ==:2)
    at XMLHttpRequest.<anonymous> (js?v=bqZI4yofKeBK6AnXz7kIBjLmjYvI+Ywn4rLXWJPjJb6r7hqqKEyE2XgzVKLAO64cyzfdMoVjHL0l3nWHkF08lQ==:2)

Clicking on the "Clear all non-protected errors" link at the bottom of the page results in an InvalidOperationException

Incorrect Content-Type
   at Microsoft.AspNetCore.Http.Features.FormFeature.ReadForm()
   at StackExchange.Exceptional.ExceptionalMiddleware.<HandleRequestAsync>d__6.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at XXX.Controllers.ErrorsController.<Exceptions>d__0.MoveNext()

Lastly, once you protect an exception, you can't un-protect it.

@NickCraver
Copy link
Owner Author

@nickalbrecht Thanks for all the feedback, lots of good stuff here. Would you mind filing the "things I'd expect in ASP.NET Core" as an issue? Let's enumerate those, I agree things like ILogger and such should be implemented and having a good list of these types of issues would be awesome.

I'll think about the Func for the exception page, the problem there is that Settings has to be platform agnostic (since the HttpContext primitives and such differ). I agree that's a totally valid use case, will gather some ideas on what we can do.

@nickalbrecht
Copy link

nickalbrecht commented Sep 18, 2017

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
Copy link

nickalbrecht commented Sep 18, 2017

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
Copy link
Owner Author

@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
Copy link

nickalbrecht commented Sep 19, 2017

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
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

@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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

nickalbrecht commented Oct 10, 2017

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
Copy link
Owner Author

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

@nickalbrecht
Copy link

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
Copy link
Owner Author

@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
Copy link

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

@nickalbrecht
Copy link

nickalbrecht commented Oct 31, 2017

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
Copy link
Owner Author

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
Copy link

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

x.DataIncludeRegex = new RegEx(".*", RegexOptions.Compiled);

@NickCraver
Copy link
Owner Author

@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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

stijnherreman commented Dec 19, 2017

@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
Copy link

KiarashS commented Feb 21, 2018

@NickCraver,
These features can be very useful:

  • Add a custom link button with customisable link and text in top of Error listing and Error Detail pages for back to a special address. (with default link of root address and default text to "Back")

  • Also, add a custom link button with customisable text in top of Error Detail page for back to a Error listing page. (with default text to "Back to Error listing")

@nickalbrecht
Copy link

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
Copy link

DaniilSokolyuk commented Mar 5, 2018

@NickCraver When i configure Exceptional with

public static IServiceCollection AddExceptional(this IServiceCollection services, Action<ExceptionalSettings> configureSettings = null)

values from action is not setted in Statics.Settings and LogNoContext using InMemoryStorage :(

@KiarashS
Copy link

@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
Copy link
Owner Author

@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
Copy link

@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
Copy link
Owner Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests