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

[WIP] ILogger integration #135

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

m0sa
Copy link
Contributor

@m0sa m0sa commented Jun 21, 2018

initial stab at ILogger integration, seems like we're getting duplicate entries now (host as well as middleware logging the same exception)

m0sa added 2 commits June 22, 2018 00:45
…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
_category = category;
_settings = settings;
_httpContextAccessor = httpContextAccessor;
_ignored = _ignoredCategories.Contains(category);
Copy link
Contributor Author

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...

Copy link
Contributor

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;
}

@NickCraver
Copy link
Owner

NickCraver commented Dec 16, 2018

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 ILogger only creates the Error and queues, but does not log to the store. Retry background behavior (which already exists, just not 1:1 with what we need here) would be to actually log the errors to the store. This needs to handle fetching behavior as well, as is currently the case only when in retry today but we can simulate that with a length check on the queue.

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.

@mmillican
Copy link
Contributor

@NickCraver Any ideas on next steps here? I'd love to help out if you have any other ideas for your last comment.

@mrahhal
Copy link

mrahhal commented May 5, 2020

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.

@NickCraver NickCraver changed the base branch from master to main June 20, 2020 13:51
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

Successfully merging this pull request may close these issues.

5 participants