[RPP Observations] Display field p75 prominently and label histogram [devtools/devtools-frontend : main]

0 views
Skip to first unread message

Paul Irish (Gerrit)

unread,
Jul 2, 2024, 4:20:59 PMJul 2
to Adam Raine, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Adam Raine

Paul Irish voted and added 1 comment

Votes added by Paul Irish

Code-Review+1

1 comment

File front_end/panels/timeline/components/LiveMetricsView.ts
Line 189, Patchset 8 (Latest): <span class="histogram-label">Ok <span class="histogram-range">(${format(thresholds[0])}-${
Paul Irish . unresolved

I know the long string sucks but this'll have to be `Needs Improvement`. We deliberately didn't want to signal that this was "ok" :)

Open in Gerrit

Related details

Attention is currently required from:
  • Adam Raine
Submit Requirements:
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I8f6ac410570aec9113e1555d74214d693c9b8184
Gerrit-Change-Number: 5665090
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Paul Irish <paul...@chromium.org>
Gerrit-CC: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Attention: Adam Raine <asr...@chromium.org>
Gerrit-Comment-Date: Tue, 02 Jul 2024 20:20:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Adam Raine (Gerrit)

unread,
Jul 2, 2024, 4:40:31 PMJul 2
to Paul Irish, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Adam Raine voted and added 1 comment

Votes added by Adam Raine

Commit-Queue+2

1 comment

File front_end/panels/timeline/components/LiveMetricsView.ts
Line 189, Patchset 8: <span class="histogram-label">Ok <span class="histogram-range">(${format(thresholds[0])}-${
Paul Irish . resolved

I know the long string sucks but this'll have to be `Needs Improvement`. We deliberately didn't want to signal that this was "ok" :)

Adam Raine

Done

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I8f6ac410570aec9113e1555d74214d693c9b8184
Gerrit-Change-Number: 5665090
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Paul Irish <paul...@chromium.org>
Gerrit-CC: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 20:40:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Paul Irish <paul...@chromium.org>
satisfied_requirement
open
diffy

Adam Raine (Gerrit)

unread,
Jul 2, 2024, 5:01:26 PMJul 2
to Paul Irish, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Adam Raine voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I8f6ac410570aec9113e1555d74214d693c9b8184
Gerrit-Change-Number: 5665090
Gerrit-PatchSet: 10
Gerrit-Owner: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Paul Irish <paul...@chromium.org>
Gerrit-CC: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 21:01:23 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Devtools-frontend LUCI CQ (Gerrit)

unread,
Jul 2, 2024, 5:31:39 PMJul 2
to Adam Raine, Paul Irish, devtools-rev...@chromium.org

Devtools-frontend LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

8 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: front_end/panels/timeline/components/LiveMetricsView.test.ts
Insertions: 12, Deletions: 6.

@@ -221,10 +221,12 @@
       await coordinator.done();
 
       const lcpHistogramEl = view.shadowRoot?.querySelector('#lcp .field-data-histogram') as HTMLDivElement;
-      assert.strictEqual(lcpHistogramEl.innerText, 'Good (≤2.50 s)\n50%\nOk (2.50 s-4.00 s)\n30%\nPoor (>4.00 s)\n20%');
+      assert.strictEqual(
+          lcpHistogramEl.innerText, 'Good (≤2.50 s)\n50%\nNeeds improvement (2.50 s-4.00 s)\n30%\nPoor (>4.00 s)\n20%');
 
       const clsHistogramEl = view.shadowRoot?.querySelector('#cls .field-data-histogram') as HTMLDivElement;
-      assert.strictEqual(clsHistogramEl.innerText, 'Good (≤0.10)\n10%\nOk (0.10-0.25)\n10%\nPoor (>0.25)\n80%');
+      assert.strictEqual(
+          clsHistogramEl.innerText, 'Good (≤0.10)\n10%\nNeeds improvement (0.10-0.25)\n10%\nPoor (>0.25)\n80%');
 
       const inpHistogramEl = view.shadowRoot?.querySelector('#inp .field-data-histogram');
       assert.isNull(inpHistogramEl);
@@ -286,10 +288,12 @@
       await coordinator.done();
 
       const lcpHistogramEl = view.shadowRoot?.querySelector('#lcp .field-data-histogram') as HTMLDivElement;
-      assert.strictEqual(lcpHistogramEl.innerText, 'Good (≤2.50 s)\n50%\nOk (2.50 s-4.00 s)\n30%\nPoor (>4.00 s)\n20%');
+      assert.strictEqual(
+          lcpHistogramEl.innerText, 'Good (≤2.50 s)\n50%\nNeeds improvement (2.50 s-4.00 s)\n30%\nPoor (>4.00 s)\n20%');
 
       const clsHistogramEl = view.shadowRoot?.querySelector('#cls .field-data-histogram') as HTMLDivElement;
-      assert.strictEqual(clsHistogramEl.innerText, 'Good (≤0.10)\n10%\nOk (0.10-0.25)\n10%\nPoor (>0.25)\n80%');
+      assert.strictEqual(
+          clsHistogramEl.innerText, 'Good (≤0.10)\n10%\nNeeds improvement (0.10-0.25)\n10%\nPoor (>0.25)\n80%');
 
       const inpHistogramEl = view.shadowRoot?.querySelector('#inp .field-data-histogram');
       assert.isNull(inpHistogramEl);
@@ -343,10 +347,12 @@
       await coordinator.done();
 
       const lcpHistogramEl = view.shadowRoot?.querySelector('#lcp .field-data-histogram') as HTMLDivElement;
-      assert.strictEqual(lcpHistogramEl.innerText, 'Good (≤2.50 s)\n50%\nOk (2.50 s-4.00 s)\n30%\nPoor (>4.00 s)\n20%');
+      assert.strictEqual(
+          lcpHistogramEl.innerText, 'Good (≤2.50 s)\n50%\nNeeds improvement (2.50 s-4.00 s)\n30%\nPoor (>4.00 s)\n20%');
 
       const clsHistogramEl = view.shadowRoot?.querySelector('#cls .field-data-histogram') as HTMLDivElement;
-      assert.strictEqual(clsHistogramEl.innerText, 'Good (≤0.10)\n10%\nOk (0.10-0.25)\n10%\nPoor (>0.25)\n80%');
+      assert.strictEqual(
+          clsHistogramEl.innerText, 'Good (≤0.10)\n10%\nNeeds improvement (0.10-0.25)\n10%\nPoor (>0.25)\n80%');
 
       const inpHistogramEl = view.shadowRoot?.querySelector('#inp .field-data-histogram');
       assert.isNull(inpHistogramEl);
```
```
The name of the file: front_end/panels/timeline/components/LiveMetricsView.ts
Insertions: 1, Deletions: 1.

@@ -186,7 +186,7 @@
         <span class="histogram-label">Good <span class="histogram-range">(&le;${format(thresholds[0])})</span></span>
         <span class="histogram-bar good-bg" style="width: ${goodPercent}"></span>
         <span>${goodPercent}</span>
-        <span class="histogram-label">Ok <span class="histogram-range">(${format(thresholds[0])}-${
+        <span class="histogram-label">Needs improvement <span class="histogram-range">(${format(thresholds[0])}-${
         format(thresholds[1])})</span></span>
         <span class="histogram-bar needs-improvement-bg" style="width: ${needsImprovementPercent}"></span>
         <span>${needsImprovementPercent}</span>
```
```
The name of the file: test/e2e/performance/landing-page_test.ts
Insertions: 6, Deletions: 6.

@@ -218,13 +218,13 @@
 
     assert.strictEqual(
         await lcpHistogram.evaluate(el => (el as HTMLElement).innerText),
-        'Good (≤2.50 s)\n96%\nOk (2.50 s-4.00 s)\n3%\nPoor (>4.00 s)\n1%');
+        'Good (≤2.50 s)\n96%\nNeeds improvement (2.50 s-4.00 s)\n3%\nPoor (>4.00 s)\n1%');
     assert.strictEqual(
         await clsHistogram.evaluate(el => (el as HTMLElement).innerText),
-        'Good (≤0.10)\n100%\nOk (0.10-0.25)\n0%\nPoor (>0.25)\n0%');
+        'Good (≤0.10)\n100%\nNeeds improvement (0.10-0.25)\n0%\nPoor (>0.25)\n0%');
     assert.strictEqual(
         await inpHistogram.evaluate(el => (el as HTMLElement).innerText),
-        'Good (≤200 ms)\n98%\nOk (200 ms-500 ms)\n2%\nPoor (>500 ms)\n1%');
+        'Good (≤200 ms)\n98%\nNeeds improvement (200 ms-500 ms)\n2%\nPoor (>500 ms)\n1%');
 
     const [lcpFieldValue, clsFieldValue, inpFieldValue] = await waitForMany(READY_FIELD_METRIC_SELECTOR, 3);
     assert.strictEqual(await lcpFieldValue.evaluate(el => el.textContent) || '', '1.20 s');
@@ -252,13 +252,13 @@
 
     assert.strictEqual(
         await lcpHistogram.evaluate(el => (el as HTMLElement).innerText),
-        'Good (≤2.50 s)\n96%\nOk (2.50 s-4.00 s)\n3%\nPoor (>4.00 s)\n1%');
+        'Good (≤2.50 s)\n96%\nNeeds improvement (2.50 s-4.00 s)\n3%\nPoor (>4.00 s)\n1%');
     assert.strictEqual(
         await clsHistogram.evaluate(el => (el as HTMLElement).innerText),
-        'Good (≤0.10)\n100%\nOk (0.10-0.25)\n0%\nPoor (>0.25)\n0%');
+        'Good (≤0.10)\n100%\nNeeds improvement (0.10-0.25)\n0%\nPoor (>0.25)\n0%');
     assert.strictEqual(
         await inpHistogram.evaluate(el => (el as HTMLElement).innerText),
-        'Good (≤200 ms)\n98%\nOk (200 ms-500 ms)\n2%\nPoor (>500 ms)\n1%');
+        'Good (≤200 ms)\n98%\nNeeds improvement (200 ms-500 ms)\n2%\nPoor (>500 ms)\n1%');
 
     const [lcpFieldValue, clsFieldValue, inpFieldValue] = await waitForMany(READY_FIELD_METRIC_SELECTOR, 3);
     assert.strictEqual(await lcpFieldValue.evaluate(el => el.textContent) || '', '1.20 s');
```
```
The name of the file: front_end/panels/timeline/components/liveMetricsView.css
Insertions: 1, Deletions: 2.

@@ -182,7 +182,7 @@
 .field-data-histogram {
   width: 100%;
   display: grid;
-  grid-template-columns: max-content auto max-content;
+  grid-template-columns: 50% auto max-content;
   grid-auto-rows: 1fr;
   column-gap: 8px;
   justify-items: flex-end;
@@ -196,7 +196,6 @@
 
 .histogram-label {
   width: 100%;
-  overflow: hidden;
   font-weight: bold;
 }
 
```

Change information

Commit message:
[RPP Observations] Display field p75 prominently and label histogram

This CL also determines the metric rating at the UI level so that we
can use the same rating logic for both local and field data. In the
future we should consolidate this logic further with the trace engine.

The histogram will eventually go in a tooltip, but to reduce complexity
in this CL the tooltip has been deferred to another CL.

https://screenshot.googleplex.com/BKy7jNUwgyfqswb
Bug: 313906362
Change-Id: I8f6ac410570aec9113e1555d74214d693c9b8184
Reviewed-by: Paul Irish <paul...@chromium.org>
Commit-Queue: Adam Raine <asr...@chromium.org>
Files:
  • M front_end/models/crux-manager/CrUXManager.ts
  • M front_end/models/live-metrics/LiveMetrics.ts
  • M front_end/models/live-metrics/web-vitals-injected/OnEachInteraction.ts
  • M front_end/models/live-metrics/web-vitals-injected/spec/spec.ts
  • M front_end/models/live-metrics/web-vitals-injected/web-vitals-injected.ts
  • M front_end/panels/timeline/components/LiveMetricsView.test.ts
  • M front_end/panels/timeline/components/LiveMetricsView.ts
  • M front_end/panels/timeline/components/liveMetricsView.css
  • M test/e2e/performance/landing-page_test.ts
Change size: L
Delta: 9 files changed, 232 insertions(+), 147 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Paul Irish
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I8f6ac410570aec9113e1555d74214d693c9b8184
Gerrit-Change-Number: 5665090
Gerrit-PatchSet: 11
Gerrit-Owner: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Adam Raine <asr...@chromium.org>
Gerrit-Reviewer: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Paul Irish <paul...@chromium.org>
open
diffy
satisfied_requirement
Reply all
Reply to author
Forward
0 new messages