-
-
Notifications
You must be signed in to change notification settings - Fork 230
feat: Add option to exclude certain HTTP statuses from tracing #5034
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
5286daa
5c17c82
e9e00d9
edb9820
09b4a44
16e2759
dc9fd97
6dd6af8
3b4f7ff
9393cdc
e90dd9b
902f6c3
a2e3157
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| using Sentry.Extensibility; | ||
| using Sentry.Internal.OpenTelemetry; | ||
|
|
||
| namespace Sentry.Internal; | ||
|
|
||
| internal class TraceIgnoreStatusCodeTransactionProcessor : ISentryTransactionProcessor | ||
| { | ||
| private readonly SentryOptions _options; | ||
|
|
||
| public TraceIgnoreStatusCodeTransactionProcessor(SentryOptions options) | ||
| { | ||
| _options = options; | ||
| } | ||
|
|
||
| public SentryTransaction? Process(SentryTransaction transaction) | ||
| { | ||
| if (_options.TraceIgnoreStatusCodes.Count == 0) | ||
| { | ||
| return transaction; | ||
| } | ||
|
|
||
| if (transaction.Data.TryGetValue(OtelSemanticConventions.AttributeHttpResponseStatusCode, out var statusCodeObj) | ||
| && statusCodeObj is int statusCode | ||
jamescrosswell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
jamescrosswell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| && _options.TraceIgnoreStatusCodes.ContainsStatusCode(statusCode)) | ||
|
Check failure on line 24 in src/Sentry/Internal/TraceIgnoreStatusCodeTransactionProcessor.cs
|
||
| { | ||
| return null; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Sampling Decision From the dev docs:
Should this have other effects as well,
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
jamescrosswell marked this conversation as resolved.
Show resolved
Hide resolved
jamescrosswell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return transaction; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| using Sentry.Internal.OpenTelemetry; | ||
|
|
||
| namespace Sentry.Tests; | ||
|
|
||
| public class TraceIgnoreStatusCodeTransactionProcessorTests | ||
| { | ||
| private static SentryOptions OptionsWithIgnoredCodes(params HttpStatusCodeRange[] ranges) | ||
| { | ||
| var options = new SentryOptions(); | ||
| foreach (var range in ranges) | ||
| { | ||
| options.TraceIgnoreStatusCodes.Add(range); | ||
| } | ||
| return options; | ||
| } | ||
|
|
||
| private static SentryTransaction TransactionWithStatusCode(int statusCode) | ||
| { | ||
| var transaction = new SentryTransaction("name", "operation"); | ||
| transaction.SetData(OtelSemanticConventions.AttributeHttpResponseStatusCode, statusCode); | ||
| return transaction; | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Process_EmptyIgnoreList_ReturnsTransaction() | ||
| { | ||
| // Arrange | ||
| var options = new SentryOptions(); | ||
| var processor = new TraceIgnoreStatusCodeTransactionProcessor(options); | ||
jamescrosswell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| var transaction = TransactionWithStatusCode(404); | ||
|
|
||
| // Act | ||
| var result = processor.Process(transaction); | ||
|
|
||
| // Assert | ||
| result.Should().BeSameAs(transaction); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Process_StatusCodeNotInIgnoreList_ReturnsTransaction() | ||
| { | ||
| // Arrange | ||
| var options = OptionsWithIgnoredCodes(404); | ||
| var processor = new TraceIgnoreStatusCodeTransactionProcessor(options); | ||
| var transaction = TransactionWithStatusCode(200); | ||
|
|
||
| // Act | ||
| var result = processor.Process(transaction); | ||
|
|
||
| // Assert | ||
| result.Should().BeSameAs(transaction); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Process_StatusCodeInIgnoreList_ReturnsNull() | ||
| { | ||
| // Arrange | ||
| var options = OptionsWithIgnoredCodes(404); | ||
| var processor = new TraceIgnoreStatusCodeTransactionProcessor(options); | ||
| var transaction = TransactionWithStatusCode(404); | ||
|
|
||
| // Act | ||
| var result = processor.Process(transaction); | ||
|
|
||
| // Assert | ||
| result.Should().BeNull(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Process_StatusCodeInIgnoredRange_ReturnsNull() | ||
| { | ||
| // Arrange | ||
| var options = OptionsWithIgnoredCodes((400, 499)); | ||
| var processor = new TraceIgnoreStatusCodeTransactionProcessor(options); | ||
| var transaction = TransactionWithStatusCode(404); | ||
|
|
||
| // Act | ||
| var result = processor.Process(transaction); | ||
|
|
||
| // Assert | ||
| result.Should().BeNull(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Process_StatusCodeOutsideIgnoredRange_ReturnsTransaction() | ||
| { | ||
| // Arrange | ||
| var options = OptionsWithIgnoredCodes((400, 499)); | ||
| var processor = new TraceIgnoreStatusCodeTransactionProcessor(options); | ||
| var transaction = TransactionWithStatusCode(500); | ||
|
|
||
| // Act | ||
| var result = processor.Process(transaction); | ||
|
|
||
| // Assert | ||
| result.Should().BeSameAs(transaction); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Process_NoStatusCodeExtra_ReturnsTransaction() | ||
| { | ||
| // Arrange | ||
| var options = OptionsWithIgnoredCodes(404); | ||
jamescrosswell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| var processor = new TraceIgnoreStatusCodeTransactionProcessor(options); | ||
| var transaction = new SentryTransaction("name", "operation"); | ||
|
|
||
| // Act | ||
| var result = processor.Process(transaction); | ||
|
|
||
| // Assert | ||
| result.Should().BeSameAs(transaction); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Process_MultipleIgnoredCodes_MatchesAny() | ||
| { | ||
| // Arrange | ||
| var options = OptionsWithIgnoredCodes(404, 429); | ||
| var processor = new TraceIgnoreStatusCodeTransactionProcessor(options); | ||
|
|
||
| // Act & Assert | ||
| processor.Process(TransactionWithStatusCode(404)).Should().BeNull(); | ||
| processor.Process(TransactionWithStatusCode(429)).Should().BeNull(); | ||
| processor.Process(TransactionWithStatusCode(200)).Should().NotBeNull(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: potential inconsistency
I wanted to check whether we are consistent with the other
IList<HttpStatusCodeRange>-based Option, which isFailedRequestStatusCodes.But
FailedRequestStatusCodesdoes not exist on theBindableSentryOptions.Now I'm wondering, whether
BindableSentryOptions.FailedRequestStatusCodesdoes not exist becauseIf the reasoning is the latter, then I guess we should not add this
BindableSentryOptionsas 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 existingSentryOptions.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
FailedRequestStatusCodesspecifically 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #5061 as a follow up task to address the inconsistency: