-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(develop): document traceIgnoreStatusCodes
#15026
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Bundle ReportChanges will decrease total bundle size by 6 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-client-array-pushAssets Changed:
|
adinauer
left a comment
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.
Mostly LGTM. Do we already know how we want to report this in client reports? Should it be event_processor or have a separate reason?
Clarify that traceIgnoreStatusCodes applies only to server SDKs and incoming requests.
I think |
Lms24
left a comment
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.
The JS Node SDKs already have an option on our server-side http integration (basically the place that starts traces for all incoming http requests in Node severs) which is called dropSpansForIncomingRequestStatusCodes. I think it makes sense to eventually lift this to a top-level init option with a better name.
However, one thing I would consider for traceIgnoreStatusCodes is that it should also accept an inclusive interval as input, for example something like:
traceIgnoreStatusCodes: [[401, 404], [301, 303], 305, 429]
This is a lot easier for users to handle than to expect them to write out the e.g. 35+ 4xx status codes. Instead, they'd simply define [[400, 499]].
WDYT?
final suggestions: I think a couple of examples for such options are always helpful in develop docs, especially if we're going with the interval syntax.
| The SDK should log a debug level message (when debug logging is enabled with `debug` == `true` in the client options) denoting why the transaction was dropped. | ||
|
|
||
| This option must default to an empty sequence if it's introduced in a release with a minor SemVer bump. | ||
| SDKs should set the default for this option to `[404]` at the earliest release with a major SemVer bump following its introduction. |
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.
we currently default to the following range of ignored status codes: [[401, 404], [301, 303], [305, 399]].
For example, users often complain about traced redirect responses for moved resources. Similarly getting permission denied errors/forbidden (401/403) for bots sending requests to protected endpoints is equally spammy as 404s. I think we want to be a bit more aggressive in terms of default values but happy go with slimmer minimum defaults if you/other reviewers prefer.
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.
Overall it seems the default you propose makes sense.
Even if you get one of these and you don't trace it, you'll get an error/trace on the client that will tell you what happened (if there's a real bug and it's not a bot hitting the route, that is).
Just wondering if this has been released already, and if anyone complained about these new defaults?
Updated language to clarify requirements and defaults for the traceIgnoreStatusCodes option.
Clarified terminology from 'sequence' to 'collection' for default option.
Clarified the wording regarding HTTP status code ranges for the traceIgnoreStatusCodes option.
|
Addressed all comments, thanks everyone. |
Lms24
left a comment
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.
Thanks for addressing the suggestions, LGTM!
DESCRIBE YOUR PR
Documents the
traceIgnoreStatusCodesoption for SDKs.This is a proposal on how to implement it.
This contrasts with the implementation in JS because there it's an option on
httpIntegration.However, to make this work for OTEL instrumentation too, we need to make it a client option (as in POTEL SDKs there might be no concept of OTEL "integration" to pass parameters to), and rely on the span attribute (because that's the only thing the SDK sees from the spans generated by OTEL instrumentation).
Another option in non-POTEL SDKs would be to implement it as an option on each integration, but this way to specify the option and implement it makes it consistent across SDKs (regardless of POTEL/non-POTEL).