Add UMA to track the number of live URLRequests.

Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

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

[xunjieli, 2017-04-25 17:00:48]

Description was changed from

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active HttpStreamRequests
at a given time. The number reported by ResourceDispatcherHostImpl doesn't
account for the large number of HttpStreamRequests. This CL adds a UMA to
track the number of active URLRequests, so we can know whether the number of
HttpStreamRequests is internal or external to the network stack.

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

to

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active HttpStreamRequests
at a given time. The number reported by ResourceDispatcherHostImpl doesn't
account for the large number of HttpStreamRequests. This CL adds a histogram to
track the number of active URLRequests, so we can know whether the number of
HttpStreamRequests is internal or external to the network stack.

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

[xunjieli, 2017-04-25 17:01:47]

Description was changed from

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active HttpStreamRequests
at a given time. The number reported by ResourceDispatcherHostImpl doesn't
account for the large number of HttpStreamRequests. This CL adds a histogram to
track the number of active URLRequests, so we can know whether the number of
HttpStreamRequests is internal or external to the network stack.

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

to

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large number of
HttpStreamRequests is internal or external to the network 
stack.

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

[xunjieli, 2017-04-25 17:01:47]

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

[xunjieli, 2017-04-25 17:01:53]

Description was changed from

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large number of
HttpStreamRequests is internal or external to the network 
stack.

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

to

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

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

[xunjieli, 2017-04-25 17:10:06]

Hi Matt:  early data from UMA (go/outstanding_stream_request_4252017) indicates
that there can be as many as 10k active HttpStreamRequests (for a single
HttpStreamFactory) at one time, while ResourceHostDispatcherImpl reports only
<=3k outstanding requests. 

I will wait for more data to come in, but can we add a new histogram to see if a
URLRequestContext could have that many outstanding HttpStreamRequests? I would
like to see if the API is being abused or something else is going on internal to
the net stack.

(+csharrison@: FHI)

[xunjieli, 2017-04-25 17:10:13]

Patchset #1 (id:1) has been deleted

[mmenke, 2017-04-25 17:33:00]

On 2017/04/25 17:10:06, xunjieli wrote:
> Hi Matt:  early data from UMA (go/outstanding_stream_request_4252017)
indicates
> that there can be as many as 10k active HttpStreamRequests (for a single
> HttpStreamFactory) at one time, while ResourceHostDispatcherImpl reports only
> <=3k outstanding requests. 
> 
> I will wait for more data to come in, but can we add a new histogram to see if
a
> URLRequestContext could have that many outstanding HttpStreamRequests? I would
> like to see if the API is being abused or something else is going on internal
to
> the net stack.
> 
> (+csharrison@: FHI)

Should this do the same thing as the RDH code does, to make the values at least
a little more comparable?

[mmenke, 2017-04-25 17:47:14]

On 2017/04/25 17:33:00, mmenke wrote:
> On 2017/04/25 17:10:06, xunjieli wrote:
> > Hi Matt:  early data from UMA (go/outstanding_stream_request_4252017)
> indicates
> > that there can be as many as 10k active HttpStreamRequests (for a single
> > HttpStreamFactory) at one time, while ResourceHostDispatcherImpl reports
only
> > <=3k outstanding requests. 
> > 
> > I will wait for more data to come in, but can we add a new histogram to see
if
> a
> > URLRequestContext could have that many outstanding HttpStreamRequests? I
would
> > like to see if the API is being abused or something else is going on
internal
> to
> > the net stack.
> > 
> > (+csharrison@: FHI)
> 
> Should this do the same thing as the RDH code does, to make the values at
least
> a little more comparable?

SHould be aware that the values are actually not at all comparable, since an RDH
uses multiple URLRequestContexts, and there are a couple of URLRequestContexts
used for only a few requests (Like SafeBrowsing), but if you just look at the
upper end of the spectrum, the numbers should be somewhat comparable, with the
change.

[xunjieli, 2017-04-25 18:14:55]

On 2017/04/25 17:47:14, mmenke wrote:
> On 2017/04/25 17:33:00, mmenke wrote:
> > On 2017/04/25 17:10:06, xunjieli wrote:
> > > Hi Matt:  early data from UMA (go/outstanding_stream_request_4252017)
> > indicates
> > > that there can be as many as 10k active HttpStreamRequests (for a single
> > > HttpStreamFactory) at one time, while ResourceHostDispatcherImpl reports
> only
> > > <=3k outstanding requests. 
> > > 
> > > I will wait for more data to come in, but can we add a new histogram to
see
> if
> > a
> > > URLRequestContext could have that many outstanding HttpStreamRequests? I
> would
> > > like to see if the API is being abused or something else is going on
> internal
> > to
> > > the net stack.
> > > 
> > > (+csharrison@: FHI)
> > 
> > Should this do the same thing as the RDH code does, to make the values at
> least
> > a little more comparable?
> 
> SHould be aware that the values are actually not at all comparable, since an
RDH
> uses multiple URLRequestContexts, and there are a couple of URLRequestContexts
> used for only a few requests (Like SafeBrowsing), but if you just look at the
> upper end of the spectrum, the numbers should be somewhat comparable, with the
> change.

URLRequestContext::CreateRequest() is a const method. I can't track the largest
seen there without modifying the method signature.  Do you have any suggestion?
Yes, I am only interested in the upper end of the spectrum,. The number tracked
by a URLRequestContext should be comparable to that of JobController which is
associated with a HttpStreamFactory.

[mmenke, 2017-04-25 18:19:50]

On 2017/04/25 18:14:55, xunjieli wrote:
> On 2017/04/25 17:47:14, mmenke wrote:
> > On 2017/04/25 17:33:00, mmenke wrote:
> > > On 2017/04/25 17:10:06, xunjieli wrote:
> > > > Hi Matt:  early data from UMA (go/outstanding_stream_request_4252017)
> > > indicates
> > > > that there can be as many as 10k active HttpStreamRequests (for a single
> > > > HttpStreamFactory) at one time, while ResourceHostDispatcherImpl reports
> > only
> > > > <=3k outstanding requests. 
> > > > 
> > > > I will wait for more data to come in, but can we add a new histogram to
> see
> > if
> > > a
> > > > URLRequestContext could have that many outstanding HttpStreamRequests? I
> > would
> > > > like to see if the API is being abused or something else is going on
> > internal
> > > to
> > > > the net stack.
> > > > 
> > > > (+csharrison@: FHI)
> > > 
> > > Should this do the same thing as the RDH code does, to make the values at
> > least
> > > a little more comparable?
> > 
> > SHould be aware that the values are actually not at all comparable, since an
> RDH
> > uses multiple URLRequestContexts, and there are a couple of
URLRequestContexts
> > used for only a few requests (Like SafeBrowsing), but if you just look at
the
> > upper end of the spectrum, the numbers should be somewhat comparable, with
the
> > change.
> 
> URLRequestContext::CreateRequest() is a const method. I can't track the
largest
> seen there without modifying the method signature.  Do you have any
suggestion?
> Yes, I am only interested in the upper end of the spectrum,. The number
tracked
> by a URLRequestContext should be comparable to that of JobController which is
> associated with a HttpStreamFactory.

We kind of cheat, in that URLRequest::URLRequest's constructor (Which is called
from URLRequestContext::CreateRequest()) modifies URLRequestContext::requests_. 
Could something similar for the URLRequest counter - in fact, seems like a good
idea to make this part of adding a request to requests_, so the logic is always
coupled.

[xunjieli, 2017-04-25 19:24:34]

On 2017/04/25 18:19:50, mmenke wrote:
> On 2017/04/25 18:14:55, xunjieli wrote:
> > On 2017/04/25 17:47:14, mmenke wrote:
> > > On 2017/04/25 17:33:00, mmenke wrote:
> > > > On 2017/04/25 17:10:06, xunjieli wrote:
> > > > > Hi Matt:  early data from UMA (go/outstanding_stream_request_4252017)
> > > > indicates
> > > > > that there can be as many as 10k active HttpStreamRequests (for a
single
> > > > > HttpStreamFactory) at one time, while ResourceHostDispatcherImpl
reports
> > > only
> > > > > <=3k outstanding requests. 
> > > > > 
> > > > > I will wait for more data to come in, but can we add a new histogram
to
> > see
> > > if
> > > > a
> > > > > URLRequestContext could have that many outstanding HttpStreamRequests?
I
> > > would
> > > > > like to see if the API is being abused or something else is going on
> > > internal
> > > > to
> > > > > the net stack.
> > > > > 
> > > > > (+csharrison@: FHI)
> > > > 
> > > > Should this do the same thing as the RDH code does, to make the values
at
> > > least
> > > > a little more comparable?
> > > 
> > > SHould be aware that the values are actually not at all comparable, since
an
> > RDH
> > > uses multiple URLRequestContexts, and there are a couple of
> URLRequestContexts
> > > used for only a few requests (Like SafeBrowsing), but if you just look at
> the
> > > upper end of the spectrum, the numbers should be somewhat comparable, with
> the
> > > change.
> > 
> > URLRequestContext::CreateRequest() is a const method. I can't track the
> largest
> > seen there without modifying the method signature.  Do you have any
> suggestion?
> > Yes, I am only interested in the upper end of the spectrum,. The number
> tracked
> > by a URLRequestContext should be comparable to that of JobController which
is
> > associated with a HttpStreamFactory.
> 
> We kind of cheat, in that URLRequest::URLRequest's constructor (Which is
called
> from URLRequestContext::CreateRequest()) modifies
URLRequestContext::requests_. 
> Could something similar for the URLRequest counter - in fact, seems like a
good
> idea to make this part of adding a request to requests_, so the logic is
always
> coupled.

Thanks, Matt. I uploaded a Patch #2. Is this what you had in mind? PTAL.

[mmenke, 2017-04-25 20:58:47]

On 2017/04/25 19:24:34, xunjieli wrote:
> On 2017/04/25 18:19:50, mmenke wrote:
> > On 2017/04/25 18:14:55, xunjieli wrote:
> > > On 2017/04/25 17:47:14, mmenke wrote:
> > > > On 2017/04/25 17:33:00, mmenke wrote:
> > > > > On 2017/04/25 17:10:06, xunjieli wrote:
> > > > > > Hi Matt:  early data from UMA
(go/outstanding_stream_request_4252017)
> > > > > indicates
> > > > > > that there can be as many as 10k active HttpStreamRequests (for a
> single
> > > > > > HttpStreamFactory) at one time, while ResourceHostDispatcherImpl
> reports
> > > > only
> > > > > > <=3k outstanding requests. 
> > > > > > 
> > > > > > I will wait for more data to come in, but can we add a new histogram
> to
> > > see
> > > > if
> > > > > a
> > > > > > URLRequestContext could have that many outstanding
HttpStreamRequests?
> I
> > > > would
> > > > > > like to see if the API is being abused or something else is going on
> > > > internal
> > > > > to
> > > > > > the net stack.
> > > > > > 
> > > > > > (+csharrison@: FHI)
> > > > > 
> > > > > Should this do the same thing as the RDH code does, to make the values
> at
> > > > least
> > > > > a little more comparable?
> > > > 
> > > > SHould be aware that the values are actually not at all comparable,
since
> an
> > > RDH
> > > > uses multiple URLRequestContexts, and there are a couple of
> > URLRequestContexts
> > > > used for only a few requests (Like SafeBrowsing), but if you just look
at
> > the
> > > > upper end of the spectrum, the numbers should be somewhat comparable,
with
> > the
> > > > change.
> > > 
> > > URLRequestContext::CreateRequest() is a const method. I can't track the
> > largest
> > > seen there without modifying the method signature.  Do you have any
> > suggestion?
> > > Yes, I am only interested in the upper end of the spectrum,. The number
> > tracked
> > > by a URLRequestContext should be comparable to that of JobController which
> is
> > > associated with a HttpStreamFactory.
> > 
> > We kind of cheat, in that URLRequest::URLRequest's constructor (Which is
> called
> > from URLRequestContext::CreateRequest()) modifies
> URLRequestContext::requests_. 
> > Could something similar for the URLRequest counter - in fact, seems like a
> good
> > idea to make this part of adding a request to requests_, so the logic is
> always
> > coupled.
> 
> Thanks, Matt. I uploaded a Patch #2. Is this what you had in mind? PTAL.

Sorry, I was trying to suggestion you could combine the insert call and the
logging, and adding a tracking variable to URLRequestContext.  If you prefer to
just do this, that's fine, I'm just not sure if you want to compare this to the
result of the RDH histogram or not.

[xunjieli, 2017-04-25 21:21:20]

https://codereview.chromium.org/2837313002/diff/60001/net/url_request/url_req...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2837313002/diff/60001/net/url_request/url_req...
net/url_request/url_request_context.cc:130:
largest_outstanding_requests_count_seen_ = url_requests_->size();
Matt: sorry, I might be missing something. How do I modify the tracking variable
(|largest_outstanding_requests_count_seen_|) while keeping InsertURLRequest() a
const method?

Thanks!

[mmenke, 2017-04-25 21:50:48]

https://codereview.chromium.org/2837313002/diff/60001/net/url_request/url_req...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2837313002/diff/60001/net/url_request/url_req...
net/url_request/url_request_context.cc:130:
largest_outstanding_requests_count_seen_ = url_requests_->size();
On 2017/04/25 21:21:20, xunjieli wrote:
> Matt: sorry, I might be missing something. How do I modify the tracking
variable
> (|largest_outstanding_requests_count_seen_|) while keeping InsertURLRequest()
a
> const method?
> 
> Thanks!

I assume this doesn't build because InsertURLRequest is not const?

I had assumed that since this was modifying url_requests_, we had non-const
access to the URLRequestContext, but I guess we store it in a unique_ptr just to
get around this issue?  That seems weird....  I'd suggest just making this
mutable instead.

Sorry that I didn't realize that earlier.  This solution would have worked with
your original CL, but I didn't realize the unique_ptr magic.  I think combining
this with set insertion is prettier, anyways, though.

[xunjieli, 2017-04-25 22:04:23]

Description was changed from

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

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

to

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

BUG=711721
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-04-25 22:06:03]

Description was changed from

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

BUG=711721
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

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

[xunjieli, 2017-04-25 22:07:39]

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

[xunjieli, 2017-04-25 22:07:40]

Thanks, Matt. Done.
+holte@ PTAL at histograms.xml. Thanks

[xunjieli, 2017-04-25 22:07:50]

Patchset #4 (id:80001) has been deleted

[Steven Holte, 2017-04-26 01:38:40]

lgtm

[mmenke, 2017-04-26 14:24:35]

Looks great, LGTM!

[xunjieli, 2017-04-26 14:32:19]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-04-26 14:32:34]

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-26 15:51:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493217139230580,
"parent_rev": "d434dda595a984d1d39697d991ef03626c55b041", "commit_rev":
"3fcbcc190e8d78af50840b7535893a8662b5e74b"}

[commit-bot: I haz the power, 2017-04-26 15:51:38]

Description was changed from

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

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

to

==========
Add UMA to track the number of live URLRequests.

UMA data shows that there can be a large number of active 
HttpStreamRequests at a given time. The number reported by 
ResourceDispatcherHostImpl doesn't account for the large number of 
HttpStreamRequests. This CL adds a histogram to track the number of 
active URLRequests, so we can know whether the cause for the large 
number of HttpStreamRequests is internal or external to the network 
stack.

BUG=711721

Review-Url: https://codereview.chromium.org/2837313002
Cr-Commit-Position: refs/heads/master@{#467332}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fcbcc190e8d78af50840b753589...
==========

[commit-bot: I haz the power, 2017-04-26 15:51:39]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3fcbcc190e8d78af50840b753589...