feat: Add AlwaysCallTransactionNameProvider option#5159
feat: Add AlwaysCallTransactionNameProvider option#5159jamescrosswell wants to merge 1 commit intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5159 +/- ##
==========================================
+ Coverage 74.05% 74.07% +0.01%
==========================================
Files 501 501
Lines 18111 18114 +3
Branches 3521 3522 +1
==========================================
+ Hits 13413 13418 +5
+ Misses 3838 3837 -1
+ Partials 860 859 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| - If the change is not user-facing, add `#skip-changelog` to the PR description | ||
| - Otherwise generated automatically from [Commit message conventions](https://develop.sentry.dev/engineering-practices/commit-messages/) | ||
| - Otherwise generated automatically from [Commit message conventions](https://develop.sentry.dev/engineering-practices/commit-messages/). | ||
| - Do **NOT** add changelogs manually. |
There was a problem hiding this comment.
Not specifically related to this PR but I ran into it on this run. Can extract to a separate PR if we really want.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // If no Name was found for Transaction, then we don't have the route. | ||
| if (transaction.Name == string.Empty) | ||
| // Also run this block if the caller has opted into always calling the TransactionNameProvider. | ||
| if (transaction.Name == string.Empty || _options.AlwaysCallTransactionNameProvider) |
There was a problem hiding this comment.
Bug: When AlwaysCallTransactionNameProvider is true, a manually set transaction name can be overwritten with the URL path if the custom name provider returns null.
Severity: LOW
Suggested Fix
Modify the condition at line 182 to prevent overwriting an already-set transaction name. The check should only proceed if the transaction name is currently empty, even when AlwaysCallTransactionNameProvider is true. For example: if (transaction.Name == string.Empty && _options.AlwaysCallTransactionNameProvider). This would respect manually set names while still allowing the provider to function when no name has been set.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/Sentry.AspNetCore/SentryTracingMiddleware.cs#L182
Potential issue: A manually set transaction name can be overwritten under specific
conditions. This happens if `_options.AlwaysCallTransactionNameProvider` is set to
`true`, and the custom `TransactionNameProvider` returns `null` or is not present. The
logic at line 182 will then proceed to an `else` block that overwrites the
`transaction.Name` with a fallback value derived from the request method and path. This
behavior silently discards the transaction name that was manually set earlier in the
request pipeline, which is likely not the intended outcome.
Did we get this right? 👍 / 👎 to inform future reviews.
| /// template is <b>not</b> retained); callers that want to preserve the route template should | ||
| /// return it themselves from the provider. | ||
| /// </remarks> | ||
| public bool AlwaysCallTransactionNameProvider { get; set; } |
There was a problem hiding this comment.
question: temporary until next major?
I'm wondering if I'm understanding it right:
Is this a temporary option for the current major-cycle to not introduce behavior-breaking changes, and we will wither then
- flip with the next major (enabled by default but possible opt-out) ... and then remove in the major after that
- or remove in the next major directly
This thought is now "escalating" in a general question:
How to deal with temporary opt-ins that we make the default in the next major, and potentially even remove then in the next major?
I think what .NET does is not doing this via .NET APIs (since the approach of the .NET Framework is that "APIs are forever" ... and instead introduce MSBuild properties (like Runtime Async's <Features>runtime-async=on</Features>),
or Runtime Feature Switches,
or Environment Variables.
On the other hand, unlike the .NET Framework, we do have a single entry point via Init(Options)/AddSentry(Options)/UseSentry(Options), which is the canonical way of configuring our SDK ... but then I'm thinking if we should "mark" such "temporary opt-ins that might become the default in an upcoming major" in a particular way ... like putting them in an Experimental namespace/type ... but that also doesn't quite fit as it's not Experimental, but Temporary ... for the current major-cycle ... hmmm ...
I see that I am a bit uncertain here.
Where are your thoughts?
There was a problem hiding this comment.
A temporary opt in, moving to a temporary opt out is a fairly straight forward way to let SDK customers easily do what they want... unless there are some compelling/damning arguments against it. What is the concern? Like what would be the worst case scenario?
About the most serious consequence that I can imagine is that some SDK customers might have to set a new option (either in csharp or in an appsettings.json file) when they bump to v7...
2 participants
Resolves #5123