Skip to content
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

[REPLAY] Discard mouse/touch event without x/y position #1993

Merged

Conversation

ThibautGeriz
Copy link
Contributor

@ThibautGeriz ThibautGeriz commented Feb 2, 2023

Motivation

We have a bunch of error in the player where we have mouse record without x/y with numeric value.

The hypothesis is that the event are not generated by the browser but by JS using dispatchEvent

Changes

  • Discard mouse/touch event without x/y position
  • Add telemetry with isTrusted to validate/invalidate the theory

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@ThibautGeriz ThibautGeriz force-pushed the thibaut.gery/discard-mousetouch-record-without-x-y branch 2 times, most recently from c27f7c0 to 7536f20 Compare February 2, 2023 16:43
@ThibautGeriz ThibautGeriz force-pushed the thibaut.gery/discard-mousetouch-record-without-x-y branch from 7536f20 to 100d9e5 Compare February 2, 2023 16:53
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2023

Codecov Report

Merging #1993 (1ab44fb) into main (a6533af) will increase coverage by 0.04%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
+ Coverage   93.37%   93.41%   +0.04%     
==========================================
  Files         145      145              
  Lines        5511     5515       +4     
  Branches     1256     1258       +2     
==========================================
+ Hits         5146     5152       +6     
+ Misses        365      363       -2     
Impacted Files Coverage Δ
packages/rum/src/domain/record/observers.ts 81.86% <92.30%> (+4.86%) ⬆️
...s/rum-core/src/domain/contexts/pageStateHistory.ts 80.48% <0.00%> (-12.20%) ⬇️
...rum-core/src/domain/contexts/foregroundContexts.ts 90.76% <0.00%> (-3.08%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ThibautGeriz ThibautGeriz marked this pull request as ready for review February 2, 2023 16:59
@ThibautGeriz ThibautGeriz requested review from a team as code owners February 2, 2023 16:59

sandbox = document.createElement('div')
a = document.createElement('a')
a.setAttribute('tabindex', '0') // make the element focusable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 suggestion: ‏the element does not need to be focusable for this test, you could remove the tabindex attribute

💬 suggestion: ‏in fact, you don't need an element for this test, you could simply dispatch the event on document.body, so you could remove sandbox and a

Comment on lines 162 to 165
addTelemetryDebug('mouse/touch event without x/y', {
isTrusted: event.isTrusted,
position: { x, y },
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ question: ‏ What will those telemetry logs used for?

💬 suggestion: ‏ If it's to see whether ignoring untrusted events would help, I would suggest emiting a telemetry log only when the event is untrusted. This would reduce the amount of unneeded logs messages.

Suggested change
addTelemetryDebug('mouse/touch event without x/y', {
isTrusted: event.isTrusted,
position: { x, y },
})
if (event.isTrusted) {
addTelemetryDebug('mouse/touch event without x/y', {
position: { x, y },
})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, if we have event without x/y that are both trusted and not trusted I would like to know about it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will those telemetry logs used for?

I want to know if the next major release will fix this issue for and we can remove that code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know that some untrusted events don't have coordinates. I don't think we need to confirm that anymore, since we can easily reproduce it.

Now, what I'm worried about is the amount of telemetry logs that could be sent. If you still think that we should add a log when an untrusted event don't have coordinates, I would suggest to limit the amount of telemetry debug by only logging the first one, something like:

let wasEventWithoutCoordinatesLogSent = false

function tryComputeCoordinates(event) {
  ...
  if (!wasEventWithoutCoordinatesLogSent && !Number.isFinite(...) && ...) {
    wasEventWithoutCoordinatesLogSent = true
    addTelemetryDebug('mouse/touch event without x/y', {
      isTrusted: event.isTrusted,
      position: { x, y },
    })
  }
  ...
}

Copy link
Contributor Author

@ThibautGeriz ThibautGeriz Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I see your point, I will only send the telemetry if event is trusted

packages/rum/src/domain/record/observers.ts Outdated Show resolved Hide resolved
Comment on lines 162 to 164
addTelemetryDebug('mouse/touch event without x/y', {
isTrusted: event.isTrusted,
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥜 nitpick: ‏ ‏you could also move the telemetry debug in tryComputeCoordinates

packages/rum/src/domain/record/observers.ts Outdated Show resolved Hide resolved
@ThibautGeriz ThibautGeriz merged commit 2941685 into main Feb 3, 2023
@ThibautGeriz ThibautGeriz deleted the thibaut.gery/discard-mousetouch-record-without-x-y branch February 3, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants