newrelic / newrelic-dotnet-agent

The New Relic .NET language agent.
Apache License 2.0
97 stars 62 forks source link

Include IIS virtual directory path in request.uri attribute of transactions in ASP.NET Core #1624

Open tYoshiyuki opened 1 year ago

tYoshiyuki commented 1 year ago

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

Include IIS virtual directory path in request.uri attribute of transactions in ASP.NET Core

Feature Description

thank you always.

When using .NET Agent, I felt that there was a difference in the transaction request.uri attribute between ASP.NET Core and ASP.NET MVC (ASP.NET Web API2).

In ASP.NET MVC, the IIS virtual directory path is included in the request.uri attribute. In contrast, ASP.NET Core doesn't seem to include the IIS virtual directory path in the request.uri attribute.

I'm using both his ASP.NET MVC and ASP.NET Core IIS applications with .NET Agent, but I think it would be more natural for users to have the same request.uri attribute settings.

Evidence is provided below.

I have created two sample applications. image

aspnet-sample : ASP.NET MVC application Access with a URL like [http://localhost/aspnet-sample/api/values/].

aspnet-core-sample : ASP.NET Core application Access with a URL like [http://localhost/aspnet-core-sample/weatherforecast].

NRQL execution result. image

For ASP.NET MVC applications, the IIS virtual directory path [aspnet-sample] is included in the request.uri.

On the other hand, for ASP.NET Core applications, the client path [aspnet-core-sample] is not included in the request.uri.

Describe Alternatives

You can change the default behavior by using SetTransactionUri (.NET agent API). https://docs.newrelic.com/jp/docs/apm/agents/net-agent/net-agent-api/set-transaction-uri/

Here's an example of setting it in ASP.NET Core middleware. GetDisplayUrl (Microsoft.AspNetCore.Http.Extensions.UriHelper) is a method that combines PathBase and Path to create an entire URL.

// Add configure the HTTP request pipeline.
app.Use(async (context, next) =>
{
    NewRelic.Api.Agent.NewRelic.SetTransactionUri(new Uri(context.Request.GetDisplayUrl()));
    await next.Invoke();
});

I can change the safe behavior with the above additional code, but I would be very happy if it was realized as the default behavior of .NET Agent.

Additional context

It's just a guess, but I think it's because the URL path information from ASP.NET Core is stored separately in HttpRequest.PathBase and HttpRequest.Path. https://stackoverflow.com/questions/58614864/whats-the-difference-between-httprequest-path-and-httprequest-pathbase-in-asp-n

Therefore, I think that it will be improved if you change the part that sets request.uri of transaction in .NET Agent of ASP.NET Core to HttpRequest.PathBase and HttpRequest.Path.

// WrapPipelineMiddleware.cs
private ITransaction SetupTransaction(HttpRequest request)
{
    var path = request.Path.Value; // <- This code
    path = "/".Equals(path) ? "ROOT" : path.Substring(1);

    var transaction = _agent.CreateTransaction(
        isWeb: true,
        category: EnumNameCache<WebTransactionType>.GetName(WebTransactionType.ASP),
        transactionDisplayName: path,
        doNotTrackAsUnitOfWork: true);

    transaction.SetRequestMethod(request.Method);
    transaction.SetUri(request.Path); // <- This code

    if (request.QueryString.HasValue)
    {
        var parameters = new Dictionary<string, string>();
        foreach (var keyValuePair in request.Query)
        {
            parameters.Add(keyValuePair.Key, keyValuePair.Value);
        }

        transaction.SetRequestParameters(parameters);
    }

    return transaction;
}

Priority

Really Want

workato-integration[bot] commented 1 year ago

https://issues.newrelic.com/browse/NEWRELIC-8617

nr-ahemsath commented 1 year ago

@tYoshiyuki: Thank you for taking the time to write up a very well documented and described feature request!

I agree in general that behavioral consistency between our ASP.NET and ASP.NET Core instrumentation would make sense. However, for what it's worth, ASP.NET Core applications aren't always IIS-hosted, and so by making this change we could end up creating behavioral inconsistency for ASP.NET Core instrumentation depending on hosting model.

At any rate, I think that making this change in request.uri attribute settings for ASP.NET Core instrumentation might be considered a breaking change by existing customers, so this would have to wait for the next major release version (11). Our product manager would make the determination of whether or not to include this work in that release.