dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.4k stars 10k forks source link

Log endpoint details at application startup to aid with issue diagnosis #34604

Open DamianEdwards opened 3 years ago

DamianEdwards commented 3 years ago

To aid in situations where one is trying to diagnose issues related to routing, minimal API endpoints, parameter binding sources, etc. it would be beneficial for a log to be emitted containing the endpoint details as they're registered, i.e. one log per endpoint. When the endpoint is a minimal API, it should include details of how the parameters will be bound, and return type details, along with any endpoint metadata associated with the endpoint. It might be interesting to think about how other endpoint-providing sub-systems, e.g. MVC, could augment this log with relevant information too.

The log would target the DEBUG log level and include a message and a single value containing a JSON payload of the details, e.g.:

Message: Endpoint added at route "/say/{greeting}/{name}/{age?:int}

{
    "route": {
        "pattern": "/say/{greeting}/{name}/{age?:int}",
        "verbs": "GET",
        "name": null,
        "parameters": [
            {
                "name": "greeting",
                "required": true
            },
            {
                "name": "name",
                "required": true
            },
            {
                "name": "age",
                "required": false
            }
        ]
    },
    "description": {
        "name": "GetGreeting", // Would be null if we determine it was an anonymous delegate (i.e. compiler generated name)
        "delegateReturnType": "Task<string>",
        "responseProcessing": "string", // Possible values: string, JSON, IResult
        "parameters": [
            {
                "name": "greeting",
                "required": true,
                "type": "string",
                "bindingSource": "RouteData" // Possible values: RouteData, QueryString, ServiceProvider, RequestBody
            },
            {
                "name": "name",
                "required": true,
                "type": "string",
                "bindingSource": "RouteData"
            },
            {
                "name": "age",
                "required": false,
                "type": "int",
                "bindingSource": "RouteData"
            },
            {
                "name": "filter",
                "required": false,
                "type": "string",
                "bindingSource": "QueryString"
            },
            {
                "name": "db",
                "required": true,
                "type": "MyApplication.TodoDb",
                "bindingSource": "ServiceProvider"
            }
        ]
    },
    "metadata": {
        // Keys are type name, value are result of ToString()
        "EndpointNameAttribute": "GetGreeting",
        "HttpMethodMetadata": "GET",
        "MethodInfo": ""
    }
}

@davidfowl

davidfowl commented 3 years ago

Couple of notes:

DamianEdwards commented 3 years ago
davidfowl commented 3 years ago

The signature is just friendly I guess and makes it easy to quickly match the log to the API delegate

I would get rid of this.

IIRC route constraints affect matching which is usually one of the hardest parts of diagnosing route issues, so I figured including the details explicitly would be useful here. Also if a custom route constraint is used this would be the place that confirms the short identifier is correctly mapped to the implementing type.

I would get rid of this as well, we'd need to flow this information and we don't have it all in the same place right now.

DamianEdwards commented 3 years ago

Will update the example based on the feedback

Kahbazi commented 3 years ago
"metadata": {
    // Keys are type name, value are result of ToString()
    "EndpointNameAttribute": "GetGreeting",

Does this mean all current implementations of metadata should override ToString() to return something meaningful?

DamianEdwards commented 3 years ago

@Kahbazi that would be ideal.

Kahbazi commented 3 years ago

@Kahbazi that would be ideal.

I'll send a PR overriding ToString() method for all metadata.

davidfowl commented 3 years ago

Given that we're making improvements here with throwing errors for the most common cases at startup, I think this can be punted to .NET 7.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.