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

feat: Centralize web vitals timings #635

Merged
merged 30 commits into from
Sep 5, 2023
Merged

feat: Centralize web vitals timings #635

merged 30 commits into from
Sep 5, 2023

Conversation

metal-messiah
Copy link
Member

@metal-messiah metal-messiah commented Aug 8, 2023

Centralize web vitals timings to be accessible from any part of the agent.

Overview

This PR utilizes a singleton export of the vitals timings with a pub/sub methodology to share identical data across the agent. This will create better consistency and replace areas of the agent leaning on differing methodologies.

Related Issue(s)

NR-101330

Testing

Existing timing tests should continue to pass.
New jest tests have been added for each vital file
New jest test has been made to replace some of the web-vitals browser tests that became broken by changing this behavior

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Asset Size Report

Merging this pull request will result in the following asset size changes:

Agent Asset Previous Size New Size Diff
lite loader 28.48 kB / 9.9 kB (gzip) 28.25 kB / 9.8 kB (gzip) -0.8% / -1.02% (gzip)
lite async-chunk 43.56 kB / 14.43 kB (gzip) 45.4 kB / 15.03 kB (gzip) 4.24% / 4.15% (gzip)
pro loader 45.67 kB / 15.35 kB (gzip) 45.35 kB / 15.2 kB (gzip) -0.7% / -0.93% (gzip)
pro async-chunk 62.72 kB / 20.21 kB (gzip) 64.45 kB / 20.82 kB (gzip) 2.76% / 3% (gzip)
spa loader 52.08 kB / 17.3 kB (gzip) 51.76 kB / 17.16 kB (gzip) -0.62% / -0.81% (gzip)
spa async-chunk 76.96 kB / 24.5 kB (gzip) 78.67 kB / 25.04 kB (gzip) 2.22% / 2.18% (gzip)
lite-polyfills loader 98.94 kB / 31.62 kB (gzip) 98.71 kB / 31.43 kB (gzip) -0.24% / -0.61% (gzip)
lite-polyfills async-chunk 54.61 kB / 16.41 kB (gzip) 57.86 kB / 17.24 kB (gzip) 5.95% / 5.09% (gzip)
pro-polyfills loader 118.53 kB / 37.51 kB (gzip) 118.3 kB / 37.37 kB (gzip) -0.2% / -0.37% (gzip)
pro-polyfills async-chunk 95.57 kB / 25.77 kB (gzip) 98.82 kB / 26.62 kB (gzip) 3.4% / 3.29% (gzip)
spa-polyfills loader 126.51 kB / 39.63 kB (gzip) 126.27 kB / 39.5 kB (gzip) -0.18% / -0.31% (gzip)
spa-polyfills async-chunk 110.52 kB / 30.36 kB (gzip) 113.7 kB / 31.03 kB (gzip) 2.88% / 2.22% (gzip)
worker loader 40.57 kB / 13.77 kB (gzip) 40.36 kB / 13.66 kB (gzip) -0.51% / -0.8% (gzip)
worker async-chunk 42.38 kB / 14.5 kB (gzip) 45.36 kB / 15.37 kB (gzip) 7.04% / 6.02% (gzip)

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@015e0f1). Click here to learn what that means.
The diff coverage is 97.41%.

❗ Current head d75bbfa differs from pull request most recent head 5d0b4c2. Consider uploading reports for the commit 5d0b4c2 to get more accurate results

@@           Coverage Diff           @@
##             main     #635   +/-   ##
=======================================
  Coverage        ?   86.76%           
=======================================
  Files           ?      137           
  Lines           ?     5245           
  Branches        ?      808           
=======================================
  Hits            ?     4551           
  Misses          ?      634           
  Partials        ?       60           
Flag Coverage Δ
unit-tests 79.20% <98.14%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/features/page_view_event/constants.js 100.00% <ø> (ø)
src/features/page_view_event/instrument/index.js 100.00% <ø> (ø)
src/features/page_view_timing/instrument/index.js 100.00% <ø> (ø)
src/features/spa/aggregate/index.js 78.44% <ø> (ø)
src/common/vitals/interaction-to-next-paint.js 71.42% <83.33%> (ø)
src/common/vitals/time-to-first-byte.js 80.00% <85.71%> (ø)
src/common/vitals/long-task.js 96.00% <96.00%> (ø)
src/common/constants/runtime.js 97.36% <100.00%> (ø)
src/common/vitals/constants.js 100.00% <100.00%> (ø)
src/common/vitals/cumulative-layout-shift.js 100.00% <100.00%> (ø)
... and 7 more

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

Copy link
Contributor

@patrickhousley patrickhousley left a comment

Choose a reason for hiding this comment

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

I think this is a lot cleaner and better structured from having an object that gets imported, wrote to, and read from willy nilly in the code base. I do wonder if we should not go ahead and move the other timings that are just in the page view timing aggregator: endCurrentSession and recordPageUnload.

@github-actions
Copy link

github-actions bot commented Aug 11, 2023

Static Badge

Last ran on August 28, 2023 20:27:04 CDT
Checking merge of (7b056dd) into main (d233e14)

@metal-messiah metal-messiah changed the title feat: Centralize vitals timings to be accessible by all parts of the agent feat: Centralize web vitals timings Aug 11, 2023
@metal-messiah metal-messiah marked this pull request as ready for review August 11, 2023 06:35
@patrickhousley
Copy link
Contributor

Looks good. Need jest tests for the new src/common/vitals files.

@metal-messiah metal-messiah added the large Large Engineering Effort label Aug 14, 2023
@metal-messiah
Copy link
Member Author

metal-messiah commented Aug 15, 2023

@metal-messiah metal-messiah merged commit d912e94 into main Sep 5, 2023
12 checks passed
@metal-messiah metal-messiah deleted the centralize-timings branch September 5, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Large Engineering Effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants