-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
types(node): add profilesSampleRate #6318
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.
So full disclosure, I haven't had a look at how profiling works under the hood but given that users have to add the ProfilingIntegration
in their init options, wouldn't it make more sense to pass the profilesSampleRate
as an option in the constructor of the integration? WDYT?
Replay is doing it this way at the moment: Replay Setup
So I elected for initially going with #6310, but I think it makes sense to keep it node only for now and do it in the browser later. A top level option for profiling matches tracing, and the other SDKs at the moment, so I would prefer that so that JavaScript is not the only SDK that works this way. @JonasBa can we add a doc string for this? We also need to update https://docs.sentry.io/platforms/node/configuration/options/ |
I didn't know this, so yes, in that case it makes sense to keep it as a top level option in the Node SDK. |
@@ -56,4 +56,6 @@ export interface NodeOptions extends Options<NodeTransportOptions>, BaseNodeOpti | |||
* Configuration options for the Sentry Node SDK Client class | |||
* @see NodeClient for more information. | |||
*/ | |||
export interface NodeClientOptions extends ClientOptions<NodeTransportOptions>, BaseNodeOptions {} | |||
export interface NodeClientOptions extends ClientOptions<NodeTransportOptions>, BaseNodeOptions { | |||
profilesSampleRate?: number; |
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.
Can we add a JSDoc here, explaining what this option does?
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.
Good call, I added it
Hey @JonasBa @Lms24 @AbhiPrasad |
Hey @tomgrossman, thanks for writing in! We will fix this! |
This is addressed by #6409, will be part of the next release. |
@tomgrossman oversight on my part - I should have double checked the type name 😞 |
Adds profilesSampleRate to the Node SDK init options.
Adding this because we got 3 emails last week about the typescript error by our customers and asking them to @ts-expect-error is not really acceptable.