Add UMA for number of outstanding requests in ResourceDispatcherHostImpl

Add UMA for number of outstanding requests in ResourceDispatcherHostImpl

This CL adds UMA to track the largest numbers of outstanding requests we have
seen so far in ResourceDispatcherHostImpl.

This is to understand how many requests from content/browser/loader can be
outstanding at a given time.

BUG=711721
Review-Url: https://codereview.chromium.org/2828733003
Cr-Commit-Position: refs/heads/master@{#466197}
Committed: https://chromium.googlesource.com/chromium/src/+/d1d500db43d0662a32d615cbf7ad8adc5f2d5ba3

[xunjieli, 2017-04-19 20:59:31]

xunjieli@chromium.org changed reviewers:
+ csharrison@chromium.org, mmenke@chromium.org

[xunjieli, 2017-04-19 20:59:31]

Charlie & Matt: PTAL.
I am trying to understand whether the 20k outstanding net::HttpStreamRequests
(in crbug.com/711721) is a normal number. ResourceDispatcherHost has a ~6k limit
on the number of outstanding requests per renderer. I guess 20k can be achieved
if a user has multiple renderers making requests aggressively. I want to see if
things are WAI.

Let me know if you have any suggestions.

[Charlie Harrison, 2017-04-19 23:08:28]

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1909: if
(num_in_flight_requests_ > largest_outstanding_request_count_seen_) {
I wonder if this would be better logged in Shutdown? Then you only get the max
requests for a given session.

[xunjieli, 2017-04-20 13:34:11]

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1909: if
(num_in_flight_requests_ > largest_outstanding_request_count_seen_) {
On 2017/04/19 23:08:28, Charlie Harrison wrote:
> I wonder if this would be better logged in Shutdown? Then you only get the max
> requests for a given session.

Similar to crbug.com/704988, Chrome on Android never shuts down and some users 
seldom shut down their browser.

[Charlie Harrison, 2017-04-20 13:39:16]

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1909: if
(num_in_flight_requests_ > largest_outstanding_request_count_seen_) {
On 2017/04/20 13:34:11, xunjieli wrote:
> On 2017/04/19 23:08:28, Charlie Harrison wrote:
> > I wonder if this would be better logged in Shutdown? Then you only get the
max
> > requests for a given session.
> 
> Similar to crbug.com/704988, Chrome on Android never shuts down and some users

> seldom shut down their browser. 

OK, this LGTM but it would be preferable to hook into NetworkMetricsProvider if
it's feasible.

[xunjieli, 2017-04-20 20:06:09]

Matt: do you want to take a look before I looping in a histograms.xml owner?

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1909: if
(num_in_flight_requests_ > largest_outstanding_request_count_seen_) {
On 2017/04/20 13:39:16, Charlie Harrison wrote:
> On 2017/04/20 13:34:11, xunjieli wrote:
> > On 2017/04/19 23:08:28, Charlie Harrison wrote:
> > > I wonder if this would be better logged in Shutdown? Then you only get the
> max
> > > requests for a given session.
> > 
> > Similar to crbug.com/704988, Chrome on Android never shuts down and some
users
> 
> > seldom shut down their browser. 
> 
> OK, this LGTM but it would be preferable to hook into NetworkMetricsProvider
if
> it's feasible.

Acknowledged. I didn't find a way to hook it up with NetworkMetricsProvider. I
don't think content/ can depend on components/, can it?

[mmenke, 2017-04-20 20:07:47]

On 2017/04/20 20:06:09, xunjieli wrote:
> Matt: do you want to take a look before I looping in a histograms.xml owner?

I'll just defer to Charles, feel free to proceed.

[Charlie Harrison, 2017-04-20 20:09:14]

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1909: if
(num_in_flight_requests_ > largest_outstanding_request_count_seen_) {
On 2017/04/20 20:06:08, xunjieli wrote:
> On 2017/04/20 13:39:16, Charlie Harrison wrote:
> > On 2017/04/20 13:34:11, xunjieli wrote:
> > > On 2017/04/19 23:08:28, Charlie Harrison wrote:
> > > > I wonder if this would be better logged in Shutdown? Then you only get
the
> > max
> > > > requests for a given session.
> > > 
> > > Similar to crbug.com/704988, Chrome on Android never shuts down and some
> users
> > 
> > > seldom shut down their browser. 
> > 
> > OK, this LGTM but it would be preferable to hook into NetworkMetricsProvider
> if
> > it's feasible.
> 
> Acknowledged. I didn't find a way to hook it up with NetworkMetricsProvider. I
> don't think content/ can depend on components/, can it?

One possible way to implement it would be to pull information out of the RDH
singleton in the component. It's pretty ugly though. We'd have to change it for
the network service later too. Let's just land this, and loop back if we think
the data is not up to snuff.

[xunjieli, 2017-04-20 20:14:15]

xunjieli@chromium.org changed reviewers:
+ holte@chromium.org
- mmenke@chromium.org

[xunjieli, 2017-04-20 20:14:16]

+holte@: PTAL at histograms.xml
Thanks!

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2828733003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1909: if
(num_in_flight_requests_ > largest_outstanding_request_count_seen_) {
On 2017/04/20 20:09:14, Charlie Harrison wrote:
> On 2017/04/20 20:06:08, xunjieli wrote:
> > On 2017/04/20 13:39:16, Charlie Harrison wrote:
> > > On 2017/04/20 13:34:11, xunjieli wrote:
> > > > On 2017/04/19 23:08:28, Charlie Harrison wrote:
> > > > > I wonder if this would be better logged in Shutdown? Then you only get
> the
> > > max
> > > > > requests for a given session.
> > > > 
> > > > Similar to crbug.com/704988, Chrome on Android never shuts down and some
> > users
> > > 
> > > > seldom shut down their browser. 
> > > 
> > > OK, this LGTM but it would be preferable to hook into
NetworkMetricsProvider
> > if
> > > it's feasible.
> > 
> > Acknowledged. I didn't find a way to hook it up with NetworkMetricsProvider.
I
> > don't think content/ can depend on components/, can it?
> 
> One possible way to implement it would be to pull information out of the RDH
> singleton in the component. It's pretty ugly though. We'd have to change it
for
> the network service later too. Let's just land this, and loop back if we think
> the data is not up to snuff.

Acknowledged. Thanks.

[xunjieli, 2017-04-20 20:14:22]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[xunjieli, 2017-04-20 20:14:43]

Description was changed from

==========
Add UMA for number of outstanding requests in ResourceDispatcherHostImpl

This CL adds UMA to track the largest numbers of outstanding requests we have
seen so far in ResourceDispatcherHostImpl.

This is to understand how many requests from content/browser/loader can be
outstanding at a given time.

BUG=711721
==========

to

==========
Add UMA for number of outstanding requests in ResourceDispatcherHostImpl

This CL adds UMA to track the largest numbers of outstanding requests we have
seen so far in ResourceDispatcherHostImpl.

This is to understand how many requests from content/browser/loader can be
outstanding at a given time.

BUG=711721
==========

[commit-bot: I haz the power, 2017-04-20 20:15:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 21:36:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 21:36:05]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-04-21 00:19:48]

lgtm

[xunjieli, 2017-04-21 00:20:11]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-04-21 00:20:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 00:32:52]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492734011143690, "parent_rev":
"2484bfc638fd6dc5188d20a64e48034f299d9d26", "commit_rev":
"d1d500db43d0662a32d615cbf7ad8adc5f2d5ba3"}

[commit-bot: I haz the power, 2017-04-21 00:33:37]

Description was changed from

==========
Add UMA for number of outstanding requests in ResourceDispatcherHostImpl

This CL adds UMA to track the largest numbers of outstanding requests we have
seen so far in ResourceDispatcherHostImpl.

This is to understand how many requests from content/browser/loader can be
outstanding at a given time.

BUG=711721
==========

to

==========
Add UMA for number of outstanding requests in ResourceDispatcherHostImpl

This CL adds UMA to track the largest numbers of outstanding requests we have
seen so far in ResourceDispatcherHostImpl.

This is to understand how many requests from content/browser/loader can be
outstanding at a given time.

BUG=711721

Review-Url: https://codereview.chromium.org/2828733003
Cr-Commit-Position: refs/heads/master@{#466197}
Committed:
https://chromium.googlesource.com/chromium/src/+/d1d500db43d0662a32d615cbf7ad...
==========

[commit-bot: I haz the power, 2017-04-21 00:33:39]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/d1d500db43d0662a32d615cbf7ad...