Make better names for Web API attributes based routing

[SergeyKanzhelev]

When attribute-based routing is used by Web API application - Web SDK collects request names as Path. It doesn't recognize the templates.

@gardnerjr suggested the following code to fix it:

if (routeValues != null && routeValues.Count > 0)
{
    object controller;                    
    routeValues.TryGetValue("controller", out controller);
    string controllerString = (controller == null) ? string.Empty : controller.ToString();

    if (!string.IsNullOrEmpty(controllerString))
    {
        object action;
        routeValues.TryGetValue("action", out action);
        string actionString = (action == null) ? string.Empty : action.ToString();

       name = controllerString;
        if (!string.IsNullOrEmpty(actionString))
        {
            name += "/" + actionString;
        }
        else
        {
            if (routeValues.Keys.Count > 0)
            {
                // We want to include arguments because in WebApi action is usually null 
                // and action is resolved by controller, http method and number of arguments
                var sortedKeys = routeValues.Keys
                    .Where(key => !string.Equals(key, "controller", StringComparison.OrdinalIgnoreCase))
                    .OrderBy(key => key, StringComparer.OrdinalIgnoreCase)
                    .ToArray();

                string arguments = string.Join(@"/", sortedKeys);
                name += " [" + arguments + "]";
            }
        }
    }
    else // try to find the applicable asp.net webapi route if possible
    {
        object val;
        object[] routes = null;

        // at runtime this can't cast directly to IHttpRouteData because there are multiple versions of webapi
        if (routeValues.TryGetValue("MS_SubRoutes", out val) && (routes = val as object[]) != null && routes.Length > 0)
        {
            object first = routes.OrderBy(GetPrecedenceForRoute).FirstOrDefault();

            // take the first route's template. if we could link with system.web.http, it would be this:
            /*
            if (!string.IsNullOrEmpty(first.Route.RouteTemplate))
            {
                string routeTemplate= first.Route.RouteTemplate;
                if (!string.IsNullOrEmpty(routeTemplate))
                {
                    name = routeTemplate;
                }
            }
            */

            // through reflection:
            var getter = first?.GetType().GetProperty("Route")?.GetGetMethod();
            if (getter != null)
            {
                var route = getter.Invoke(first, null);
                var templateGetter = route?.GetType().GetProperty("RouteTemplate")?.GetGetMethod();
                if (templateGetter != null)
                {
                    var routeTemplate = templateGetter.Invoke(route, null) as string;
                    if (!string.IsNullOrEmpty(routeTemplate))
                    {
                        name = routeTemplate;
                    }
                }
            }
        }
    }
}

This code can be improved by:

1. Caching methods you took via reflection as delegates.
2. Do not try to get ms_route if delegates are undefined
3. For consistency the logic for older apps can/should be changed to use var url = ((System.Web.Routing.Route)ctx.RouteData.Route).Url;

Need to clean up code and implement functional tests.

[gardnerjr]

Yes, and the snag i ran into (hence all the reflection above) is that there are multiple distinct major versions of WebAPI, so if i used any of the actual interfaces, the code here would only work for one or another version of WebAPI.

Something similar probably also needs to be done in the .net Core version of ASP.Net WebAPI as well?

[gardnerjr]

there's also a GetPrecedenceForRoute method missing above (as any request could possibly match to multiple routes, so this method tries to find the highest weighted match). warning: more messy reflection, and usage of decimal which i personally had never typed before in c#...

private static decimal GetPrecedenceForRoute(object o)
{
    decimal precedence = 1;

    var getter = o?.GetType().GetProperty("Route")?.GetGetMethod();
    if (getter != null)
    {
        var route = getter.Invoke(o, null);

        var dataTokensGetter = route?.GetType().GetProperty("DataTokens")?.GetGetMethod();

        if (dataTokensGetter != null)
        {
            var dataTokens = dataTokensGetter.Invoke(route, null) as IDictionary<string, object>;
            object p = null;
            if ((dataTokens?.TryGetValue("precedence", out p) ?? false) && p is decimal)
            {
                precedence = (decimal)p;
            }
        }
    }

    return precedence;
}

[northtyphoon]

Any update on the fix?

[ishepherd]

Any update?

[TimothyMothra]

@zakimaksyutov what is the priority (p1, p2, p3) and milestone for this (2.7, 2.8, later)

[jadesai]

This seems to have worked for us:

    private static readonly HashSet<string> PathParamtersToLog = new HashSet<string>(StringComparer.InvariantCultureIgnoreCase)
    {
        "controller",
        "action"
    };

        string guidPattern = @"([a-z0-9]{8}[-][a-z0-9]{4}[-][a-z0-9]{4}[-][a-z0-9]{4}[-][a-z0-9]{12})";

        try
        {
            telemetry.Context.Operation.Name = Regex.Replace(telemetry.Context.Operation.Name, guidPattern, "[id]", RegexOptions.IgnoreCase);

            var req = telemetry as RequestTelemetry;
            if (req?.Url != null)
            {
                var ops = telemetry.Context.Operation.Name.Split(' ');
                var method = new HttpMethod(ops[0]);
                var route = GlobalConfiguration.Configuration.Routes.GetRouteData(new HttpRequestMessage(method, req.Url));

                if (route != null)
                {
                    if (route.Values != null && route.Values.ContainsKey("MS_SubRoutes"))
                    {
                        route = (route.Values["MS_SubRoutes"] as IHttpRouteData[])?.FirstOrDefault() ?? route;
                    }

                    var template = route.Route?.RouteTemplate ?? (string.Join("/", route.Values.Keys.Select(k => PathParamtersToLog.Contains(k) ? route.Values[k] as string : k)));
                    PathParamtersToLog.ToList().ForEach(param => template = template.Replace($"{{{param}}}", route.Values.ContainsKey(param) ? route.Values[param] as string : $"{{{param}}}"));
                    telemetry.Context.Operation.Name = method.ToString() + " /" + template;
                }
                else
                {
                    telemetry.Context.Operation.Name = Regex.Replace(telemetry.Context.Operation.Name, guidPattern, "[id]", RegexOptions.IgnoreCase);
                }
            }
        }
        catch (Exception ex)
        {
            Log.Info($"Cannot update operation name {telemetry.Context.Operation.Name} due to {ex.Message}");
        }
    }

[sadjadbp]

+1 for this feature

[Vossekop]

I'm also very keen to see this improved.

[generatives]

+1 for something like this

[ASChatmon]

+1 for this.

[LouisGordon]

+1 for this.

[davekats]

+1 on this

[cijothomas]

Though a lot of users are asking for this feature, this is not prioritized for this year. Will be addressing this through OpenTelemetry route in the future, and not likely in this repo.

[rhythmnewt]

+1 for this

[deyanp]

+1

but please consider also the case when instead of Controllers with Route attributes direct mapping is used (IEndpointRouteBuilder.MapPut, MapGet, MapMethods)

[tbasallo]

Has this been addressed in one of the newer SDKs or is it planned? Having hundreds of entries because the ID is part of the URL versus showing /path/[id] is not helping identify real issues or aggregate correctly.

Can we get a status update on this or another issue where this is being tracked (if it exists)?

[angularsen]

Posted a workaround here: https://stackoverflow.com/a/70049170/134761 Uses the route name, if specified, otherwise the route template + API version to compute a request telemetry name.

Example name: GET /config v2.0

[github-actions[bot]]

This issue is stale because it has been open 300 days with no activity. Remove stale label or this will be closed in 7 days. Commenting will instruct the bot to automatically remove the label.