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

new_audit: add work-during-interaction diagnostic #13982

Merged
merged 2 commits into from
May 10, 2022
Merged

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented May 10, 2022

part of #13916

Breaks down the tasks occurring during the INP event. Tests are very light, still working on them

Example report: https://lighthouse-ddszazj4j-googlechrome.vercel.app/gh-pages/viewer/?gist=0b9f538660fe83c57c94acb5b6a04be0

Example lighthouse post showing new diagnostic audit

@brendankenny brendankenny requested a review from a team as a code owner May 10, 2022 01:05
@brendankenny brendankenny requested review from adamraine and removed request for a team May 10, 2022 01:05
const displayValue = str_(UIStrings.displayValue, {timeInMs: duration, interactionType});

return {
score: duration < MAX_DURATION_THRESHOLD ? 1 : 0,
Copy link
Member

Choose a reason for hiding this comment

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

This pass/fail condition is directly tied to the actual INP value. It seems like this audit is similar to layout-shift-elements because it provides a detailed breakdown of the metric value. WDYT about making this an informative audit just like layout-shift-elements?

Copy link
Member Author

@brendankenny brendankenny May 10, 2022

Choose a reason for hiding this comment

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

I forget the argument now for layout-shift-elements, but it would seem strange to me not to put a pass/fail on this audit. If it's a slow interaction we should highlight it in red as an important diagnostic to focus on. Happy to discuss this more, though.

However it doesn't make sense to use an arbitrary threshold, I had it in there for testing and forgot about it. I updated to pull the p10 from the INP audit.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but would like to discuss after 9.6

let totalExecutionTimeForURL = 0;
for (const [groupId, timespanMs] of Object.entries(timingByGroupId)) {
if (timespanMs === 0) continue;
timingByGroupId[groupId] = timespanMs;
Copy link
Member

Choose a reason for hiding this comment

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

What is this line doing? Seems like it will never change timingByGroupId

Copy link
Member Author

Choose a reason for hiding this comment

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

What is this line doing? Seems like it will never change timingByGroupId

Good catch, it's doing nothing :)

Originally it was a CPU multiplier line (brought over from BootupTime), but since this doesn't support simulated throttling, it became useless.

const displayValue = str_(UIStrings.displayValue, {timeInMs: duration, interactionType});

return {
score: duration < MAX_DURATION_THRESHOLD ? 1 : 0,
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but would like to discuss after 9.6

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