feat(tracing): Add hook for trace sampling function to SDK options #2820
Conversation
size-limit report
|
lobsterkatie
force-pushed
the
kmclb-add-traces-sampler-option
branch
3 times, most recently
from
d157b23 to
0ba634a
Compare
There was a problem hiding this comment.
My main concern here is whether we can have a clear and consistent type def for SamplingContext, not an arbitrary mapping of strings to anything.
That should make documentation easier and usage easier.
Looking at the impl. I'm also not convinced it has to be added to startTransaction as a new argument. If we consider Hub is an interface, changing method signatures is a breaking change.
Perhaps this is just because there's no usage example yet -- tests would help clarify what are the use cases being addressed.
packages/gatsby/README.md
Outdated
| tracesSampleRate: 1, // this is just to test, you should lower this in production | ||
|
|
||
| // A rate of 1 means all traces will be sent, so it's good for testing. | ||
| // In production, you'll likely want to either choose a lower rate or use `tracesSampler` instead. |
There was a problem hiding this comment.
I like how clear and concise this is.
Noting that we may have to apply a similar change to other places that mention tracesSampleRate.
If this type of documentation is only in packages/gatsby, we could also consider replacing it with a link to the official docs, and then maintain only one copy of the example.
In addition, we could provide an example that uses tracesSampler instead of tracesSampleRate, so that users looking at this don't need to go figure out what "use tracesSampler instead" entails.
There was a problem hiding this comment.
Have updated the gatsby readme and will come back to the question of linking when I write the docs for this.
packages/types/src/options.ts
Outdated
| @@ -84,7 +85,7 @@ export interface Options { | |||
| * 0.0 = 0% chance of a given trace being sent (send no traces) | |||
| * 1.0 = 100% chance of a given trace being sent (send all traces) | |||
| * | |||
| * Default: 0.0 | |||
| * Can be omitted without disabling tracing if `tracesSampler` is defined. If neither is defined, tracing is disabled. | |||
There was a problem hiding this comment.
Too many conditions and negations for humans to parse here.
Please put tracesSampler close together to this option -- they are related. Then we can explain:
To enable tracing, either set
tracesSampleRateto a non-zero value or settracesSampler.
There was a problem hiding this comment.
The linter insists on having all properties before any methods, so I moved tracesSampleRate to be the last property and tracesSampler to be the first method, so they're together. I also reworded the description. LMK if you feel like it's clearer now.
| // for all node servers that we currently support, we store the incoming request object (which is an instance of | ||
| // http.IncomingMessage) on the domain | ||
|
|
||
| // the domain members are stored as an array, so our only way to find the request is to iterate through the array | ||
| // and compare types |
There was a problem hiding this comment.
This sounds rather sketchy. Is this existing behavior? Does it work properly under concurrent requests?
There was a problem hiding this comment.
It is in fact the existing behavior, and my understanding is that scope/thread/concurrency safety the reason we still use the deprecated domain package, because we don't have another way to accomplish that. @kamilogorek is the expert here, though.
| const request = domain.members.find((member: any) => isInstanceOf(member, requestType)); | ||
|
|
||
| if (request) { | ||
| defaultSampleContext.request = extractNodeRequestData(request); |
There was a problem hiding this comment.
Could we unify the type definition of SampleContext for node + browser?
SampleContext.url = ...
SampleContext.method = ...What's the advantage of having request in node and location in browser?
The drawback include more complex documentation and less specific type definition for SampleContext (which ties back to documentation and helping people implement their custom samplers).
There was a problem hiding this comment.
The idea is to give people as much data as is easily grabbed. Naturally, that data is going to be different depending on the context - browser window, service worker, or node server.
lobsterkatie
force-pushed
the
kmclb-add-traces-sampler-option
branch
from
88596ae to
56f8b6e
Compare
|
@rhcarvalho I've tried to address your concerns re: specificity (or lack thereof) in the definition of the
LMK what you think. /**
* Context data passed by the user when starting a transaction, to be used by the tracesSampler method.
*/
export interface CustomSampleContext {
[key: string]: any;
}
/**
* Data passed to the `tracesSampler` function, which forms the basis for whatever decisions it might make.
*
* Adds default data to data provided by the user. See {@link Hub.startTransaction}
*/
export interface SampleContext extends CustomSampleContext {
/**
* Object representing the URL of the current page or worker script. Passed by default in a browser or service worker
* context.
*/
location?: Location | WorkerLocation;
/**
* Object representing the incoming request to a node server. Passed by default when using the TracingHandler.
*/
request?: ExtractedNodeRequestData;
} |
lobsterkatie
force-pushed
the
kmclb-add-traces-sampler-option
branch
3 times, most recently
from
7150597 to
ce227d0
Compare
| extensions?: { | ||
| /** Hack to prevent bundlers from breaking our usage of the domain package in the cross-platform Hub package */ | ||
| domain?: typeof domain & { | ||
| /** | ||
| * The currently active domain. This is part of the domain package, but for some reason not declared in the | ||
| * package's typedef. | ||
| */ | ||
| active?: domain.Domain; | ||
| }; | ||
| } & { | ||
| /** Extension methods for the hub, which are bound to the current Hub instance */ | ||
| // eslint-disable-next-line @typescript-eslint/ban-types | ||
| [key: string]: Function; | ||
| }; |
There was a problem hiding this comment.
H: Are we sure that doesn't break in Browser environments?
There was a problem hiding this comment.
I will check again, for sure, but the change here isn't a behavior change but merely a TS one. We've always put the domain there, it's just that before we didn't include it in the type so we had to do a lot of eslint-disable @no-unsafe-member-access and eslint-disable @no-explicit-any. My understanding is that TS stuff all gets stripped?
Anyway, will check and update here.
There was a problem hiding this comment.
UPDATE: Seems to build just fine, with no special accommodation in my set up.
There was a problem hiding this comment.
@lobsterkatie It seems like these changes broke @sentry/browser for TS users by requiring node types 😕 issue: #2927
Okay, switched from passing That said, I think inheritance is a common and important enough concern that I'd like to be able to do better than "this existing decision probably came from the parent, but honestly, we don't know for sure." Okay, I lied. This should get decided before we merge |
lobsterkatie
force-pushed
the
kmclb-add-traces-sampler-option
branch
from
b707cfc to
8572b08
Compare
| * The currently active domain. This is part of the domain package, but for some reason not declared in the | ||
| * package's typedef. | ||
| */ | ||
| active?: domain.Domain; |
There was a problem hiding this comment.
We shouldn't be using @types/node in an isomorphic package.
|
@kamilogorek @lobsterkatie This feature introduced #2971 some months ago, the only solution for many is to down level the package version. It appears to have fallen off the radar despite a number of us customers having this problem. We're now pushing on three months with no feedback, let alone a fix. |
|
Can someone front Sentry at least provide some feedback, even if it's as trite as "We have no plans to resolve this..." so customers like us can make some decisions? @kamilogorek @lobsterkatie |
|
@dvins please try https://github.com/getsentry/sentry-javascript/releases/tag/6.0.4. #2971 is now closed, if there's anything else preventing you from upgrading please let us know in a new issue. Thanks! |
Currently, tracing is either on, with a static sample rate for all transactions, or off. This doesn't let users either
a) sample different transactions at different rates or
b) filter out certain transactions all together.
This PR introduces a new SDK option,
tracesSampler, whose value is a callback with the signaturetracesSampler(samplingContext: SamplingContext): number | boolean,where
samplingContextis a combination of automatically-provided data (varying by SDK/integration) and data passed by the user tostartTransaction()as an optional third parameter. Given that data, thetracesSamplerfunction can then compute the appropriate sample rate and return it to the sampling function.Example use cases for variable sample rate:
Example use cases for filtering:
(A user can, of course, combine these ideas as well, returning a variable sample rate for some traces and 0 to filter out others.)
In order to support this addition, a few other changes needed to be made:
@sentry/utilsmisc.tsto eliminate circular dependencies within@sentry/utilsWorkerLocationtypeStill to do:
SampleContextobject, and put it therelinting andsize check errorsisNaNin rate validatorsampleContextdepending on the SDK)