New issue
Advanced search Search tips

Issue 853211 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug
Proj-Servicification



Sign in to add a comment

Network service: Hook up network histograms.

Project Member Reported by mmenke@chromium.org, Jun 15 2018

Issue description

In particular, the Histograms logged in  ResourceDispatcherHostImpl::DidFinishLoading are not recorder when the network service is enabled (Net.ErrorCodesForMainFrame and friends, some cert / TLS histograms, and some time until success histograms)

Also, ResourceLoader histograms are not recorded (Net.ResourceLoader.TimeToURLRequestStart, Navigation.BackForward.WasCached, Net.ResourceLoader.ReadDeferral, Net.HttpResponseInfo.ConnectionInfo.MainFrame, and Net.Prefetch histograms).

I think we want most of these working before launching on Canary, so we have more data to look at.
 

Comment 1 by mmenke@chromium.org, Jun 18 2018

There are also histograms in chrome_network_delegate.cc (Net.HttpRequestCompletionErrorCodes.MainFrame) that we should presumably preserve (That one, in particular, we should probably move to the same place we move the other main frame histograms).

Comment 2 by mmenke@chromium.org, Jun 19 2018

Owner: mmenke@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 20 2018

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

commit 4cc4b91548e0b7944577e0fb5a2fa683f13ac096
Author: Matt Menke <mmenke@chromium.org>
Date: Wed Jun 20 12:53:03 2018

NetworkService: Hook up NetworkChangeNotifier's HistogramWatcher.

Unfortunately, the old patch to set it up in IOThread is still needed,
since the in-process NetworkService is leaked when the OOP network
service is disabled.

Bug= 853211 

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ic216b2d83d703cff776a77aaea6a2244a9eca367
Reviewed-on: https://chromium-review.googlesource.com/1105068
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568806}
[modify] https://crrev.com/4cc4b91548e0b7944577e0fb5a2fa683f13ac096/net/base/network_change_notifier.cc
[modify] https://crrev.com/4cc4b91548e0b7944577e0fb5a2fa683f13ac096/services/network/network_change_manager.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 20 2018

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

commit 28cab8257d212d7b07cc431cbd775a255e531aa4
Author: Matt Menke <mmenke@chromium.org>
Date: Wed Jun 20 20:52:37 2018

Move Net.HttpRequestCompletionErrorCodes to NetworkContext.

This will cause histogram will be collected both when the network
service is enabled, and when it's not.

Also update LayeredNetworkDelegate::OnCompletedInternal to pass in the
net_error.

Bug:  853211 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I8294b26b9cbe031cdb54adf7fc752c6717605b95
Reviewed-on: https://chromium-review.googlesource.com/1108083
Reviewed-by: Scott Little <sclittle@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569004}
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/chrome/browser/net/chrome_network_delegate.cc
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/chrome/browser/net/chrome_network_delegate_unittest.cc
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/components/data_use_measurement/core/data_use_network_delegate.cc
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/components/data_use_measurement/core/data_use_network_delegate.h
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/net/base/layered_network_delegate.cc
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/net/base/layered_network_delegate.h
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/net/base/layered_network_delegate_unittest.cc
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/services/network/network_context.cc
[modify] https://crrev.com/28cab8257d212d7b07cc431cbd775a255e531aa4/services/network/network_context_unittest.cc

Comment 5 by dxie@google.com, Jun 27 2018

Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows

Comment 6 by mmenke@chromium.org, Jun 29 2018

Cc: csharrison@chromium.org
So I've run into some issues here with ErrorCodesForMainFrame / ErrorCodesForSubresources:

My initial plan was to hook up everything through a WebContentsObserver, since navigations can be monitored there, and it has a ResourceLoadComplete method.  Unfortunately, that doesn't currently work.

For subresources, ResourceLoadComplete is not invoked for aborted requests (At least requests aborted by the ResourceDispatcher - not sure about resources that are failed with ERR_ABORTED by the loading stack).  Also not sure if, in the case a navigation is interrupted, if the old RenderFrameHost can be destoyed before error messages make it to the WebContents.  Frame resources may have the same problem (And that's before we even get into things that we decide to download).

We could just to log frame requests at the WebContents layer - I think we can see main frame aborts of all types there, if we're careful, and log subresources at the ResourceDispatcher layer.

Worth noting that whatever we do here, I think we're going to lose logging of downloads, which I'm fine with, as long as we don't consider them aborts.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 6

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

commit a294b0902431602456c50538a5c0b0649c3abe69
Author: Matt Menke <mmenke@chromium.org>
Date: Fri Jul 06 17:53:32 2018

NetworkService: Hook up ErrorCodesForMainFrame and related histograms.

Since the network service has (or should have...) no concept of main
frames, and only sees network requests (As opposed to file requests,
etc), this requires moving the code into the ResourceDispatcher and,
to catch aborts and failures that don't receive any headers, the
NavigationURLLoader code as well.

Bug:  853211 
Change-Id: Ib969220cc66b1d0fed6babfaf5e1c7d9a54dbfec
Reviewed-on: https://chromium-review.googlesource.com/1121148
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573011}
[add] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/chrome/browser/net/network_request_metrics_browsertest.cc
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/chrome/test/BUILD.gn
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/components/metrics/net/network_metrics_provider.cc
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/components/metrics/net/network_metrics_provider.h
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/content/common/BUILD.gn
[add] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/content/common/net/record_load_histograms.cc
[add] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/content/common/net/record_load_histograms.h
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/net/websockets/websocket_stream.cc
[modify] https://crrev.com/a294b0902431602456c50538a5c0b0649c3abe69/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)
Marking this as fixed - there are other histograms in rdh and resource_loader, but I've filed separate bugs for the rest of them.

Sign in to add a comment