-
-
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
feat(tracing): Add interaction transaction as an experiment #6210
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.
Looking good so far. I wonder if having a dedicated interactionTracesSampleRate
is the way to go in the long run (wouldn't this confuse users?). But y'all probably already had conversations about that so take it with a grain of salt.
if (_experiments?.interactionSampleRate ?? 0 > 0) { | ||
this._registerInteractionListener(_experiments?.interactionSampleRate ?? 0); |
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.
Nit but the linter probably complains about this in multiple occurances in this PR:
We don't use optional chaining and nullish coalescing operators in the SDK as their bundle size impact is higher than using "traditional" checks like
Also, for this particular case: It might be the missing morning coffee but I don't entirely understand the 0 > 0
check. Wouldn't this just evaluate to false
?
if (_experiments?.interactionSampleRate ?? 0 > 0) { | |
this._registerInteractionListener(_experiments?.interactionSampleRate ?? 0); | |
if (_experiments && _experiments.interactionSampleRate != null) { | |
this._registerInteractionListener(_experiments.interactionSampleRate || 0); |
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 the idea of the code you mentioned is to not register the listener when interactionSampleRate === 0
?
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.
Thanks for looking, both of you 👏
optional chaining and nullish coalescing operators in the SDK as their bundle size impac
Right 😄, I do know that but it's been a bit, I forgot. I'll re-arrange this. Too much product code recently 😏
not register the listener when interactionSampleRate === 0
The default experiments object that gets set into this should be making this 0, if it's either unset or 0, either way we do not want to add a listener as that would affect the side not on the experiment.
I wonder if having a dedicated interactionTracesSampleRate is the way to go in the long run
It's definitely not, you're right. I'd prefer we're smarter about which ones we send, always trying to make sure we have the one represented by INP when I add it, and leverage biasing in dynamic sampling. For now I've added this rate for experimenting since I'm concerned about sending many many times the transactions (interactions are way more frequent on an SPA) for our frontend than we currently do. In general I'd be leaning towards simple / no-config defaults but still offering control in some manner (either here or elsewhere, such as DS) if a user chooses to do so.
I can see users just wanting everything to be sent especially if they are smaller and are easily within their transaction quota. Something to consider before we un-experiment this, at least.
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.
added this rate for experimenting since I'm concerned about sending many many times the transactions
For now can we just use tracesSampler
to sample based on op name in the frontend and remove logic around a dedicated sample rate? The mobile SDK implementations do not have this. Saves us bundle size also.
I can see users just wanting everything to be sent especially if they are smaller and are easily within their transaction quota
In order to make sure we don't blow up quota we'll have to do the following.
- Gate this behind some kind of flag (ideally boolean), but default it to false
- Update the performance monitoring docs
Sentry.init
instructions to have the flag on by default, so new users get this functionality automatically - Flip the flag in the major version
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.
For now can we just use tracesSampler
I like that, fixed.
Also @k-fish mobile team merged in getsentry/develop#766: https://develop.sentry.dev/sdk/performance/ui-event-transactions/ |
This adds interactions as an experiment so we can try it out with Sentry SaaS before tuning it for general release. It's behind a rate as an experiment option and only makes idleTransactions on 'click' for now. Future considerations include us likely having differing idleTimeout settings, and being more clever with which data we include, but this should be enough to start experimenting.
b668242
to
b34bae9
Compare
Summary
This adds interactions as an experiment so we can try it out with Sentry SaaS before tuning it for general release. It's behind a rate as an experiment option and only makes idleTransactions on 'click' for now. Future considerations include us likely having differing idleTimeout settings, and being more clever with which data we include, but this should be enough to start experimenting.
For rollout, we can potentially release this as a branch / beta for our SaaS experiment first.
Other: