Skip to content

Custom logging for Unsupported Version response #963

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

Closed
1 task done
Sirozha1337 opened this issue Feb 20, 2023 · 5 comments
Closed
1 task done

Custom logging for Unsupported Version response #963

Sirozha1337 opened this issue Feb 20, 2023 · 5 comments

Comments

@Sirozha1337
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

I am trying to log messages when requests are passing an unsupported version in parameters.

I have found a comment related to it:
#886 (comment)

But I can't understand how to implement this solution.

Example code:

Program.cs

using Asp.Versioning;
using Asp.Versioning.Routing;

var builder = WebApplication.CreateBuilder( args );


builder.Services.AddControllers();
builder.Services.AddTransient<ILogger<ApiVersionMatcherPolicy>>(
     sp => sp.GetRequiredService<ILoggerFactory>().CreateLogger<ApiVersionMatcherPolicy>());
builder.Services.AddApiVersioning(
                    options =>
                    {
                        options.ApiVersionReader = new HeaderApiVersionReader("x-api-version");
                    })
                .AddMvc();
var app = builder.Build();

// Configure the HTTP request pipeline.

app.UseHttpsRedirection();
app.UseAuthorization();
app.MapControllers();
app.Run();

Controllers/MyTestController.cs

using Asp.Versioning.Routing;

namespace ApiVersioning.Examples.Controllers;

using Asp.Versioning;
using Microsoft.AspNetCore.Mvc;

[Route( "api/test" )]
[ApiVersion( 1.0 )]
[ApiController]
public class MyTestController : ControllerBase
{
    [HttpGet("foo")]
    public string Foo(ApiVersion version) => "Version " + version;
}

I call the API with "x-api-version" set to "2", naturally I receive 400 error,

Putting the breakpoint on "sp.GetRequiredService().CreateLogger()" I see that it's called the first time I receive an error. So, the logger is created, but it's not called after that. It seems like I have to tick some other option in order for logger to be requested.

Describe the solution you'd like

I'd like for those kind of errors to be always logged with "Warn" log level, so that I could control them in my settings.json like this:

"Logging": {
    "LogLevel": {
      "Default": "Information",
      "Asp.Versioning.ApiVersionMatcherPolicy": "Trace"
    }
}

If there's some other way it is done in this package, I'd like to know how to use it. Because currently I have found nothing about this in the project's Wiki.

Additional context

.NET 6 with Asp.Versioning.Mvc/6.4.0

@Sirozha1337
Copy link
Author

Ok, I checked out the source and traced the logger object. Looks like this line is the culprit:


The logger doesn't get passed to UnsupportedApiVersionEndpoint, so no logging available for that kind of error.
Looks like a bug to me.

@commonsensesoftware
Copy link
Collaborator

This would certainly be an enhancement as opposed to a bug simply because things work exactly the same way they have since day 1. There is no documentation on logging because there's never been clear documentation or direction for it as a feature. In 7 years, no one has ever asked for additional details or configuration. Gold ⭐ my friend!

If I recall correctly, I think I even considered eliminating logging altogether in the 6.0 timeframe. What exists/remains simply has parity with the <= 5.x versions. The scenario you've asked for doesn't exist back in 5.1 either:

Client errors are not errors to the server. The most interesting thing on the server is information that cannot be known to the client, which is very little; for example, candidate action names. All client errors are returned as ProblemDetails. In 6.4, you can add the logging you want by replacing the IProblemDetailsFactory (API Versioning) or ProblemDetailsFactory (MVC Core) service. It doesn't matter much which one you choose, but replacing the IProblemDetailsFactory is probably simpler. It would be nearly identical to the internal MvcProblemDetailsFactory with the addition of logging:

https://github.com/dotnet/aspnet-api-versioning/blob/e9242d65295f2e9d18ab324d0776c85e75321ab6/src/AspNetCore/WebApi/src/Asp.Versioning.Mvc/MvcProblemDetailsFactory.cs

The problem type will indicate if it's due to an unsupported API version.

I'm not strongly opposed to baking in some additional logging, but it needs to be laser-focused on specific scenarios with valuable feedback for developers. A client error is not a warning to a server, so I won't want that to be the default. A client making a request with an unsupported API version is no different that calling an API with invalid data; effectively, the client doesn't know how to call the server. If there's a way to change that in a developer app, I'm fine with that. The thing that should be a warning to a server is a high number of client errors; especially for an unsupported version. Unfortunately, logging can never solve that alone. Some higher level monitoring and alerting needs to look at the call rate, potentially by client, and then decide if a notification is warranted when too many unsupported API version requests occur. Making sure this type of instrumentation exists so that a developer have that type of monitoring and alerting is interesting.

@Sirozha1337
Copy link
Author

@commonsensesoftware well, my idea was that there might be some problem with logging on the client, so it would just stop working and I wouldn't know the reason, because there are no logs on the server-side. Yes, it might seem stupid, logging same stuff from two places because of some unlikely scenario. But stranger things have happened in production, so it's better to be safe than sorry.

Thanks for the workaround. It works like a charm, thank you!

If someone has a similar problem, the workaround looks somewhat like this:

internal sealed class MyProblemDetailsFactory : IProblemDetailsFactory
{
    private readonly ProblemDetailsFactory _factory;
    private readonly ILogger<MyProblemDetailsFactory> _logger;

    public MyProblemDetailsFactory( ProblemDetailsFactory factory, ILogger<MyProblemDetailsFactory> logger )
    {
        _factory = factory;
        _logger = logger;
    }

    public ProblemDetails CreateProblemDetails(
        HttpRequest request,
        int? statusCode = null,
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null )
    {
        var httpContext = request.HttpContext;
        var problemDetails = _factory.CreateProblemDetails( httpContext, statusCode, title, type, detail, instance );
        DefaultProblemDetailsFactory.ApplyExtensions(problemDetails );
        
        if (problemDetails.Type == "https://docs.api-versioning.org/problems#unsupported")
        {
            _logger.LogWarning("Request with unsupported API Version: {Detail}", problemDetails.Detail);
        }
        
        return problemDetails;
    }
}

And then replace the original IProblemDetailsFactory in the DI:

builder.Services.Replace(new ServiceDescriptor(
     typeof(IProblemDetailsFactory),
     typeof(MyProblemDetailsFactory),
     ServiceLifetime.Transient));

@commonsensesoftware
Copy link
Collaborator

Glad this works for you. Again, I'm not completely against improving the logging, but it has to be quantified as to what makes sense for the 99%. Once it's added and officially supported, it's hard to ever remove.

It's worth noting that you can use ProblemDetailsDefaults.Unsupported.Type instead of the magic string if you want to. 😄

Finally, in 7.0 things change. They change because ASP.NET Core changed. ASP.NET Core 7.0 introduces a new IProblemDetailsService. This effectively does the same thing as IProblemDetailsFactory. API Versioning needed ProblemDetails for Minimal APIs, but there was no available service to do so in ASP.NET 6.0, when they were introduced. The existing ProblemDetailsFactory is part of MVC Core - arguably a design mistake. In 7.0, IProblemDetailFactory is removed as it's unnecessary. You will replace it with an IProblemDetailsWriter, of which there can be many. Most everything else is the same. You also have to opt in by calling builder.Services.AddProblemDetails(), which believe it or not, some people do not want. For more details, see the Error Handling: Problem Details topic on Microsoft Learn.

@commonsensesoftware
Copy link
Collaborator

I believe this issue has reached its logical conclusion. You've found a workable solution to the original issue. Adding or changing IProblemDetailsFactory or IProblemDetailsWriter is the approach to use for customizing this behavior in .NET 7.0+. Don't forget to opt into problem details with services.AddProblemDetails().

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

No branches or pull requests

2 participants