NickCraver / StackExchange.Exceptional

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

Exception not logged if HttpContext is not passed as parameter #163

Closed florinciubotariu closed 5 years ago

florinciubotariu commented 5 years ago

Hello. I've installed Exceptional into an ASP.NET Core 2.2 application and I have some issues in some parts of the application when I want to log exceptions.

In the documentation of the Log method, it is stated that for non web requests, we should pass null for HttpContext. Also, it is stated that it should not be "forgotten for most web application usages, so it's not an optional parameter." https://github.com/NickCraver/StackExchange.Exceptional/blob/41323b6fe2f4c6feca3545a98335d539f030c249/src/StackExchange.Exceptional.AspNetCore/AspNetCoreExtensions.cs#L33-L69

Looking at the code, I see that the HttpContext in mandatory for two operations:

  1. Get the Exceptional settings.
  2. Transfer data from HttpContext to Error.

The problem is that if you do not supply the HttpContext as parameter, a NullReferenceException is thrown because RequestServices is being accessed.

Is it possible to have the exception logged even if the HttpContext is not supplied? I'm thinking of:

  1. If you do not supply the context, then simply do not transfer data from it to Error.
  2. Get the Exceptional settings that were passed in the DI initializer (ExceptionalServiceCollection.AddExceptional)

I'm asking this because there are some special cases when you are in an ASP.NET Core application but you don't have access to the HttpContext (in my case, Custom JSON converter and a long running WebSocket connection)

MattJeanes commented 5 years ago

I personally use ex.LogNoContext() - you should be able to use that if you also see ex.Log()

An excerpt from some of my (prod) code:

if (context != null)
{
    exception.Log(context, _categoryName, customData: customData);
}
else
{
    exception.LogNoContext(_categoryName, customData: customData);
}

However, I also remember reading that documentation about passing null to Log - @NickCraver is this still correct or should people now use the LogNoContext method?

florinciubotariu commented 5 years ago

Thank you @MattJeanes . Works without problems!

I did not see the LogNoContext method in the documentation from https://nickcraver.com/StackExchange.Exceptional but it seems some useful information can be found here.

MattJeanes commented 5 years ago

Great to hear! I guess we'll wait and see what Nick has to say about how this should work

NickCraver commented 5 years ago

If you see the set properties method below, we intended to handle null context and log all the information available...that's a regression for sure. I'm fixing it up and adding tests right now.

NickCraver commented 5 years ago

Fixed in https://github.com/NickCraver/StackExchange.Exceptional/commit/c36d68a86875c1ff79516bce5877aca073566a98 - it should be up on MyGet to test shortly :) I'll update NuGet after a good Stack Overflow test run this week (updating several libraries there).

If this doesn't resolve you issue, please ping and I'll reopen!

MattJeanes commented 5 years ago

That's what I figured. So when should we use LogNoContext vs Log with a null HttpContext?

NickCraver commented 5 years ago

There are 2 main cases where you'd use LogNoContext:

  1. When you know you don't have any ever (just for efficiency).
  2. When you're not even in a web project and don't want to reference anything with HttpContext.

For example, a user can use Exceptional.Shared in a console app with no knowledge or methods for HttpContext at all. .LogNoContext exists in a lower-level shared package and is designed for users not doing web at all. But it's generally usable whenever you don't want to reference or have a HttpContext for whatever reason (e.g. background thread).

MattJeanes commented 5 years ago

Ah yes, they're in different packages. If I recall the LogNoContext was added because of console apps and to avoid the mess around the fact that HttpContext differed between AspNetCore and AspNet and depending on if you used the main StackExchange.Exceptional package or StackExchange.Exceptional.AspNetCore you'd get a Log extension method with the 2 variants of HttpContext