core(cumulative-layout-shift): experiment with new shared trace engine#15702
core(cumulative-layout-shift): experiment with new shared trace engine#15702connorjclark merged 9 commits intomainfrom
Conversation
3faf419 to
6a4f4ba
Compare
How often does this happen with the 500ms limit these days? Which branch are the smoke tests hitting? #15703 makes sense, but this PR still feels like a strange approach to incorporating the DT trace engine. Putting it behind a double RNG (page content * load conditions) makes assessing any issues more difficult (which engine generated the result?). And without the upstream changes, it's exchanging 20 lines of code with 10K + the same 20 lines of code. |
|
In response to Brendan's comment, this PR now (pasting from first post EDIT):
We can consider switching over when we've upstream the thing re: user input edge cases, and gotten some data from Sentry about unexpected differences (if any). |
brendankenny
left a comment
There was a problem hiding this comment.
Thanks @connorjclark!
| const differ = | ||
| newEngineResult.cumulativeLayoutShift !== cumulativeLayoutShift || | ||
| newEngineResult.cumulativeLayoutShiftMainFrame !== cumulativeLayoutShiftMainFrame; | ||
| if (differ) { |
There was a problem hiding this comment.
Could stick it in debugData as well if it would be interesting to compare in HTTP Archive to get ballpark numbers ("out of 7M or whatever runs where both approaches ran, there were no differences"). That would require passing it back in the computed artifact, but having something in the return value would also be an avenue for a test asserting that this path is running.
There was a problem hiding this comment.
Good idea. Can I leave it like this or should I flatten it out?
There was a problem hiding this comment.
also added a newEngineResultDiffered boolean to help with querying
There was a problem hiding this comment.
leave it like this or should I flatten it out
probably like this is more ergonomic in this file and it doesn't really matter in HTTP Archive since it's querying into JSON either way
|
|
||
| // Run with the new trace engine, and only throw an error if we are running our unit tests. | ||
| // Otherwise, simply report any differences or errors to Sentry. | ||
| // TODO: TraceEngine always drops `had_recent_input` events, but Lighthouse is more lenient. |
There was a problem hiding this comment.
I always get mixed up on this so just correct me if I'm wrong, but isn't it less lenient?
Some events may have had_recent_input: true so wouldn't be counted by Chrome/DT toward the CLS total, but Lighthouse includes those events (potentially making CLS worse) because it wasn't really input, it was the emulation viewport change?
edit: I guess unless it's saying it's more lenient from the perspective of the events getting filtered out, haha
There was a problem hiding this comment.
yeah, the filter itself is more lenient in what it allows through. It's a lot of summarize so I just have the comment defer to "the comment in getLayoutShiftEvents"
| "webtreemap-cdt": "^3.2.1" | ||
| }, | ||
| "dependencies": { | ||
| "@paulirish/trace_engine": "^0.0.7", |
There was a problem hiding this comment.
Doesn't really matter regardless so just out of curiosity, what's the bundle size impact? 😬
There was a problem hiding this comment.
@paulirish can this be revived? https://lh-build-tracker.herokuapp.com/
the devtools bundle: 1948329 -> 2069598 (+118KB), pre-compression
This pulls in the new shared trace engine via
@paulirish/trace_engine, and uses it to calculate CLS. If it fails, it falls back to our current approach.Also falls back to current approach if any trace has recent input, which we handle differently and need to upstream to the shared trace engine.
EDIT:
This PR now runs the new shared trace engine to try it out, but still determines CLS based off our existing implementation. If the new shared trace engine differs, we'll get reports in Sentry.