feat!: don't trace 301, 305-399 or 401-404 responses by default - #5632
Conversation
TraceIgnoreStatusCodes now defaults to [(301, 303), (305, 399), (401, 404)], the value the SDK spec recommends for the first major release after the option was introduced. Clear the list to trace every request again. Closes #4740 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## version7 #5632 +/- ##
===========================================
Coverage ? 74.76%
===========================================
Files ? 515
Lines ? 18910
Branches ? 3689
===========================================
Hits ? 14139
Misses ? 3894
Partials ? 877 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ASP.NET MVC, Razor Pages and minimal APIs redirect with a 302 after a successful form POST, so ignoring 302/303 would drop the transactions that do the real work. Match sentry-ruby's default instead: [301, (305, 399), (401, 404)]. See getsentry/sentry-docs#19658 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| /// </summary> | ||
| public IList<HttpStatusCodeRange> TraceIgnoreStatusCodes { get; set; } = []; | ||
| public IList<HttpStatusCodeRange> TraceIgnoreStatusCodes { get; set; } = new List<HttpStatusCodeRange> | ||
| { | ||
| 301, | ||
| (305, 399), | ||
| (401, 404) | ||
| }; | ||
|
|
There was a problem hiding this comment.
Bug: Configuring TraceIgnoreStatusCodes via a configuration file incorrectly replaces the new default values instead of appending to them, causing more transactions to be traced.
Severity: MEDIUM
Suggested Fix
Update the configuration binding logic to merge user-provided values from the configuration file with the default values, rather than replacing them. The logic should append values from BindableSentryOptions.TraceIgnoreStatusCodes to the existing options.TraceIgnoreStatusCodes list.
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/SentryOptions.cs#L954-L961
Potential issue: The configuration binding logic for `TraceIgnoreStatusCodes` replaces
the entire default list if any values are provided in a configuration file, such as
`appsettings.json`. This is due to a null-coalescing assignment (`??`) that was not
updated to handle the new, non-empty default list. As a result, users who customize this
setting to add their own status codes will unintentionally lose the new default ignored
codes (e.g., 401-404). This can lead to an unexpected increase in traced transactions
and higher span quota consumption, as status codes that were intended to be ignored by
default will now be traced.
Also affects:
test/Sentry.Tests/BindableSentryOptionsTests.cs:26~67
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
That's not a bug - it's by design. We provide some defaults but people can override these via configuration. Otherwise there would be no way to remove any of our defaults via configuration (it would have to be configured in code).
|
Nice! Looks good to me, the Agent found a couple of things regarding outgoing HTTPCalls: The new default also drops outgoing calls under OpenTelemetryThe description says outgoing HTTP spans are unaffected. That holds for child spans. With OpenTelemetry, though, an I reproduced this with a real
The spec says the option "applies exclusively to incoming requests". The processor has behaved this way since #5034, but until now it only affected people who set the list themselves. This PR turns it on for every ASP.NET Core app that uses OpenTelemetry. A 401 or 403 from a downstream API usually means expired credentials, which people want to see. Skipping if (_options.TraceIgnoreStatusCodes.Count == 0 || transaction.Operation == "http.client")With that change, the SDK sends all three cases again, and Configuration
Smaller things
|
|
Thanks @ric-oliv, great catch on the OpenTelemetry one.
|
Closes #4740
Summary
SentryOptions.TraceIgnoreStatusCodesnow defaults to[301, (305, 399), (401, 404)]instead of an empty list. The option was added in #5034.ASP.NET Core and ASP.NET apps, including ASP.NET Core apps instrumented with OpenTelemetry, no longer send a transaction for an incoming request that ends in one of those status codes: mostly infrastructure redirects (e.g. 307 from
UseHttpsRedirection, 301 from rewrite rules) and 401–404 responses. Those requests rarely help with debugging, are often triggered by scanning bots, and use up span quota.The issue originally asked for 404 only. We widened it to cover the other low-value codes so that this default only has to break once.
Why not the spec's default?
The SDK spec recommends
[[301, 303], [305, 399], [401, 404]], but getsentry/sentry-docs#19658 adds a disclaimer that this default doesn't suit every framework, because some use 302/303 for successful redirects..NET is one of them. MVC's scaffolded Create/Edit/Delete actions end with
RedirectToAction(...), and Razor PagesRedirectToPage(),LocalRedirect()after an Identity login, minimal APIs'Results.Redirectand Blazor static SSR'sNavigateToafter a form post all send a 302. Those POSTs are where the database writes happen, so ignoring 302 would drop the most useful transactions in a typical app.We therefore match sentry-ruby's default, which is the spec value minus 302/303. The cost is that cookie-auth challenges (an anonymous request to an
[Authorize]page, redirected to the login page with a 302) are still traced.Migration
To trace every request again, clear the list:
To ignore only some of these codes, replace the list, e.g.
options.TraceIgnoreStatusCodes = [404];, or inappsettings.json:Setting
"TraceIgnoreStatusCodes": []inappsettings.jsondoes not clear the default.Microsoft.Extensions.Configurationbinds an empty JSON array asnull(checked with the reflection and source-generated binders, 8.0 and 10.0), so the SDK can't tell "empty" from "not set". To opt out from configuration alone, set a status code that no response uses:Configuration can't express ranges yet, so keeping the default and adding a code from
appsettings.jsonmeans listing every code. See #5644.Notes for review
event_processorclient-report discards.SentryClient.CaptureTransactionalready does both for any transaction processor that returnsnull.version7first. With OpenTelemetry, an outgoingHttpClientcall with no local parent becomes anhttp.clienttransaction of its own, and the processor onmaindrops it when the downstream status matches the list. This PR's default would turn that on for every ASP.NET Core app using OpenTelemetry. fix: TraceIgnoreStatusCodes no longer drops outgoing HTTP requests traced with OpenTelemetry #5643 limits the processor tohttp.servertransactions, as the spec requires. Outgoing calls that are child spans of a request were never affected.🤖 Generated with Claude Code