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
feat(node): Add tracePropagationTargets
option
#5521
Conversation
size-limit report 📦
|
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.
This looks great. And I agree, having a browser/node-specific JSDoc justifies defining the option twice.
Just had some minor questions and remarks but nothing blocking.
Also, thanks for taking the time and adding all these tests despite this change being tough to test properly!
@@ -90,7 +103,26 @@ type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedR | |||
function _createWrappedRequestMethodFactory( | |||
breadcrumbsEnabled: boolean, | |||
tracingEnabled: boolean, | |||
tracePropagationTargets: (string | RegExp)[] | undefined, |
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.
optional/question: Does it make sense to extract (string | RegExp)[] | undefined
to a type?
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.
Yeah that makes a lot of sense! Can you check whether the location where I extracted it makes sense?: 0aa153f
Also I extracted it without | undefined
because that works better with the optional operator in the init options.
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.
I think it sort of makes sense but I can see why this might look a little off. Seems like we don't really have a tracing-related file in the types package. Adding it to the transaction, span or baggage files also doesn't make more sense...
So wdyt about adding a new tracing.ts
file? Or we add it to the misc.ts
file ?
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.
Created a tracing.ts file. Thanks for the input!
packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts
Show resolved
Hide resolved
Co-authored-by: Lukas Stracke <lukas@stracke.co.at>
Co-authored-by: Lukas Stracke <lukas@stracke.co.at>
This PR adds the
tracePropagationTargets
option as outlined in #5403, getsentry/develop#636, and DACI (internal) to the node SDK init options.The
tracePropagationTargets
option can be used to control to which request the SDK should attach trace data (at the time of writing viasentry-trace
andbaggage
headers).Its behavior (at least for node) is as follows: If not provided, the SDK will attach tracing data to all outgoing requests. If provided, the SDK will only attach trace data when the request's URL matches one of the items in the
tracePropagationTargets
array.Resolves #5403
Notes: