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

✨ Collect long animation frames as long task events #3272

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

thomas-lebeau
Copy link
Collaborator

@thomas-lebeau thomas-lebeau commented Jan 9, 2025

Motivation

GA Collecting long animation frames

Changes

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@thomas-lebeau thomas-lebeau force-pushed the thomas.lebeau/loaf-remove-ff branch from acdc77d to c3dbf48 Compare January 9, 2025 10:58
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2025

Codecov Report

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

Project coverage is 93.53%. Comparing base (7512878) to head (923877d).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
packages/rum-core/src/boot/startRum.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3272      +/-   ##
==========================================
+ Coverage   93.51%   93.53%   +0.02%     
==========================================
  Files         288      288              
  Lines        7599     7598       -1     
  Branches     1730     1730              
==========================================
+ Hits         7106     7107       +1     
+ Misses        493      491       -2     

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

Copy link

cit-pr-commenter bot commented Jan 10, 2025

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 144.54 KiB 144.56 KiB 28 B +0.02%
Logs 51.13 KiB 51.09 KiB -46 B -0.09%
Rum Slim 103.39 KiB 103.42 KiB 36 B +0.03%
Worker 24.50 KiB 24.50 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base Average Cpu Time (ms) Local Average Cpu Time (ms) 𝚫
addglobalcontext 0.002 0.002 0.000
addaction 0.040 0.041 0.002
addtiming 0.001 0.001 0.000
adderror 0.046 0.067 0.021
startstopsessionreplayrecording 1.181 0.010 -1.171
startview 0.374 0.427 0.053
logmessage 0.034 0.023 -0.011
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 7.80 KiB 29.45 KiB 21.65 KiB
addaction 40.19 KiB 60.13 KiB 19.95 KiB
addtiming 6.58 KiB 26.79 KiB 20.21 KiB
adderror 45.16 KiB 62.23 KiB 17.08 KiB
startstopsessionreplayrecording 6.69 KiB 25.10 KiB 18.42 KiB
startview 420.68 KiB 414.89 KiB -5922 B
logmessage 42.37 KiB 60.44 KiB 18.07 KiB

🔗 RealWorld

Comment on lines 73 to 76
const filterdLogs = logs.filter(
// Ignore long-animation-frame warning (only happens on Edge)
(log) => !log.message.includes("The entry type 'long-animation-frame' does not exist or isn't supported")
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A bunch of e2e are verifying the number browser logs (example)

Another solution would be to makes these test for the console log they are actually expecting. But at the same time it is nice to ensure there is no unexpected console logs.

Copy link
Member

@BenoitZugmeyer BenoitZugmeyer Jan 10, 2025

Choose a reason for hiding this comment

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

💬 suggestion:
We should probably adjust performanceObservable.ts to skip observing unsupported entries to avoid spamming the console on customer websites. Something like:

if (!supportPerformanceTimingEvent(options.type)) {
  return
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Performance.supportedEntryTypes is not supported everywhere. For example,
Long Task are supported since chrome 58, but Performance.supportedEntryTypes only since Chrome 73, while we supposedly support chrome 63+

Copy link
Member

Choose a reason for hiding this comment

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

Well still, I think we should find a way to avoid displaying the warning in browsers without loaf support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, as finally we're falling back to Long tasks when LoAF is not supported, I'm starting either Loaf collection or Long task collection in startRum depending on Performance.supportedEntryTypes.

It should work to get rid of the warning because everywhere LoAF are supported Performance.supportedEntryTypes is also supported.

@thomas-lebeau thomas-lebeau marked this pull request as ready for review January 10, 2025 13:35
@thomas-lebeau thomas-lebeau requested a review from a team as a code owner January 10, 2025 13:35
@thomas-lebeau thomas-lebeau force-pushed the thomas.lebeau/loaf-remove-ff branch from b7171fe to 923877d Compare January 13, 2025 08:44
@thomas-lebeau thomas-lebeau merged commit dfc7ba1 into main Jan 13, 2025
19 checks passed
@thomas-lebeau thomas-lebeau deleted the thomas.lebeau/loaf-remove-ff branch January 13, 2025 11:25
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.

3 participants