feat: Add AlwaysCallTransactionNameProvider option#5159
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5159 +/- ##
==========================================
- Coverage 74.20% 74.08% -0.12%
==========================================
Files 501 501
Lines 18113 18118 +5
Branches 3521 3524 +3
==========================================
- Hits 13440 13422 -18
- Misses 3811 3837 +26
+ Partials 862 859 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Submodule modules/sentry-cocoa was not changed in this PR, so reset to main's version. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| if (transaction.Name.Length == 0 || forceCustomName) | ||
| { | ||
| var method = context.Request.Method.ToUpperInvariant(); | ||
|
|
||
| // If we've set a TransactionNameProvider, use that here | ||
| var customTransactionName = context.TryGetCustomTransactionName(); | ||
| if (!string.IsNullOrEmpty(customTransactionName)) | ||
| if (customName.Value is { } customTransactionName) | ||
| { | ||
| transaction.Name = $"{method} {customTransactionName}"; | ||
| tracer.NameSource = TransactionNameSource.Custom; |
There was a problem hiding this comment.
Bug: When AlwaysCallTransactionNameProvider is true, a manually set transaction name is incorrectly overwritten by the name from the provider.
Severity: MEDIUM
Suggested Fix
Modify the condition at line 183 to prevent overwriting the transaction name if it has already been manually changed. The check should incorporate a condition like transaction.Name == initialName before allowing the TransactionNameProvider to overwrite the name when AlwaysCallTransactionNameProvider is true.
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#L184-L192
Potential issue: When `_options.AlwaysCallTransactionNameProvider` is set to `true`, the
logic at line 184 will overwrite any manually set transaction name with the name
provided by the `TransactionNameProvider`. The condition `forceCustomName` at line 183
evaluates to true based only on the option being enabled and the provider returning a
value, without checking if the transaction name was already changed from its initial
value during the request. This behavior contradicts the option's documentation, which
implies it should only override the initial route-derived name, not a name that was set
manually mid-request.
There was a problem hiding this comment.
This is something that can be controlled via the implementation of the customTransactionName function that the SDK user supplies (they can check if a value exists and return null if they don't want to override this). I think it's more flexible if we leave it like this - otherwise they wouldn't be able to use the customTransactionName to override values.
@Flash0ver thoughts?
There was a problem hiding this comment.
Hmm ... how could the user control this?
With the TransactionNameProvider receiving HttpContext as in and returning string? as out, how could the user delegate perform this check without the actual ITransactionTracer?
| /// returns <c>null</c> or <see cref="string.Empty"/>, the routed name is preserved (or the URL-path | ||
| /// fallback applies when no route was resolved). | ||
| /// </remarks> | ||
| public bool AlwaysCallTransactionNameProvider { get; set; } |
There was a problem hiding this comment.
thought: rename
I'm not sure I like the name:
To me, AlwaysCall.. reads more of an implementation detail, in particular the ..Call.. part, rather than the semantic effect.
I'll try some alternatives, let me know what you think:
OverrideTransactionName/OverwriteTransactionName- doesn't imply the use of the user-provided
TransactionNameProvider
- doesn't imply the use of the user-provided
OverrideTransactionNameWithTransactionNameProvider- too long
PreferTransactionNameProvider- implies the
TransactionNameProvider, and also precedence
- implies the
ForceTransactionNameProvider- might be too strong, because when the user-delegate returns
null, it's not overwritten
- might be too strong, because when the user-delegate returns
I kinda like PreferTransactionNameProvider.
What do you think?
Or should we stick with AlwaysCallTransactionNameProvider?
| var customName = new Lazy<string?>(context.TryGetCustomTransactionName); | ||
| var forceCustomName = _options.AlwaysCallTransactionNameProvider && customName.Value is not null; | ||
| if (transaction.Name.Length == 0 || forceCustomName) |
There was a problem hiding this comment.
thought: I find the Lazy here a bit hard to read
And it's also a bit of an overkill, since there's no thread contention.
If I get it right, it's used as a cache to run the user-callback only once.
I wonder if we can rewrite it a bit without the Lazy<> 🤔.
If it would be harder to read without the Lazy<>, then let's use the hint that it is not accessed concurrently:
new Lazy<string?>(context.TryGetCustomTransactionName, LazyThreadSafetyMode.None)| var customName = new Lazy<string?>(context.TryGetCustomTransactionName); | ||
| var forceCustomName = _options.AlwaysCallTransactionNameProvider && customName.Value is not null; | ||
| if (transaction.Name.Length == 0 || forceCustomName) |
There was a problem hiding this comment.
question: string-check ... potential NRE
The "TransactionName"-check changed:
-transaction.Name == string.Empty
+transaction.Name.Length == 0From a defensive perspective:
Are we sure that transaction.Name here is never null?
| if (transaction.Name.Length == 0 || forceCustomName) | ||
| { | ||
| var method = context.Request.Method.ToUpperInvariant(); | ||
|
|
||
| // If we've set a TransactionNameProvider, use that here | ||
| var customTransactionName = context.TryGetCustomTransactionName(); | ||
| if (!string.IsNullOrEmpty(customTransactionName)) | ||
| if (customName.Value is { } customTransactionName) | ||
| { | ||
| transaction.Name = $"{method} {customTransactionName}"; | ||
| tracer.NameSource = TransactionNameSource.Custom; |
There was a problem hiding this comment.
Hmm ... how could the user control this?
With the TransactionNameProvider receiving HttpContext as in and returning string? as out, how could the user delegate perform this check without the actual ITransactionTracer?
3 participants
Resolves #5123