Repository navigation
feat: Add option to exclude certain HTTP statuses from tracing - #5034
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Breaking Changes 🛠
Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5034 +/- ##
=======================================
Coverage 74.02% 74.02%
=======================================
Files 499 500 +1
Lines 18049 18063 +14
Branches 3512 3515 +3
=======================================
+ Hits 13361 13372 +11
- Misses 3831 3837 +6
+ Partials 857 854 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| && statusCodeObj is int statusCode | ||
| && _options.TraceIgnoreStatusCodes.ContainsStatusCode(statusCode)) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
question: Sampling Decision
From the dev docs:
If the value of this attribute matches one of the status codes in
traceIgnoreStatusCodes, the SDK MUST set the transaction's sampling decision tonot sampled.
Should this have other effects as well,
like setting the "sample_rate" on the DynamicSamplingContext as well,
or am I misunderstanding something there?
There was a problem hiding this comment.
At this stage, the Dynamic Sampling Context is no longer relevant... the transaction tracer has already been finished and a snapshot of the transaction has been created in the form of a SentryTransaction (a more or less immutable DTO - certainly it's not used for tracing any longer). Any outbound requests that were made using the DSC have already been completed so there's no way to retrospectively change the sample rate that was communicated for those.
| public string? CacheDirectoryPath { get; set; } | ||
| public bool? CaptureFailedRequests { get; set; } | ||
| public List<string>? FailedRequestTargets { get; set; } | ||
| public List<int>? TraceIgnoreStatusCodes { get; set; } |
There was a problem hiding this comment.
issue: potential inconsistency
I wanted to check whether we are consistent with the other IList<HttpStatusCodeRange>-based Option, which is FailedRequestStatusCodes.
But FailedRequestStatusCodes does not exist on the BindableSentryOptions.
Now I'm wondering, whether BindableSentryOptions.FailedRequestStatusCodes does not exist because
- we forgot it
- we didn't add it because of scalar status codes vs Status-Code-Ranges
If the reasoning is the latter, then I guess we should not add this BindableSentryOptions as well, for consistency.
If, instead of only allowing scalar bindable options,
we also want to be able to depict Ranges via BindableSentryOptions, perhaps we could enable parsing from
{
"Sentry": {
"TraceIgnoreStatusCodes": [[301, 303], [305, 399], [401, 404]],
"TraceIgnoreStatusCodes": [ {"start": 301, "end": 303}, {"start": 305, "end": 399}, {"start": 401, "end":404}]
}
}or something like that.
If we'd like to do that, for both this TraceIgnoreStatusCodes, and the already existing SentryOptions.FailedRequestStatusCodes, perhaps we want to hold off on this Bindable-Option for now, and introduce Bindable-HttpStatusCodeRange-Collections in a separate issue/PR.
What do you think?
There was a problem hiding this comment.
But FailedRequestStatusCodes does not exist on the BindableSentryOptions.
Ideally as many of the options as possible would also be configurable via bindings (e.g. apssettings.json).
I think I didn't solve for FailedRequestStatusCodes specifically because the broader AOT problem was huge in scope. The shortest path to completion was simply to omit any non trivial properties (primitives) from the bindable options and require SDK users configure these instead via code.
I'm not really sure how many people will use the configuration bindings for these two properties, so a relatively simple solution to start with is probably not a bad thing. This PR adds the ability to configure lists of explicit codes to ignore via bindings... if people want to do more complex things like ranges, that would have to be done in code.
We could create a custom binding that was more sophisticated... it's certainly technically possible but would date longer to build and test. We have a backlog of over 300 issues so I'd recommend we just go with this simpler solution for the time being (or omit the ability to configure trace ignore status codes via bindings entirely).
We could also, for consistency, add an issue to the backlog to enable some similar bindable options for FailedRequestStatusCodes... with a medium priority initially (unless we get feedback from the community that this would be useful).
There was a problem hiding this comment.
Added #5061 as a follow up task to address the inconsistency:
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
## DESCRIBE YOUR PR Documents `SentryOptions.TraceIgnoreStatusCodes` for the .NET SDK. The option shipped in 6.4.0 ([getsentry/sentry-dotnet#5034](getsentry/sentry-dotnet#5034)), but the .NET docs don't mention it. In 7.0.0 its default changes from an empty list to 301, 305–399 and 401–404 ([getsentry/sentry-dotnet#5632](getsentry/sentry-dotnet#5632)), so ASP.NET Core and ASP.NET apps will stop sending transactions for those responses. - Adds an `SdkOption` entry to the Tracing Options on the shared .NET options page. It covers what the option filters, how to set codes and ranges, the 7.0.0 default and why 302/303 aren't in it, how to trace everything again, and `appsettings.json`. - Adds a short mention, linking to the option, to the server integration sections of the ASP.NET Core and ASP.NET tracing pages. Notes for review: - The docs aren't versioned, so the entry describes 6.x (empty default) and says the default changes in 7.0.0. It's accurate today and can merge before 7.0.0 ships. - The entry is wrapped in a `PlatformSection` for the guides that run Sentry's ASP.NET or ASP.NET Core integration: ASP.NET, ASP.NET Core, Blazor Server, AWS Lambda and Google Cloud Functions. The option has no effect elsewhere. - "Outgoing `HttpClient` requests aren't affected" holds from 6.12.0 ([getsentry/sentry-dotnet#5643](getsentry/sentry-dotnet#5643)). From 6.4.0 to 6.11.x, an app using `UseOpenTelemetry` that set the option could also lose root `http.client` transactions whose downstream status matched. I left that fixed bug off the page. - With `UseOtlp`, spans go straight to Sentry's OTLP endpoint and never reach the SDK's transaction processors, so the option doesn't apply. The page says so in one line. - The `[]` vs `[0]` caveat for `appsettings.json` only matters from 7.0.0, when the default is non-empty, and is worded that way. Configuration can't express ranges yet ([getsentry/sentry-dotnet#5644](getsentry/sentry-dotnet#5644)). - The code sample uses a collection initializer rather than a collection expression, because ASP.NET apps on .NET Framework default to C# 7.3. It was compiled and run against Sentry 6.12.0. Closes getsentry/sentry-dotnet#5708 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. Select exactly one option. For deadlines, replace `YYYY-MM-DD` with the due date. You can update this information later by editing the PR description. - [ ] Urgent deadline (GA date, etc.): YYYY-MM-DD - [x] Other deadline: 2026-11-30 (planned sentry-dotnet 7.0.0 release) - [ ] No deadline: Not urgent, can wait up to 1 week+ ## PRE-MERGE CHECKLIST _Make sure you've checked the following before merging your changes:_ - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Ricardo Colombo Oliveira <github@ricoliv.com>
4 participants
resolves: #4739