core(cumulative-layout-shift): introduce CDT's trace processor for CLS #15225
Conversation
| }); | ||
| // We really want the logic that getLayoutShiftEvents provides for filtering out | ||
| // some events based on recent input. | ||
| // Would it make sense to upstream this to CDT? |
There was a problem hiding this comment.
I would say yes. If we are going to unify our trace processors then we should avoid modifying the results.
There was a problem hiding this comment.
@jackfranklin does filtering out user-initiated layout shifts from perf panel sound good?
(we don't need to block this initial integration on this)
There was a problem hiding this comment.
In the perf panel we have historically shown shifts even if they were user initiated, but no reason that the handler couldn't expose the set of user initiated shifts and set of non-user initiated shifts, or something like that, so that any consumer can use whichever set they want.
| @@ -113,6 +115,122 @@ const modifications = [ | |||
| 'Platform', | |||
| ], | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
All this complexity melts away once we validate this shared trace processor approach, and extract to a shared library.
There was a problem hiding this comment.
Is this the optimal order of operations? I haven't seen the design discussions so far, but this requires a bunch of generation/generated code to do a task we already know both code bases can do (CLS is straightforward and they both have extensive CLS tests).
The shared library is what historically no one had a unified vision of and is going to require work that will need to stick around (e.g. making code living in DevTools importable). Does it make more sense to start there so the infrastructure won't have to melt away, it won't be needed from the start?
| cumulativeLayoutShift = data.LayoutShifts.sessionMaxScore; | ||
| } catch (e) { | ||
| // Something failed, so fallback to our own implementation. | ||
| log.error('Error running SDK.TraceProcessor', e); |
There was a problem hiding this comment.
I think this deserves to be captured in sentry. If there are situations where the DT trace processor breaks then we should investigate and fix them.
| @@ -220,3 +338,9 @@ function writeGeneratedFile(outputPath, contents) { | |||
| } | |||
|
|
|||
| modifications.forEach(doModification); | |||
| writeGeneratedFile(`${outDir}/models/trace/handlers/handlers.js`, ` | |||
There was a problem hiding this comment.
Was it just easier to rewrite this file? Can you add a comment about this?
is there a doc for this? |
I will ping you on corp with some info, but there is not a definitive plan yet as really the idea with this PR was to validate the approach and that the architecture of this engine is suitable. I think the next step is to chat about what we really mean by "shared library" and think through how it would work in practice. |
|
We're gonna import this from npm now, so closing! |
cdt-build-lib.js