LoAF Summary attribution information #574

tunetheweb · 2024-12-06T21:19:07Z

Fixes #559

Includes object like this:

"longAnimationFrameSummary": {
    "numLongAnimationFrames": 3,
    "numIntersectingScripts": 9,
    "slowestScript": {
      "entry": // Not Shown,
      "subpart": "processingDuration",
      "intersectingDuration": 395,
      "totalDuration": 395,
      "compileDuration": 0,
      "executionDuration": 395,
      "forcedStyleAndLayoutDuration": 144,
      "pauseDuration": 0,
      "invokerType": "event-listener",
      "invoker": "BUTTON.onclick",
      "sourceURL": "",
      "sourceFunctionName": "",
      "sourceCharPosition": -1,
    },
    "totalDurationsPerSubpart": {
        "inputDelay": {
            "user-callback": 28,
            "event-listener": 185
        },
        "processingDuration": {
            "event-listener": 434
        }
    },
    "totalForcedStyleAndLayoutDuration": 144,
    "totalNonForcedStyleAndLayoutDuration": 3144099.700000003,
    "totalScriptDuration": 647
}

rviscomi

Thanks Barry! Left a few comments.

README.md

rviscomi · 2024-12-09T16:46:51Z

src/attribution/onINP.ts

+      }
+      totalScriptTime += script.duration;
+      numScripts++;
+      totalForcedStyleAndLayout += script.forcedStyleAndLayoutDuration;


I think there's an open question whether we should only be taking the blocking/intersecting portion of all duration metrics. WDYT?

Unfortunately we can't for forced style and layout since we don't have the timings LoAF. Maybe that's a good reason to keep this simple and just take it for all intersecting scripts (which is what it does currently)?

src/attribution/onINP.ts

README.md

brendankenny

incidentally, it seems like we're leaning subparts, not phases for LCP, so we might want to do the same thing here (or object quickly on the LCP side of things :)

src/types/inp.ts

src/attribution/onINP.ts

tunetheweb · 2024-12-10T13:31:59Z

incidentally, it seems like we're leaning subparts, not phases for LCP, so we might want to do the same thing here (or object quickly on the LCP side of things :)

I'm cool with subparts so switched to that throughout.

tunetheweb · 2024-12-10T16:59:39Z

@rviscomi I've flattened the structure, and updated the comment in #574 (comment) to show that.

rviscomi

LGTM! I'll try it out in the next RC.

philipwalton · 2025-01-15T05:51:41Z

README.md

+       * NOTE: This may be be less than the total count of scripts in the Long
+       * Animation Frames as some scripts may occur before the interaction.
+       */
+      numIntersectingScripts: number;


Rather that just listing the number of intersecting scripts, I wonder if it would be more useful to create an array of intersecting scripts (which then someone could easily call .length on).

Related, how common is it for scripts from multiple different LoAF entries to all be intersecting with INP?

philipwalton · 2025-01-15T05:53:27Z

README.md

+      /**
+       * The number of Long Animation Frames intersecting the INP interaction.
+       */
+      numLongAnimationFrames: number;


This seems unnecessary because someone could easily do longAnimationFrameEntries.length, right?

philipwalton · 2025-01-15T05:56:02Z

README.md

+          /**
+           * The INP sub-part where the longest script ran.
+           */
+          subpart: INPSubpart; //'inputDelay' | 'processingDuration' | 'presentationDelay';


On web.dev these are currently referred to as "phases". Are we planning to change that?

@brendankenny I think it was you that told me were were moving towards subparts to mirror LCP, is that right?

Yes, we should definitely unify. I suggested it specifically because we didn't want to simultaneously cement "phases" here in web-vitals and "subparts" over in CrUX, and everyone in the LCP discussions agreed on "subpart".

philipwalton · 2025-01-15T05:57:34Z

README.md

+          /**
+           * The INP sub-part where the longest script ran.
+           */
+          subpart: INPSubpart; //'inputDelay' | 'processingDuration' | 'presentationDelay';


Suggested change

subpart: INPSubpart; //'inputDelay' | 'processingDuration' | 'presentationDelay';

subpart: INPSubpart; // 'input-delay' | 'processing-duration' | 'presentation-delay'

For consistency with other enum-type options.

This is interesting. Yes as an enum they should use that format. However this is then exposed in output—as both property names and values:

"slowestScript": { ... "subpart": "processingDuration", "intersectingDuration": 100, ... }, "totalDurationsPerSubpart": { "processingDuration": { "event-listener": 100 } },

And so they should really follow camelCase? Though the invoker type is hyphenated (and that's part of the spec) so it's a little inconsistent anyway in that regards

The other consideration is that camelCase is also consistent with what we use for LCP sub parts, but there they are strict property names (not values) and we don't have them defined as an enum.

philipwalton · 2025-01-15T06:04:00Z

README.md

+       * The slowest Long Animation Frame script that intersects the INP
+       * interaction.
+       */
+      slowestScript: SlowestScriptSummary;


Right now the SlowestScriptSummary object duplicates many of the native PerformanceScriptTiming properties, and it's not clear to me how valuable that is. AFAICT the only "computed" values are subpart and intersectingDuration.

What if instead we just had three top-level attribution properties?

slowestScriptEntry

slowestScriptPhase (or subpart)

slowestScriptIntersectingDuration

src/attribution/onINP.ts

philipwalton · 2025-01-15T06:20:04Z

src/types/inp.ts

@@ -24,6 +24,140 @@ export interface INPMetric extends Metric {
  entries: PerformanceEventTiming[];
 }

+export type INPSubpart =
+  | 'inputDelay'


Suggested change

| 'inputDelay'

| 'input-delay'

philipwalton · 2025-01-15T06:20:13Z

src/types/inp.ts

@@ -24,6 +24,140 @@ export interface INPMetric extends Metric {
  entries: PerformanceEventTiming[];
 }

+export type INPSubpart =
+  | 'inputDelay'
+  | 'processingDuration'


Suggested change

| 'processingDuration'

| 'processing-duration'

philipwalton · 2025-01-15T06:20:22Z

src/types/inp.ts

+export type INPSubpart =
+  | 'inputDelay'
+  | 'processingDuration'
+  | 'presentationDelay';


Suggested change

| 'presentationDelay';

| 'presentation-delay';

philipwalton · 2025-01-15T06:20:44Z

src/types/inp.ts

+  /**
+   * The INP sub-part where the longest script ran.
+   */
+  subpart: INPSubpart; //'inputDelay' | 'processingDuration' | 'presentationDelay';


Suggested change

subpart: INPSubpart; //'inputDelay' | 'processingDuration' | 'presentationDelay';

subpart: INPSubpart; // 'input-delay' | 'processing-duration' | 'presentation-delay'

philipwalton · 2025-01-15T18:34:13Z

README.md

+       * The slowest Long Animation Frame script that intersects the INP
+       * interaction.
+       */
+      slowestScript: SlowestScriptSummary;


Also, I wonder if longestScript would be better and more consistent with other API names (e.g. Long Tasks and Long Animation Frames).

test/e2e/onINP-test.js

src/attribution/onINP.ts

philipwalton · 2025-01-16T05:40:11Z

src/attribution/onINP.ts

+        subpart: slowestScriptSubpart,
+        intersectingDuration: slowestScriptDuration,
+        totalDuration: slowestScriptEntry?.duration,
+        compileDuration:


Using the ?. optional chaining operator here seems risky because if slowestScriptEntry is undefined then you'll end up doing math operations on undefined, which will result in NaN.

I know there's been some discussion of removing these properties anyway, but if we don't then it's probably better to just omit these (or set them to 0) if there's no slowest script.

Co-authored-by: Philip Walton <[email protected]>

LoAF Summary attribution information

f195e55

tunetheweb mentioned this pull request Dec 6, 2024

Add more LoAF diagnostics to the INP attribution object #559

Open

21 tasks

tunetheweb requested review from philipwalton and rviscomi December 6, 2024 21:21

tunetheweb added 6 commits December 7, 2024 14:08

Add tests

0cb43d6

Fix tests

95237c6

Typo

194cc08

Fixed the wrong one!

1b49c03

Debug test

d8cb2a1

Remove failing test

0b6b12f

rviscomi reviewed Dec 9, 2024

View reviewed changes

tunetheweb added 5 commits December 9, 2024 17:48

Review feedback

aad54d8

Typo

0cdc93f

Fix ScriptInvokerType

918e068

Comments and names

4b16fac

Fix test

add385a

brendankenny reviewed Dec 10, 2024

View reviewed changes

tunetheweb added 2 commits December 10, 2024 12:43

Merge branch 'v5' into load-attribution

320eaa6

Review feedback

94abf79

tunetheweb added 3 commits December 10, 2024 16:40

Document updates

dcd95df

Sync README

9513369

Flatten SlowestScriptSummary into LongAnimationFrameSummary

e0c2f08

Types

80d272a

rviscomi approved these changes Dec 11, 2024

View reviewed changes

Move slowestScript back under it's own object

807c85b

philipwalton requested changes Jan 15, 2025

View reviewed changes

tunetheweb added 2 commits January 15, 2025 16:19

Merge branch 'v5' into load-attribution

7f4c7d9

Review feedback

f8fa543

More feedback

9c61110

philipwalton reviewed Jan 15, 2025

View reviewed changes

test/e2e/onINP-test.js Outdated Show resolved Hide resolved

philipwalton reviewed Jan 15, 2025

View reviewed changes

src/attribution/onINP.ts Outdated Show resolved Hide resolved

philipwalton reviewed Jan 16, 2025

View reviewed changes

tunetheweb and others added 3 commits January 20, 2025 16:07

Update test/e2e/onINP-test.js

19a2f42

Co-authored-by: Philip Walton <[email protected]>

Update src/attribution/onINP.ts

934b803

Co-authored-by: Philip Walton <[email protected]>

Linting

3fa8ae0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LoAF Summary attribution information #574

LoAF Summary attribution information #574

tunetheweb commented Dec 6, 2024 •

edited

Loading

rviscomi left a comment

rviscomi Dec 9, 2024

tunetheweb Dec 9, 2024

brendankenny left a comment

tunetheweb commented Dec 10, 2024

tunetheweb commented Dec 10, 2024

rviscomi left a comment

philipwalton Jan 15, 2025

philipwalton Jan 15, 2025

philipwalton Jan 15, 2025

tunetheweb Jan 15, 2025

brendankenny Jan 15, 2025

philipwalton Jan 15, 2025

tunetheweb Jan 15, 2025

philipwalton Jan 15, 2025

philipwalton Jan 15, 2025

philipwalton Jan 15, 2025

philipwalton Jan 15, 2025

philipwalton Jan 15, 2025

philipwalton Jan 15, 2025

philipwalton Jan 16, 2025

	subpart: INPSubpart; //'inputDelay' \| 'processingDuration' \| 'presentationDelay';
	subpart: INPSubpart; // 'input-delay' \| 'processing-duration' \| 'presentation-delay'

LoAF Summary attribution information #574

Are you sure you want to change the base?

LoAF Summary attribution information #574

Conversation

tunetheweb commented Dec 6, 2024 • edited Loading

rviscomi left a comment

Choose a reason for hiding this comment

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

tunetheweb commented Dec 10, 2024

tunetheweb commented Dec 10, 2024

rviscomi 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

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

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

tunetheweb commented Dec 6, 2024 •

edited

Loading