getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.76k stars 1.52k forks source link

Add beforeSendProfile #8655

Open Fwang36 opened 11 months ago

Fwang36 commented 11 months ago

Problem Statement

Users would like more control over which profiles get sent to Sentry.

The end timestamps of transactions are captured with beforeSendTransaction, so users are able to use beforeSendTransaction to determine the durations of transactions and decide which ones to send.

With beforeSendProfile, users would be able to target send profiles attached to slow transactions after a decision was made with beforeSendTransaction.

Solution Brainstorm

Add beforeSendProfile

Lms24 commented 11 months ago

Generally, this sounds worthwhile to me

@JonasBa what are your thoughts?

JonasBa commented 11 months ago

Yeah, makes sense to me. I am tempted to say users should use profilesSampler in this case with the reason being that there is no need to even start the profiler if you are going to discard the data.

I do think it makes sense to introduce beforeSendProfile so our APIs do no differ across event types (probably worth adding beforeSendReplay too, I'm not sure it exists today)

Lms24 commented 11 months ago

I am tempted to say users should use profilesSampler in this case with the reason being that there is no need to even start the profiler if you are going to discard the data

That's a good point, @Fwang36 did users mention cases that profilesSampler doesn't cover but beforeSendProfile would?

We also might want to mention profilesSampler in the docs. If I'm not missing something, I can't find documentation for it at the moment.

Fwang36 commented 11 months ago

For sampling with tracesSampler or profilesSampler, only the start timestamp is there since those functions run before the actual transaction. Because of that there's no way to determine whether the transaction is "slow" or not. beforeSendTransaction (and probably beforeSendProfile) runs after the transaction finishes, so it has the end timestamps of the transaction, allowing users to get the difference in the timestamps to determine whether a transaction is slow or fast.

So for example, with beforeSendProfile, it should be possible for a user to only send profiles that are the result of "slow" transactions

JonasBa commented 11 months ago

@Fwang36 I would not recommend anyone should do that unless the reason is cost optimization and even in that case, I would highly recommend this be a temporary solution.

The reason is that if you have no fast transactions, you will not be able to tell what a slow transaction is. This will affect how your dashboard looks and what numbers you are presented with. Performance is a moving target and the data you collect should represent the population of your users. The other way to look at this problem is to ask yourself, do you want to optimize performance for a very small subset of the population < 1% that experience very long delays or should you improve the p50 value and make the app faster for almost everyone?

These are the types of informed decisions you want to be able to make with the data you are collecting.

That said, if you absolutely cannot work around it, you can achieve this today by preventing the transaction from being sent. This will work because profiles are only ever sent in the same envelope as a transaction, thus not sending one will not send the other. Note: We keep a ring buffer of max 50 profiles that we keep in memory, so expect a small increase here if you are going to be discarding transactions.