feat(tracing): Expose cancelIdleTimeout and add option to make it permanent #7236
Conversation
Replay SDK metrics 🚀
develop |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
|---|---|---|---|---|---|---|---|---|---|
| 12e34d4 | +28.57 ms | +0.00 ms | +5.77 pp | +930.12 kB | +1.04 MB | +2.26 kB | +41 B | +1 | +109.67 ms |
| c46c56c | +65.45 ms | -0.00 ms | +5.38 pp | +930.26 kB | +1.07 MB | +2.21 kB | +41 B | +1 | +91.29 ms |
| 7f4c4ec | +56.64 ms | -0.00 ms | +5.57 pp | +927.42 kB | +1.06 MB | +2.21 kB | +41 B | +1 | +110.83 ms |
| 00d2360 | +55.18 ms | +0.00 ms | +2.23 pp | +934.14 kB | +1.05 MB | +2.22 kB | +41 B | +1 | +71.65 ms |
Last updated: Tue, 21 Feb 2023 10:34:55 GMT
size-limit report 📦
|
|
Can you add some more details about why we need |
krystofwoldrich
changed the title
restartIdleTimeout to the public interface
|
@AbhiPrasad I've updated the readme with more detail. |
|
This is ready for feedback. 🙏 |
|
Hi @krystofwoldrich! 👋 The Performance team is also working on UI event transactions for Frontend transactions this quarter, and we had a question about this: For what reason do we want to keep the transaction running if the same UI element is being interacted with? This might not result in the best product experience, because imagine a scenario where a user repeatedly clicks on an element which fetches data -> the transaction will end up extremely large. Transactions like this won't be a good representation of the spans involved in an interaction -> paint loop. |
|
👋 @0Calories Thanks for the reply. I imagine a refresh scenario, a user is tapping a refresh button. In that case, I would like to see all the requests in one transaction as they are related to each other. If the tapping of the button would cause multiple transactions to be created we would lose the information about the sequence. There might be some broken requests before the first successful one. After the idle timeout, the transaction will be finished and on the next tap, a new one will be created. The scenarios are described here. Maybe @philipphofmann knows more reasons why to keep the same interaction with the same element in one transaction. |
|
@krystofwoldrich I had read all of the scenarios and most of them make sense to me 👍 Regarding your scenario with a user repeatedly tapping a refresh button, I can see why this would be valuable. However, the Performance product was built around the idea that one transaction would represent a singular action (i.e pageload, navigation, click), and excessive data stored within interactions would muddle up our metrics and make it harder to understand the true overall performance of that action. The scenario you describe sounds like it would better served by Sessions / Replay, since it already captures all interactions a user is performing within a session |
|
@0Calories So you would treat |
|
But regardless of this one scenario, we need to expose |
|
@0Calories we've mainly decided to do that to save user's transaction quota if someone is rage-tapping on a button, for example. It can indeed result in endless transactions, but those we should prevent with a Btw, this is also the reason we're dropping child-less transactions now - to save the transaction quota, as these transactions don't really bring any value if there are no spans in them. With introduction of INP this may change though. |
|
We added UI event transactions to collect more spans. Our SDKs can't reliably detect the end of a UI event transaction. If a user clicks a button multiple times, we thought it makes sense not to finish the ongoing transaction cause it doesn't mark the finish point of it, while a different event most likely does. We can reconsider this behavior, though. Now that I think about it again, I think the same behavior as |
|
I do think that @romtsn on the topic of transaction quota being consumed by users who could rage click, I agree this is a valid concern, but I was thinking that user interaction transactions would be sampled at a much lower rate to prevent this problem. We haven't yet thought into how this would tie into DS, but at the very least I was thinking we could have an SDK level sample rate for interaction transactions that could be configurable, but would default to a low rate like 0.05 Also, child-less transactions should definitely be discarded — I don't think they'd be useful even for INP. |
I think @k-fish mentioned that we actually should still capture them in case INP is under the bad threshold. I think it makes sense, because if there are some operations triggered by the user interaction that we don't capture as spans (some heavy animations, or just some computation on the main thread, etc.), it would still contribute to bad INP, despite being child-less. But Kevan to confirm. |
|
Let's move the UI Event Transactions changes to the develop docs so this PR is not blocked. |
|
Sorry for the late response 🙏
I like this, since that's exactly what's happening, who knows if a rage click on a slow loading interaction kills the INP interaction? It's good to have them split up instead of multiple actions (click etc.) inside the same transaction.
Yes 100%, if it's the INP interaction transaction we have to send it in since otherwise we won't be properly collecting that measurement, and it's a good indication we should have spans there. For other span-less interactions I don't think it matters as much since they aren't very interesting, ideally if the browser is reporting that it's a long delay between action and paint, even if it doesn't have a long task or http request it still means something is happening that might be concern, so we might want to revisit this later, but for now this works (letting the child-less INP interaction through). edit: sorry clicked enter which closed this prematurely 😆 Aside:
|
|
I created an issue to change this behaviour on Android and iOS getsentry/team-mobile#85 |
|
Can someone review this? It's blocking this RN PR: |
Agree! On mobile we could actually already point people to profiles and from there it should be rather clear what's going on, even if there are no child spans. |
AbhiPrasad
changed the title
|
Thank you 🙂 |
krystofwoldrich
changed the title
This relates to UI Event Transactions and its implementation in RN.
Public
cancelIdleTimeoutwith a permanent option will be used in case of the scenario:Same UI element with the different event. We want to finish the current UI Event Transaction by the end of the last span and we don't need theIdleTrasactionto wait anymore as the newly created span will be assigned to the newly started transaction for thedifferent user interaction event.Context:
Todo