fix: Implement flood control with 1024 max events #359
Conversation
|
#339 has been introduced because we saw quite a lot of "clicks go crazy" where several hundreds or even thousands were collected on some random elements. I agree that the fix provided back then is pretty limiting (only one click per element is tracked) and I agree with your potential fix. I am not sure that you will be able to deduct a Let's see what this gives us. |
rpapani
changed the title
| @@ -35,9 +35,12 @@ const blocksMO = window.MutationObserver ? new MutationObserver(blocksMCB) | |||
| const mediaMO = window.MutationObserver ? new MutationObserver(mediaMCB) | |||
| /* c8 ignore next */ : {}; | |||
|
|
|||
| let maxEvents = 1023; | |||
There was a problem hiding this comment.
@trieloff Since I'm decrementing this, using maxEvents instead of MAX_EVENTS. If using const is recommended, then I'll do something like
const MAX_EVENTS = 1023;
let maxEvents = MAX_EVENTS;
Please let me know if this way is preferred.
modules/index.js
Outdated
| function trackCheckpoint(checkpoint, data, t) { | ||
| const { weight, id } = window.hlx.rum; | ||
| if (isSelected) { | ||
| if (isSelected && maxEvents > 0) { |
There was a problem hiding this comment.
| if (isSelected && maxEvents > 0) { | |
| if (isSelected && --maxEvents > 0) { |
If ESLint doesn't complain, then I'd one-liner this.
There was a problem hiding this comment.
| if (isSelected && maxEvents > 0) { | |
| if (isSelected && --maxEvents) { |
every byte counts.
There was a problem hiding this comment.
yes, lint is complaining for no-plusplus, so, either we need to add the eslint-disable or avoid --, I'll be using the latter.
test/it/click.test.html
Outdated
| a.click(); | ||
| await new Promise((resolve) => setTimeout(resolve, 1)); | ||
| } | ||
| assert(window.events.length === 1024 , '1024th event should not trigger new checkpoint'); |
There was a problem hiding this comment.
| assert(window.events.length === 1024 , '1024th event should not trigger new checkpoint'); | |
| assert(window.events.length === 1024 , '1025th event should not trigger new checkpoint'); |
| @@ -35,9 +35,12 @@ const blocksMO = window.MutationObserver ? new MutationObserver(blocksMCB) | |||
| const mediaMO = window.MutationObserver ? new MutationObserver(mediaMCB) | |||
| /* c8 ignore next */ : {}; | |||
|
|
|||
| let maxEvents = 1023; | |||
There was a problem hiding this comment.
I would document why 1023...
There was a problem hiding this comment.
Looks like we've a byte limit, so adding the documentation may take more. Is it ok to add that in the commit?
@trieloff WDYT?
There was a problem hiding this comment.
Comments are removed thus all good.
|
🎉 This PR is included in version 2.30.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.31.0-beta.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
If the end users are clicking on an element repetitively, it typically indicates a problem like broken experience, confusing UI etc. #339 introduced to not to track the repeating clicks, because of which we won't be able to find some of the issues faced by users.
Currently, PSS considers if the user is clicking 10 times on the same selector with in the same session, we'll record as a rageclick (naming is little different in PSS) and based on such instances, we'll create an opportunity for our customers to fix. For PSS to continue to find these blind clicks and help our customers, we'd like to have the repeating clicks collected as valid RUM checkpoints.
This PR will introduce the flood control to limit the max events collection to 1024.
Related PRs - #339