-
Notifications
You must be signed in to change notification settings - Fork 169
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
[WIP] ILogger integration #135
base: main
Are you sure you want to change the base?
Conversation
…te entries now (host as well as middleware logging the exception
fixing double logs, where middleware was writing exceptions to an ILogger, fixes unit tests
src/StackExchange.Exceptional.AspNetCore/StackExchange.Exceptional.AspNetCore.csproj
Outdated
Show resolved
Hide resolved
_category = category; | ||
_settings = settings; | ||
_httpContextAccessor = httpContextAccessor; | ||
_ignored = _ignoredCategories.Contains(category); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could attach something to Exception.Data, to prevent the same exception from being logged twice.
A reasonable case of this happening would be if an exception gets logged to an ILogger, is then rethrown, is ultimately captured by the middleware, and is logged again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into exactly that while doing my own kinda version of this and doing that worked great:
/// <summary>
/// This is used internally to stop an exception from being logged twice
/// <para />
/// This is usually needed when code closer to the source logs extra information e.g. ids
/// and then rethrows to trigger the error middleware, which ends up logging it again
/// <para />
/// Returns true if the exception has already been logged, or false otherwise
/// </summary>
/// <param name="exception"></param>
public static bool ExceptionLogged(Exception exception)
{
var key = $"Exceptional.Logged";
var logged = exception.Data.Contains(key);
if (!logged)
{
exception.Data[key] = true;
}
return logged;
}
Leaving it up to the user, if they want to.
I got this working, tests in, etc. But there's a problem. We've started out with bad assumptions of basic flows here, and that's my fault. This needs a poke at possibly refactoring the queue behavior in an ErrorStore (maybe a timer instead of a thread...but that's not without downsides) and the Going to poke this some more tonight and hopefully have a 2.2 RTM release this week. Additional thought: this should be optional - we need to make it configurable. |
@NickCraver Any ideas on next steps here? I'd love to help out if you have any other ideas for your last comment. |
This has been open for a long while. Any plans on ever continuing this? This sounds like a really great thing to have and would allow Exceptional to be the central place for viewing error-ish messages. |
initial stab at ILogger integration, seems like we're getting duplicate entries now (host as well as middleware logging the same exception)