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

Only log application errors #50

Open
blairlearn opened this issue Oct 19, 2022 · 2 comments
Open

Only log application errors #50

blairlearn opened this issue Oct 19, 2022 · 2 comments

Comments

@blairlearn
Copy link
Collaborator

blairlearn commented Oct 19, 2022

Issue description

Both application and user errors are being indiscriminately written to the Windows Application log.

This is because our controllers throw APIErrorException for errors and use a global exception handler to return a suitable HTTP status code. Despite the exception handler in the startup, these exceptions are considered unhandled exceptions and logged in the Windows event log, regardless of whether the user was due to application logic or something the user did.

ESTIMATE TBD

What's the expected change?

  • Errors resulting from user inputs are not logged.
  • When running on Windows, errors resulting from application errors are written to the Windows Application log.

What's the current functionality?

  • All errors are written to the Windows Application log.

What's the updated acceptance criteria?

Additional details / screenshot

image

Related Tickets

@blairlearn blairlearn transferred this issue from NCIOCPL/sitewide-search-api May 11, 2023
@blairlearn blairlearn changed the title Log user errors as Debug level Fix Error Handling May 11, 2023
@blairlearn
Copy link
Collaborator Author

Part of the solution is to create an Exception Filter. The filter should behave as:

  • For APIErrorException, the HTTP status is set according to the value set in the exception, and the exception is marked as handled.
  • Known application failure points continue to log their own errors and throw APIInternalException. The HTTP status is set to 500, and the exception is marked as handled.
  • All other errors (i.e. "The ones we don't know about") are logged, the HTTP status is set to 500, and the exception is marked as handled.
// Partial implemenation of the exception filter

using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Filters;

using NCI.OCPL.Api.Common;

public class NCIResponseExceptionFilter : IActionFilter, IOrderedFilter
{
    public int Order => int.MaxValue - 10;

    public void OnActionExecuting(ActionExecutingContext context) { }

    public void OnActionExecuted(ActionExecutedContext context)
    {
        if (context.Exception is APIErrorException httpResponseException)
        {
            context.Result = new ObjectResult(new { Message = httpResponseException.Message })
            {
                StatusCode = httpResponseException.HttpStatusCode
            };

            context.ExceptionHandled = true;
        }
    }
}

// Register the filter in NciStartupBase
services.AddControllers(options => {
    options.Filters.Add<NCIResponseExceptionFilter>();
});

@blairlearn
Copy link
Collaborator Author

blairlearn commented May 11, 2023

Set up logging to the Windows event viewer with code along the lines of this in NciProgramBase.cs, but use the appropriate log type for the OS (e.g. RuntimeInformation.IsOSPlatform(OSPlatform.Windows))

public static IHostBuilder CreateHostBuilder (string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureLogging (logging =>
        {
            logging.AddEventLog (new EventLogSettings
            {
                SourceName = "YourSourceName",
                LogName = "Application"
            }):
        })
        .ConfigureWebHostDefaults (webBuilder =>
        {
            webBuilder. UseStartup<Startup> ( ) :
        }):

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

1 participant