New issue
Advanced search Search tips

Issue 854851 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Filter no-fetch-event controllers from most UMA about service worker controlled page loads

Project Member Reported by falken@chromium.org, Jun 21 2018

Issue description

Currently we log perf metrics like  PageLoad.Clients.ServiceWorker.PaintTiming* whenever a page has a controller service worker. However, if that controller didn't have a fetch event handler, no service worker was started or involved in loading the page, since we have a no fetch event handler optimization. We aren't   interested in these cases for such UMA, so the UMA shouldn't include them.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d5c7dd3680d7018c785549be54ae9b0e837ef59

commit 4d5c7dd3680d7018c785549be54ae9b0e837ef59
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Jun 22 14:10:05 2018

Remove Memory.*.ServiceWorkerControlledMainFrameDidFinishLoad UMA.

This UMA can be misleading because it also is recorded for
controllers without fetch event handlers. These service workers
don't run on page load and don't intercept requests, so it's
probably best to not count them.

But we are not currently using the UMA, and it's under
"Experimental" so we can remove it now and add back
an improved metric if the need arises.

Bug:  854851 
Change-Id: I6f40b573aa950c44a761f8c7ceebf3b7d8ded665
Reviewed-on: https://chromium-review.googlesource.com/1111751
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569602}
[modify] https://crrev.com/4d5c7dd3680d7018c785549be54ae9b0e837ef59/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/4d5c7dd3680d7018c785549be54ae9b0e837ef59/tools/metrics/histograms/histograms.xml

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c5b3ef7d542a3be683839b3d90b4f64bf6082cb

commit 6c5b3ef7d542a3be683839b3d90b4f64bf6082cb
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jun 26 03:09:26 2018

service worker: Filter no-fetch-event controllers from some UMA.

When a page has a controller service worker without a fetch event
handler, the service worker is not started and does not intercept
requests.  Therefore we don't care about such loads for most UMA about
controlled page loads.

Bug:  854851 
Change-Id: I55198f43177c3d2468028672380bd2a8d08eb1f4
Reviewed-on: https://chromium-review.googlesource.com/1109314
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570317}
[modify] https://crrev.com/6c5b3ef7d542a3be683839b3d90b4f64bf6082cb/chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc
[modify] https://crrev.com/6c5b3ef7d542a3be683839b3d90b4f64bf6082cb/third_party/blink/renderer/core/loader/document_loader.cc
[modify] https://crrev.com/6c5b3ef7d542a3be683839b3d90b4f64bf6082cb/tools/metrics/histograms/histograms.xml

Comment 3 by falken@chromium.org, Jun 27 2018

Labels: M-69

Comment 4 by falken@chromium.org, Jun 27 2018

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ab9c21956e2f5208ea6b162bfe9c0d8e3b1f313

commit 9ab9c21956e2f5208ea6b162bfe9c0d8e3b1f313
Author: Matt Falkenhagen <falken@chromium.org>
Date: Sat Jul 07 00:38:21 2018

histograms: Add Clients.ServiceWorker2* as affected_histogram to relevant suffixes.

ServiceWorker2 replaced ServiceWorker and should be listed as an affected_histogram
wherever ServiceWorker was.

Missed in r570317 (https://chromium-review.googlesource.com/1109314)

Bug:  854851 
Change-Id: I03320e68b72d38b8062356fdba12fda542501530
Reviewed-on: https://chromium-review.googlesource.com/1127901
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573129}
[modify] https://crrev.com/9ab9c21956e2f5208ea6b162bfe9c0d8e3b1f313/tools/metrics/histograms/histograms.xml

Sign in to add a comment