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

[AzureMonitorexporter] fix operation name #41623

Closed
wants to merge 7 commits into from

Conversation

TimothyMothra
Copy link
Contributor

@TimothyMothra TimothyMothra commented Jan 26, 2024

When using Asp.Net Core Routing rules, our Exporter is currently recording the full pattern as the Operation Name.
This means that requests to different Controllers or Actions will show identical Operation Names, as shown in this screenshot.

Screenshot 2024-01-25 115910

This can be reproduced in an MVC app configured like this:

app.MapControllerRoute(
    name: "default",
    pattern: "{controller=Home}/{action=Index}/{id?}");

My test app has a HomeController and CustomerController. Regardless which controller is invoked, The url.path shows the correct Controller/Action. The http.route shows the configured pattern.

  • https://localhost:44311/Home/Index

    Activity.ActivitySourceName: Microsoft.AspNetCore
    Activity.DisplayName:        GET {controller=Home}/{action=Index}/{id?}
    Activity.Kind:               Server
    Activity.Tags:
        server.address: localhost
        server.port: 44311
        http.request.method: GET
        url.scheme: https
        url.path: /
        network.protocol.version: 2
        http.route: {controller=Home}/{action=Index}/{id?}
        http.response.status_code: 200
    
  • https://localhost:44311/Customer/Index

    Activity.ActivitySourceName: Microsoft.AspNetCore
    Activity.DisplayName:        GET {controller=Home}/{action=Index}/{id?}
    Activity.Kind:               Server
    Activity.Tags:
        server.address: localhost
        server.port: 44311
        http.request.method: GET
        url.scheme: https
        url.path: /Customer/Index
        network.protocol.version: 2
        http.route: {controller=Home}/{action=Index}/{id?}
        http.response.status_code: 200
    

This is in part due to the data provided by the OpenTelemetry.Instrumentation.AspNetCore library, where the configured pattern is used as http.route. This is a known issue in that library: open-telemetry/opentelemetry-dotnet-contrib#1730

Changes

  • adjust TraceHelper to better handle cases where http.route contains a controller pattern.
  • Add more test cases

With this change:
image

@github-actions github-actions bot added the Monitor - Exporter Monitor OpenTelemetry Exporter label Jan 26, 2024
@TimothyMothra TimothyMothra marked this pull request as ready for review January 26, 2024 22:42
@vishweshbankwar
Copy link
Contributor

@TimothyMothra Could you confirm if the output with this change matches with the output when exporter without this change is used with 1.6.0-beta.3 version of the instrumentation library?

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@TimothyMothra

This comment was marked as off-topic.

@bunkrur
Copy link

bunkrur commented Jan 27, 2024

Thank you for this!

@insylogo
Copy link

insylogo commented Jan 29, 2024

Routes should not be fully populated in the names if you don't want to blow up cardinality on requests i.e. when you go to app insights, you should see something like /MyController/MyAction/{id} and not /MyController/MyAction/123, otherwise you can't get accurate statistics in most views for request timing etc.

It is of course better than /{Controller}/{Action}/{id}, and I'm not even sure how this populates all the route fields with these changes?

TimothyMothra and others added 2 commits January 29, 2024 10:01
Co-authored-by: Vishwesh Bankwar <vishweshbankwar@users.noreply.github.com>
@TimothyMothra TimothyMothra marked this pull request as draft January 29, 2024 21:06
@TimothyMothra
Copy link
Contributor Author

Closing this PR for now. Tracking the issue here: #41650
Were going to work with the OpenTelemetry community to see what fixes can be applied there.

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

Successfully merging this pull request may close these issues.

6 participants