feat: Add option to exclude certain HTTP statuses from tracing#5034
feat: Add option to exclude certain HTTP statuses from tracing#5034jamescrosswell wants to merge 13 commits intomainfrom
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}
jamescrosswell
changed the base branch from
main
to
fix/transaction/tracer-setdata-not-sent
jamescrosswell
changed the base branch from
fix/transaction/tracer-setdata-not-sent
to
main
| && 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.
src/Sentry/Internal/TraceIgnoreStatusCodeTransactionProcessor.cs
Outdated
Show resolved
Hide resolved
| 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:
test/Sentry.Tests/TraceIgnoreStatusCodeTransactionProcessorTests.cs
Outdated
Show resolved
Hide resolved
![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.
src/Sentry/Internal/TraceIgnoreStatusCodeTransactionProcessor.cs
Outdated
Show resolved
Hide resolved
3 participants
resolves: #4739