core(third-party-summary): add blocking time impact #9486

patrickhulce · 2019-07-30T21:22:36Z

Summary
After some discussion, we settled on surfacing the blocking time of third parties rather than a full-fledged opportunity of TTI savings of removing all third-party resources. It shows up as failing if the impact to TBT is >250ms (which would push you out of a ~90 score range on TBT).

Related Issues/PRs
closes #9256

benschwarz · 2019-07-30T21:34:43Z

I know that Total Blocking Time is the newly coined name for main thread long tasks, but I feel like it's a little confusing in this context without a more descriptive label referencing the "main thread" more directly.

Specifically, I'm referring to the table heading "Blocking Time". It could easily be confused as the time that a given vendor delayed http requests.

Could the label be: Main thread blocking time?

lighthouse-core/audits/third-party-summary.js

paulirish · 2019-08-14T00:19:14Z

lighthouse-core/test/audits/third-party-summary-test.js

@@ -31,6 +32,7 @@ describe('Third party summary', () => {
          url: 'https://marketingplatform.google.com/about/tag-manager/',
        },
        mainThreadTime: 104.70300000000002,
+        blockingTime: 18.186999999999998,


just interesting to see the impact here drop (104ms -> 18ms)

yeah I'm a bit disappointed to see the very busy third parties disappear, but if it's an incentive to script responsibly than I guess it's still a very positive outcome so I'm :)

My biggest concern is that no one is going to understand what "main-thread blocking time" actually is and they'll assume it's the time spent which would be sad.

FWIW this is also not as drastic in the lantern case, it goes from 419 -> 250. While still a big drop, the number is still noticeable.

brendankenny

My biggest concern is that no one is going to understand what "main-thread blocking time" actually is and they'll assume it's the time spent which would be sad.

this concerns me, too, but I guess we'll see how it goes :)

Other than that, though, LGTM! Only superficial comments.

lighthouse-core/audits/third-party-summary.js

brendankenny · 2019-08-14T20:49:22Z

lighthouse-core/audits/third-party-summary.js

  /** Description of a Lighthouse audit that identifies the code on the page that the user doesn't control. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
  description: 'Third-party code can significantly impact load performance. ' +
    'Limit the number of redundant third-party providers and try to load third-party code after ' +
    'your page has primarily finished loading. [Learn more](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/loading-third-party-javascript/).',
  /** Label for a table column that displays the name of a third-party provider that potentially links to their website. */
  columnThirdParty: 'Third-Party',
-  /** Label for a table column that displays how much time each row spent executing on the main thread, entries will be the number of milliseconds spent. */
-  columnMainThreadTime: 'Main Thread Time',
+  /** Label for a table column that displays how much time each row spent blocking other work on the main thread, entries will be the number of milliseconds spent. */


I think there might have been an issue with describing the "main thread" to the tc... @exterkamp? Or was it for this audit and it's already been clarified :)

The problem was with "thread" being used, in regards to the description in first-interactive

I think this is fine here. Maybe add something about threads. But I think that is unnecessary. This LGTM.

lighthouse-core/audits/third-party-summary.js

brendankenny · 2019-08-14T20:53:13Z

lighthouse-core/audits/third-party-summary.js

 };

 const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

+// A page passes when all third-party code blocks for less than 250 ms.


maybe put some version of the comment in the PR description

It shows up as failing if the impact to TBT is >250ms (which would push you out of a ~90 score range on TBT)

in here to give motivation to the particular number?

yeah sg

Suggested change

// A page passes when all third-party code blocks for less than 250 ms.

// A page fails this audit if the main-thread impact is >250ms (which would push you out of a ~90 score range on TBT).

brendankenny · 2019-08-14T20:54:09Z

lighthouse-core/audits/third-party-summary.js

+      // The amount of time spent on main thread is the sum of all durations.
+      entityStats.mainThreadTime += taskDuration;
+      // The amount of time spent *blocking* on main thread is the sum of all time longer than 50ms.
+      // Note that this is not totally equivalent to the TBT definition since it fails to account for FCP,


is this a TODO or worth reexamining at some point?

IMO not a TODO ever worth fixing since it should be rare that third party code actually blocks FCP and if it does I'm fine pointing the finger at it anyway. Our slight decoupling of the audit from TBT in external descriptions also makes this less of an issue.

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

lighthouse-core/audits/third-party-summary.js

exterkamp · 2019-08-14T23:05:33Z

lighthouse-core/audits/third-party-summary.js

-  }`,
+    =1 {1 third-party}
+    other {# third-parties}
+    } blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`,


Plurals must be totally contained in the {}. #9460 allows replacements in plurals like this. But I don't want to add more strings that violate this policy (I get a lot of emails saying this is against policy).

Suggested change

} blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`,

`{itemCount, plural,

=1 {1 third-party blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`}

=other {# third-parties blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`}

}`,

I would also expand on "third-party" with something like 1 third-party script blocked...

I totally forgot about this whole fiasco, honestly I don't think the number of third parties needs to be communicated here if it causes trouble

codecov · 2019-08-15T19:04:37Z

Codecov Report

Merging #9486 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9486      +/-   ##
==========================================
- Coverage   91.88%   91.87%   -0.01%     
==========================================
  Files         298      298              
  Lines       10361    10364       +3     
==========================================
+ Hits         9520     9522       +2     
- Misses        841      842       +1

Flag	Coverage Δ
#smoke	`84.4% <22.22%> (-0.05%)`	⬇️
#unit	`90.27% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
lighthouse-core/audits/third-party-summary.js	`100% <100%> (ø)`	⬆️
...house-core/computed/metrics/lantern-speed-index.js	`97.14% <0%> (-2.86%)`	⬇️

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 5eb01d6...676b9b0. Read the comment docs.

core(third-party-summary): add blocking time impact

d70939c

patrickhulce requested review from brendankenny, exterkamp and paulirish as code owners July 30, 2019 21:22

googlebot added the cla: yes label Jul 30, 2019

paulirish approved these changes Aug 14, 2019

View reviewed changes

feedbak

e434fd4

vercel bot deployed to staging August 14, 2019 15:22 View deployment

patrickhulce added 2 commits August 14, 2019 10:25

Merge branch 'master' into 3p_tbt_impact

bbd5328

Merge branch 'master' into 3p_tbt_impact

f46e441

vercel bot deployed to staging August 14, 2019 15:26 View deployment

brendankenny approved these changes Aug 14, 2019

View reviewed changes

Apply suggestions from code review

9db995b

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

vercel bot deployed to staging August 14, 2019 21:33 View deployment

display string update

782ad13

vercel bot deployed to staging August 14, 2019 21:35 View deployment

exterkamp reviewed Aug 14, 2019

View reviewed changes

shane feedback

d55a822

vercel bot deployed to staging August 15, 2019 01:22 View deployment

sample fixtures

676b9b0

vercel bot deployed to staging August 15, 2019 19:04 View deployment

tests

9b36046

vercel bot deployed to staging August 15, 2019 21:29 View deployment

patrickhulce merged commit 48be37a into master Aug 16, 2019

patrickhulce deleted the 3p_tbt_impact branch August 16, 2019 15:20

brendankenny mentioned this pull request Aug 20, 2019

core(i18n): throw on excess placeholder replacement values #9580

Merged

paulirish pushed a commit that referenced this pull request Nov 6, 2019

core(third-party-summary): add blocking time impact (#9486)

a50ac37

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(third-party-summary): add blocking time impact #9486

core(third-party-summary): add blocking time impact #9486

patrickhulce commented Jul 30, 2019

benschwarz commented Jul 30, 2019

paulirish Aug 14, 2019

patrickhulce Aug 14, 2019

brendankenny left a comment

brendankenny Aug 14, 2019

exterkamp Aug 14, 2019

brendankenny Aug 14, 2019

patrickhulce Aug 14, 2019 •

edited

Loading

brendankenny Aug 14, 2019

patrickhulce Aug 14, 2019

exterkamp Aug 14, 2019

exterkamp Aug 14, 2019

patrickhulce Aug 15, 2019

codecov bot commented Aug 15, 2019 •

edited

Loading

	// A page passes when all third-party code blocks for less than 250 ms.
	// A page fails this audit if the main-thread impact is >250ms (which would push you out of a ~90 score range on TBT).

-    } blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`,
+`{itemCount, plural,
+   =1 {1 third-party blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`}
+   =other {# third-parties blocked the main thread by {timeInMs, number, milliseconds}\xa0ms`}
+  }`,

core(third-party-summary): add blocking time impact #9486

core(third-party-summary): add blocking time impact #9486

Conversation

patrickhulce commented Jul 30, 2019

benschwarz commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2019 • edited Loading

Codecov Report

patrickhulce Aug 14, 2019 •

edited

Loading

codecov bot commented Aug 15, 2019 •

edited

Loading