-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ci: Add new metrics overhead app #7300
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
Conversation
To hopefully be able to debug replay issues a bit better, and have a bit more representative checking.
99b3687
to
c800954
Compare
Replay SDK metrics 🚀
develop |
Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
---|---|---|---|---|---|---|---|---|---|
dbd7a81 | +86.19 ms | -0.00 ms | +6.77 pp | +1.58 MB | +1.67 MB | +2.23 kB | +41 B | +1 | +87.99 ms |
67b0684 | +84.74 ms | -0.00 ms | +7.38 pp | +1.7 MB | +1.81 MB | +2.23 kB | +41 B | +1 | +85.52 ms |
4b95c04 | +57.56 ms | -0.00 ms | +7.94 pp | +920.88 kB | +1.05 MB | +2.21 kB | +41 B | +1 | +90.32 ms |
e60cd02 | +56.25 ms | -0.00 ms | +6.32 pp | +927.44 kB | +1.06 MB | +2.21 kB | +41 B | +1 | +117.55 ms |
e25c067 | +48.34 ms | +0.00 ms | +5.59 pp | +926.37 kB | +1.05 MB | +2.22 kB | +41 B | +1 | +65.23 ms |
b1b249b | +43.88 ms | +0.00 ms | +4.80 pp | +937.99 kB | +1.05 MB | +2.22 kB | +41 B | +1 | +111.56 ms |
12e34d4 | +28.57 ms | +0.00 ms | +5.77 pp | +930.12 kB | +1.04 MB | +2.26 kB | +41 B | +1 | +109.67 ms |
c46c56c | +65.45 ms | -0.00 ms | +5.38 pp | +930.26 kB | +1.07 MB | +2.21 kB | +41 B | +1 | +91.29 ms |
7f4c4ec | +56.64 ms | -0.00 ms | +5.57 pp | +927.42 kB | +1.06 MB | +2.21 kB | +41 B | +1 | +110.83 ms |
Previous results on branch: fn/better-metrics-app
fn/better-metrics-app
Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
---|---|---|---|---|---|---|---|---|---|
541c510 | -14.41 ms | +0.25 ms | +18.48 pp | +6.95 MB | +9.86 MB | +103.42 kB | -1.29 kB | +3 | +66.84 ms |
Last updated: Wed, 01 Mar 2023 09:30:36 GMT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just had a couple of questions
// Wait for flushing, which we set to 2000ms | ||
await new Promise(resolve => setTimeout(resolve, 2000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wait a little longer than 2000ms considering the timing inconsistencies we observed in #7266?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, true I guess! I'll extend it to 4000ms to be safe.
} else { | ||
window.cumulativeLayoutShiftScore += entry.value; | ||
} | ||
if (window.cumulativeLayoutShiftScore === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: That's the reason for removing this guard here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I missed that you just rewrote this.
I added a I also inlined the styles into the html page, as otherwise when debugging replays the styles will not be correct etc. |
To hopefully be able to debug replay issues a bit better, and have a bit more representative checking.
This is a simple plain JS test app that looks something like this:
I tried to incorporate some not so efficient stuff to be able to test stuff.
It seems to be working decently even with replay enabled. I built it in a way that you can pass
?count=xxx
in order to test it with varying amounts of DOM elements.