feat(profiling): add profilesSampler #109
Conversation
| } | ||
|
|
||
| // if the function returned 0 (or false), or if `profileSampleRate` is 0, it's a sign the profile should be dropped | ||
| if (!profilesSampleRate) { |
There was a problem hiding this comment.
Does this case need to exist? It gets handled by the if (!sampled) check below right?
There was a problem hiding this comment.
I think so, at least the tracing SDK seems to do this + the reasons are subtly different. The first check checks if profilesSampleRate was 0 or inherited 0 and second check checks if we are discarding as a consequence of the dice roll.
src/hubextensions.ts
Outdated
| } else if (customSamplingContext && customSamplingContext['parentSampled'] !== undefined) { | ||
| profilesSampleRate = customSamplingContext['parentSampled']; |
There was a problem hiding this comment.
Is this the sampling decision on the parent transaction? If that is the case, you're effectively saying sample all transactions which I don't think is the intention here.
There was a problem hiding this comment.
Yes correct, I took this logic off tracing, but I could not find the actual reasoning for inheriting this from getsentry/sentry-javascript#3068
There was a problem hiding this comment.
So if I'm correct, then the reason for this is because for distributed tracing, when the initial transaction is sampled, we would want the entire trace to be sampled. i.e. the browser page load transaction is sampled, so we want to propagate that sampling decision to all the API calls so that those transactions are also sampled.
I don't think this is what we want to do in profiling, at least not right now. Because it's effectively ignoring the profile samples rate and saying profile 100% of transactions. Now, this may be something we want but it's definitely not something we've discussed since we've been focused on profiling individual platforms not if we want to profile the entire trace.
There was a problem hiding this comment.
Ahhhhh, ok. I was not sure what the use case was, thanks for explaining! I will remove it for now, but like you mentioned, this may be something we want to actually support and is worth bringing up to the team.
There was a problem hiding this comment.
FWIW, if we are to implement this inheriting, we should not inherit the trace's sampling decision. We would want to do something like pass a profiling sampling decision from the root transaction alongside the transaction's sampling decision so that the whole trace is profile but only when the root transaction is profiled.
But again, this is something we should discuss before moving forwards.
There was a problem hiding this comment.
Yeah, we would need to mark the profile sample decision and inherit that - I just commented out the code and the skipped the test.
There was a problem hiding this comment.
@Zylphrex I just remove the code, no reason to keep it as it's easy to add back whenever.
Add profilesSampler method with similar functionality as tracesSampler. If profilesSampler is set, prefer it's outcome vs profilingSampleRate