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

👷‍♀️ [RUM 5088] Reduce INP Null Target #2950

Merged
merged 44 commits into from
Oct 10, 2024

Conversation

cy-moi
Copy link
Contributor

@cy-moi cy-moi commented Aug 26, 2024

Motivation

Save the selector in a map with timestamp as key to mitigate the null target situation.
More information in RFC.

Changes

After mitigation, about 75% of null target INP would have a selector.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@cy-moi cy-moi marked this pull request as ready for review August 26, 2024 15:17
@cy-moi cy-moi requested a review from a team as a code owner August 26, 2024 15:17
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.66%. Comparing base (74609cc) to head (6e862a6).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...core/src/domain/action/interactionSelectorCache.ts 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2950      +/-   ##
==========================================
+ Coverage   93.19%   93.66%   +0.47%     
==========================================
  Files         275      276       +1     
  Lines        7610     7626      +16     
  Branches     1707     1711       +4     
==========================================
+ Hits         7092     7143      +51     
+ Misses        518      483      -35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

cit-pr-commenter bot commented Aug 29, 2024

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 160.95 KiB 161.21 KiB 267 B +0.16%
Logs 56.00 KiB 56.00 KiB 0 B 0.00%
Rum Slim 109.53 KiB 109.79 KiB 267 B +0.24%
Worker 25.21 KiB 25.21 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base Average Cpu Time (ms) Local Average Cpu Time (ms) 𝚫
addglobalcontext 0.001 0.002 0.000
addaction 0.030 0.040 0.011
addtiming 0.001 0.001 -0.000
adderror 0.031 0.034 0.003
startstopsessionreplayrecording 0.846 0.887 0.041
startview 0.924 1.047 0.123
logmessage 0.019 0.020 0.001
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 7.67 KiB 10.08 KiB 2.41 KiB
addaction 38.77 KiB 40.74 KiB 1.97 KiB
addtiming 6.05 KiB 7.72 KiB 1.68 KiB
adderror 42.53 KiB 45.44 KiB 2.91 KiB
startstopsessionreplayrecording 3.72 KiB 4.02 KiB 303 B
startview 458.87 KiB 467.43 KiB 8.55 KiB
logmessage 39.88 KiB 40.29 KiB 415 B

🔗 RealWorld

@cy-moi cy-moi force-pushed the congyao/RUM-5590-add-telemetry-for-INP-null branch from d321307 to add9d1a Compare August 30, 2024 09:07
@cy-moi cy-moi marked this pull request as draft August 30, 2024 13:54
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Sep 10, 2024
@cy-moi
Copy link
Contributor Author

cy-moi commented Sep 10, 2024

/to-staging

@cy-moi cy-moi force-pushed the congyao/RUM-5590-add-telemetry-for-INP-null branch from 17cff3c to 399fb3a Compare September 20, 2024 16:08
@cy-moi cy-moi force-pushed the congyao/RUM-5590-add-telemetry-for-INP-null branch from 8d7accc to 9ba6691 Compare September 23, 2024 12:39
Comment on lines 431 to 433
const setInteractionMapSpy = spyOn(Map.prototype, 'set').and.callThrough()
emulateClick({ activity: { on: 'pointerdown' } })
expect(setInteractionMapSpy).toHaveBeenCalled()
Copy link
Member

Choose a reason for hiding this comment

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

💬 suggestion: ‏instead of checking for set / get/delete calls on Map, why not check if the expected value is actually in interactionSelectorMap directly?

Suggested change
const setInteractionMapSpy = spyOn(Map.prototype, 'set').and.callThrough()
emulateClick({ activity: { on: 'pointerdown' } })
expect(setInteractionMapSpy).toHaveBeenCalled()
emulateClick({ activity: { on: 'pointerdown' }, timeStamp: xxx })
expect(interactionMap.get(xxx)).toBe(xxx)

I think it would be more explicit and a bit "safer" (we are sure the value was set on the right Map)

Copy link
Contributor Author

@cy-moi cy-moi Oct 7, 2024

Choose a reason for hiding this comment

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

Thank you! The tests should be fixed with checking the map directly. This way is indeed more effective! TIL

packages/rum-core/src/domain/action/trackClickActions.ts Outdated Show resolved Hide resolved
@cy-moi cy-moi force-pushed the congyao/RUM-5590-add-telemetry-for-INP-null branch from 702e976 to f029cd9 Compare October 7, 2024 13:29
@cy-moi cy-moi force-pushed the congyao/RUM-5590-add-telemetry-for-INP-null branch from bf259e0 to 6e862a6 Compare October 10, 2024 14:05
@cy-moi cy-moi merged commit 2b11968 into main Oct 10, 2024
19 checks passed
@cy-moi cy-moi deleted the congyao/RUM-5590-add-telemetry-for-INP-null branch October 10, 2024 15:36
@cy-moi cy-moi changed the title [RUM 5088] Reduce INP Null Target 👷‍♀️ [RUM 5088] Reduce INP Null Target Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants