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

Fix: Use fallback if RoutePattern is MVC. #1188

Merged
merged 14 commits into from
Sep 10, 2021
Merged

Fix: Use fallback if RoutePattern is MVC. #1188

merged 14 commits into from
Sep 10, 2021

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Sep 9, 2021

Despite a context feature having a RouteEndpoint, it doesn't mean we can use the RawText value of the RoutePattern since it may result in a default route for different paths.
For a simple fix, I opted to just use the fallback to get the MVC route since we didn't have any proper data of the Route Template on the given Endpoint.

Additionally, that the SDK is creating transactions for items like loading a CSS file, thus results in Unknown Routes. If desired, we could use context.Request.Path.Value as a last fallback for cases like that.

Sample:
image

Also, the issue can be easily reproduced on the sample Sentry.Samples.AspNetCore.Mvc

Close #1189.

@lucas-zimerman lucas-zimerman added Bug Something isn't working Framework: ASP.NET Core labels Sep 9, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #1188 (8142f71) into main (c307373) will decrease coverage by 0.04%.
The diff coverage is 58.33%.

❗ Current head 8142f71 differs from pull request most recent head fdc1b71. Consider uploading reports for the commit fdc1b71 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1188      +/-   ##
==========================================
- Coverage   80.55%   80.51%   -0.05%     
==========================================
  Files         212      212              
  Lines        6891     6902      +11     
  Branches     1573     1579       +6     
==========================================
+ Hits         5551     5557       +6     
  Misses        822      822              
- Partials      518      523       +5     
Impacted Files Coverage Δ
...try.AspNetCore/Extensions/HttpContextExtensions.cs 82.75% <58.33%> (-17.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c307373...fdc1b71. Read the comment docs.

@lucas-zimerman lucas-zimerman marked this pull request as ready for review September 10, 2021 18:16
@bruno-garcia bruno-garcia enabled auto-merge (squash) September 10, 2021 22:55
@bruno-garcia bruno-garcia merged commit 934691f into main Sep 10, 2021
@bruno-garcia bruno-garcia deleted the ref/http-route branch September 10, 2021 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Framework: ASP.NET Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix performance transaction name when using MVC routes
4 participants