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

✨ [RUMF-775] implement Largest Contentful Paint #624

Merged
merged 5 commits into from
Nov 20, 2020

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

Expose the web-vitals LCP on initial views

Changes

  • Implement LCP tracking
  • Use it on initial views

Testing

CI + manual tests on chrome


I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner November 19, 2020 10:09
@codecov-io
Copy link

Codecov Report

Merging #624 (b43b54b) into master (de06363) will increase coverage by 0.02%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
+ Coverage   88.23%   88.25%   +0.02%     
==========================================
  Files          56       56              
  Lines        2525     2538      +13     
  Branches      546      547       +1     
==========================================
+ Hits         2228     2240      +12     
- Misses        297      298       +1     
Impacted Files Coverage Δ
packages/rum/src/browser/performanceCollection.ts 22.72% <0.00%> (-0.35%) ⬇️
.../domain/rumEventsCollection/view/viewCollection.ts 100.00% <ø> (ø)
packages/rum/src/typesV2.ts 100.00% <ø> (ø)
packages/core/src/tools/utils.ts 86.74% <100.00%> (+0.08%) ⬆️
...rc/domain/rumEventsCollection/view/trackTimings.ts 100.00% <100.00%> (ø)
packages/rum/src/transport/batch.ts 78.04% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de06363...b43b54b. Read the comment docs.

) {
const firstHidden = trackFirstHidden()

// Ignore entries that come after the first user interaction
Copy link
Contributor

Choose a reason for hiding this comment

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

it could be helpful to add a link to the spec of these new timings or maybe describe the different rules in a function comment.

Copy link
Member Author

@BenoitZugmeyer BenoitZugmeyer Nov 19, 2020

Choose a reason for hiding this comment

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

I can't find a link to show that it is needed. All the documentation (here, spec draft readme, spec draft ) seems to agree that the browser won't send those performance entries after the first user interaction. I think that's just a safeguard that web-vitals implemented because of some Chrome issues.

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