-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Configure Sentry tracing middleware automatically #2602
Conversation
…UseSentryTracing() with ASP.NET Core
// UseRouting sets a property on the builder that we use to detect when UseRouting has been added and we | ||
// register SentryTracing immediately afterwards | ||
// https://github.com/dotnet/aspnetcore/blob/8eaf4b51f73ae2b0ed079e4f8253afb53e96b703/src/Http/Routing/src/Builder/EndpointRoutingApplicationBuilderExtensions.cs#L58-L62 | ||
if (Properties.ContainsKey(EndpointRouteBuilder) && this.ShouldRegisterSentryTracing()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I get that correctly: Since the UseRouting
sets a flag and we check for that flag before forwarding to the InnerBuilder, Sentry will add the TracingMiddleware
with the UseX
that follows after UseRouting
?
Which in practical terms doesn't matter much because there is always something coming after UseRouting
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is what happens in the dotnet repo:
You can see they set that property immediately before calling UseMiddleware
(which is then intercepted by SentryTracingBuilder
). As soon as we see this property on the builder then, we know a call has been made to UseRouting
and we can safely register our tracing middleware.
It shouldn't matter whether something comes after UseRouting or not... the way it sits at the moment, we would detect the call to UseRouting, the property would already have been set at that stage and we'd register both the UseRouting middleware and the UseSentryTracing middleware. If anything gets registered after UseRouting or not doesn't matter much then.
Initially I was making the call to InnerBuilder.Use(middleware)
before checking for the existence of the property... just in case Microsoft ever changed the order in which they made the call to UseMiddleware and set the build property. However that would result in an extra (potentially unecessary) operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it the Sentry.sln.DotSettings
is not needed? Do we need to add it to the .gitignore
?
This is awesome! More out-of-the-box, less documentation needed and one pitfall! With one PR! 🔥
That change was to add |
Addresses Sentry tracing middleware should be added automatically #2202.
With this PR, it's almost never necessary for SDK users to call
UseSentryTracing
anymore, which will hopefully make it way easier for SDK users to get started with Sentry Tracing.Approach
We've used a much simpler variant of Andrew Lock's solution:
SentryTracingStartupFilter
as aIStartupFilter
IApplicationBuilder
in aSentryTracingBuilder
so that we can intercept all of the calls tobuilder.Use<T>
Use<T>
our custom builder checks to see if the__EndpointRouteBuilder
property has been set on the builder... which is a marker forUseRouting
. As soon as that property is set, we knowUseRouting
has been called and we can then register our tracing middlewareSentryAspNetCoreOptions.Instrumenter == Instrumeter.Sentry
. If not, we don't register the tracing middleware... this would happen if folks were using OpenTelemetry rather than Sentry to auto-instrument their AspNET Core applications.Caveats
Sentry.Google.Cloud.Functions is one of the few places I saw where I think users would still need to make the call to UseSentryTracing. There might be some other outlier scenarios like that but for the bulk of users, using vanilla ASP.NET Core, the developer experience should be way less complicated.