Use NetworkTrafficAnnotation in URLRequestContext UMA logging

Use NetworkTrafficAnnotation in URLRequestContext UMA logging

This CL makes use of NetworkTrafficAnnotation that is associated with 
each URLRequest to log UMA histograms.

When 'Net.URLRequestContext.OutstandingRequests' is logged, we
additionally log what the URLRequest is created for. This hopefully
can tell us why we have a large number of OutstandingRequests in the
extreme scenarios.

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

[xunjieli, 2017-05-24 15:06:26]

Description was changed from

==========
temp


temp


temp


temp


temp


temp


Partial Protobuf for Traffic Annotation.

In many use cases, it's better to define some part of a network traffic
annotation in one function and the reset of it in another one. like when a
function makes the initial request and controls policies, and then passes it
to another one to set/unset the cookies and perform the actual request. This CL
adds required definitions to make partial annotations.

BUG=656607

patch from issue 2815293002 at patchset 160001
(http://crrev.com/2815293002#ps160001)

patch from issue 2869373002 at patchset 420001
(http://crrev.com/2869373002#ps420001)

temp
==========

to

==========
temp
==========

[xunjieli, 2017-05-24 15:06:46]

Patchset #1 (id:1) has been deleted

[xunjieli, 2017-05-24 15:26:28]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2017-05-24 15:31:55]

Description was changed from

==========
temp
==========

to

==========
Use NetworkTrafficAnnotation in URLRequestContext UMA logging

This CL makes use of NetworkTrafficAnnotation that is associated with 
each URLRequest to log UMA histograms.

When 'Net.URLRequestContext.OutstandingRequests' is logged, we
additionally log what the URLRequest is created for. This hopefully
can tell us why we have a large number of OutstandingRequests in the
extreme scenarios.
==========

[xunjieli, 2017-05-24 15:45:05]

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

[xunjieli, 2017-05-24 15:45:06]

Matt: what do you think about this approach?
Ideally, we want to log how much of OutstandingRequests is by a consumer, but I
am unable to find a way to do that. The suggestion to log multidimensional
histogram
(https://chromium.googlesource.com/chromium/src/+/7682c6e584d676e2f002ab15aa84...)
isn't very applicable here.

[mmenke, 2017-05-24 15:51:46]

On 2017/05/24 15:45:06, xunjieli wrote:
> Matt: what do you think about this approach?
> Ideally, we want to log how much of OutstandingRequests is by a consumer, but
I
> am unable to find a way to do that. The suggestion to log multidimensional
> histogram
>
(https://chromium.googlesource.com/chromium/src/+/7682c6e584d676e2f002ab15aa84...)
> isn't very applicable here.

Is there a way to validate hash codes is complete and accurate, as a presubmit
or something?  Also, why do we need the hash codes in two files?

[mmenke, 2017-05-24 15:52:27]

On 2017/05/24 15:51:46, mmenke wrote:
> On 2017/05/24 15:45:06, xunjieli wrote:
> > Matt: what do you think about this approach?
> > Ideally, we want to log how much of OutstandingRequests is by a consumer,
but
> I
> > am unable to find a way to do that. The suggestion to log multidimensional
> > histogram
> >
>
(https://chromium.googlesource.com/chromium/src/+/7682c6e584d676e2f002ab15aa84...)
> > isn't very applicable here.
> 
> Is there a way to validate hash codes is complete and accurate, as a presubmit
> or something?  Also, why do we need the hash codes in two files?

(Actually, a step on the builders would make more sense than a presubmit)

[xunjieli, 2017-05-24 16:01:55]

On 2017/05/24 15:52:27, mmenke wrote:
> On 2017/05/24 15:51:46, mmenke wrote:
> > On 2017/05/24 15:45:06, xunjieli wrote:
> > > Matt: what do you think about this approach?
> > > Ideally, we want to log how much of OutstandingRequests is by a consumer,
> but
> > I
> > > am unable to find a way to do that. The suggestion to log multidimensional
> > > histogram
> > >
> >
>
(https://chromium.googlesource.com/chromium/src/+/7682c6e584d676e2f002ab15aa84...)
> > > isn't very applicable here.
> > 
> > Is there a way to validate hash codes is complete and accurate, as a
presubmit
> > or something?  Also, why do we need the hash codes in two files?
> 
> (Actually, a step on the builders would make more sense than a presubmit)

rhalavati@ (cc-ed) is working on adding a test step (either presubmit or a unit
test) to validate the hash codes. But that's not there yet. I can wait for him
to land the check first. 
On why I had the hash codes in both enums.xml and hash_codes.txt is that the
validation script once written will check against a changelist and the existing
mapping. I kept an extra copy because the enums.xml file is rather large. We can
certainly keep only one copy of the mapping.

[mmenke, 2017-05-24 16:06:40]

On 2017/05/24 16:01:55, xunjieli wrote:
> On 2017/05/24 15:52:27, mmenke wrote:
> > On 2017/05/24 15:51:46, mmenke wrote:
> > > On 2017/05/24 15:45:06, xunjieli wrote:
> > > > Matt: what do you think about this approach?
> > > > Ideally, we want to log how much of OutstandingRequests is by a
consumer,
> > but
> > > I
> > > > am unable to find a way to do that. The suggestion to log
multidimensional
> > > > histogram
> > > >
> > >
> >
>
(https://chromium.googlesource.com/chromium/src/+/7682c6e584d676e2f002ab15aa84...)
> > > > isn't very applicable here.
> > > 
> > > Is there a way to validate hash codes is complete and accurate, as a
> presubmit
> > > or something?  Also, why do we need the hash codes in two files?
> > 
> > (Actually, a step on the builders would make more sense than a presubmit)
> 
> rhalavati@ (cc-ed) is working on adding a test step (either presubmit or a
unit
> test) to validate the hash codes. But that's not there yet. I can wait for him
> to land the check first. 
> On why I had the hash codes in both enums.xml and hash_codes.txt is that the
> validation script once written will check against a changelist and the
existing
> mapping. I kept an extra copy because the enums.xml file is rather large. We
can
> certainly keep only one copy of the mapping.

Great!  LGTM, then.

[mmenke, 2017-05-24 16:07:52]

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

https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
net/url_request/url_request_context.cc:153:
std::numeric_limits<base::HistogramBase::Sample>::max() - 1);
Erm...wait...is this right?  SHould this be a sparse histogram or something?  I
think UMA_HISTOGRAM_ENUMERATION creates one bucket for every value?

[xunjieli, 2017-05-24 16:13:00]

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

[xunjieli, 2017-05-24 16:18:16]

+ asvitkine@
Alexei, could you comment on the UMA question below. 
Thank you!

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

https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
net/url_request/url_request_context.cc:153:
std::numeric_limits<base::HistogramBase::Sample>::max() - 1);
On 2017/05/24 16:07:52, mmenke wrote:
> Erm...wait...is this right?  SHould this be a sparse histogram or something? 
I
> think UMA_HISTOGRAM_ENUMERATION creates one bucket for every value?

I am not sure. I read the documentation on sparse histogram, but it says sparse
histogram is for histograms that are logged infrequently. Since
URLRequestContext::InsertURLRequest() is called for every network request, I am
not sure if that fits this use case.

+ asvitkine@
Alexei, I am adding a new histogram enum type, which is an unsigned int32. There
are about 100 of these values. What's the best way to log this data?

[Alexei Svitkine (slow), 2017-05-24 17:13:29]

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

https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
net/url_request/url_request_context.cc:153:
std::numeric_limits<base::HistogramBase::Sample>::max() - 1);
On 2017/05/24 16:18:16, xunjieli wrote:
> On 2017/05/24 16:07:52, mmenke wrote:
> > Erm...wait...is this right?  SHould this be a sparse histogram or something?

> I
> > think UMA_HISTOGRAM_ENUMERATION creates one bucket for every value?
> 
> I am not sure. I read the documentation on sparse histogram, but it says
sparse
> histogram is for histograms that are logged infrequently. Since
> URLRequestContext::InsertURLRequest() is called for every network request, I
am
> not sure if that fits this use case.
> 
> + asvitkine@
> Alexei, I am adding a new histogram enum type, which is an unsigned int32.
There
> are about 100 of these values. What's the best way to log this data? 

Do the values go outside of the signed int32 range? If not, the best way is to
use a sparse histogram. (UMA_HISTOGRAM_SPARSE()).

[xunjieli, 2017-05-24 17:23:01]

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

[xunjieli, 2017-05-24 17:23:02]

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

https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
net/url_request/url_request_context.cc:153:
std::numeric_limits<base::HistogramBase::Sample>::max() - 1);
On 2017/05/24 17:13:29, Alexei Svitkine (slow) wrote:
> On 2017/05/24 16:18:16, xunjieli wrote:
> > On 2017/05/24 16:07:52, mmenke wrote:
> > > Erm...wait...is this right?  SHould this be a sparse histogram or
something?
> 
> > I
> > > think UMA_HISTOGRAM_ENUMERATION creates one bucket for every value?
> > 
> > I am not sure. I read the documentation on sparse histogram, but it says
> sparse
> > histogram is for histograms that are logged infrequently. Since
> > URLRequestContext::InsertURLRequest() is called for every network request, I
> am
> > not sure if that fits this use case.
> > 
> > + asvitkine@
> > Alexei, I am adding a new histogram enum type, which is an unsigned int32.
> There
> > are about 100 of these values. What's the best way to log this data? 
> 
> Do the values go outside of the signed int32 range? If not, the best way is to
> use a sparse histogram. (UMA_HISTOGRAM_SPARSE()).

Thanks. 
+ rhalavati@, can we switch to signed int32 for hash code in your CL?

[Ramin Halavati, 2017-05-25 06:11:39]

On 2017/05/24 17:23:02, xunjieli wrote:
>
https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
> File net/url_request/url_request_context.cc (right):
> 
>
https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
> net/url_request/url_request_context.cc:153:
> std::numeric_limits<base::HistogramBase::Sample>::max() - 1);
> On 2017/05/24 17:13:29, Alexei Svitkine (slow) wrote:
> > On 2017/05/24 16:18:16, xunjieli wrote:
> > > On 2017/05/24 16:07:52, mmenke wrote:
> > > > Erm...wait...is this right?  SHould this be a sparse histogram or
> something?
> > 
> > > I
> > > > think UMA_HISTOGRAM_ENUMERATION creates one bucket for every value?
> > > 
> > > I am not sure. I read the documentation on sparse histogram, but it says
> > sparse
> > > histogram is for histograms that are logged infrequently. Since
> > > URLRequestContext::InsertURLRequest() is called for every network request,
I
> > am
> > > not sure if that fits this use case.
> > > 
> > > + asvitkine@
> > > Alexei, I am adding a new histogram enum type, which is an unsigned int32.
> > There
> > > are about 100 of these values. What's the best way to log this data? 
> > 
> > Do the values go outside of the signed int32 range? If not, the best way is
to
> > use a sparse histogram. (UMA_HISTOGRAM_SPARSE()).
> 
> Thanks. 
> + rhalavati@, can we switch to signed int32 for hash code in your CL?

I changed the type to int32

[Alexei Svitkine (slow), 2017-05-25 14:39:13]

On 2017/05/25 06:11:39, Ramin Halavati wrote:
> On 2017/05/24 17:23:02, xunjieli wrote:
> >
>
https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
> > File net/url_request/url_request_context.cc (right):
> > 
> >
>
https://codereview.chromium.org/2897143003/diff/40001/net/url_request/url_req...
> > net/url_request/url_request_context.cc:153:
> > std::numeric_limits<base::HistogramBase::Sample>::max() - 1);
> > On 2017/05/24 17:13:29, Alexei Svitkine (slow) wrote:
> > > On 2017/05/24 16:18:16, xunjieli wrote:
> > > > On 2017/05/24 16:07:52, mmenke wrote:
> > > > > Erm...wait...is this right?  SHould this be a sparse histogram or
> > something?
> > > 
> > > > I
> > > > > think UMA_HISTOGRAM_ENUMERATION creates one bucket for every value?
> > > > 
> > > > I am not sure. I read the documentation on sparse histogram, but it says
> > > sparse
> > > > histogram is for histograms that are logged infrequently. Since
> > > > URLRequestContext::InsertURLRequest() is called for every network
request,
> I
> > > am
> > > > not sure if that fits this use case.
> > > > 
> > > > + asvitkine@
> > > > Alexei, I am adding a new histogram enum type, which is an unsigned
int32.
> > > There
> > > > are about 100 of these values. What's the best way to log this data? 
> > > 
> > > Do the values go outside of the signed int32 range? If not, the best way
is
> to
> > > use a sparse histogram. (UMA_HISTOGRAM_SPARSE()).
> > 
> > Thanks. 
> > + rhalavati@, can we switch to signed int32 for hash code in your CL?
> 
> I changed the type to int32

Please use UMA_HISTOGRAM_SPARSE_SLOWLY to log the histogram.

[xunjieli, 2017-05-25 16:01:17]

Patchset #2 (id:60001) has been deleted

[xunjieli, 2017-05-25 16:04:14]

> Please use UMA_HISTOGRAM_SPARSE_SLOWLY to log the histogram.

Done. Thanks! PTAL.
Ramin's CL has landed. I rebased and used Ramin's new mapping.

[Alexei Svitkine (slow), 2017-05-25 16:23:08]

lgtm

https://codereview.chromium.org/2897143003/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2897143003/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/enums.xml:35492: +<enum name="URLRequestAnnotationType"
type="int">
Please add a comment here about how this is generated.

https://codereview.chromium.org/2897143003/diff/100001/tools/traffic_annotati...
File tools/traffic_annotation/hash_codes.txt (right):

https://codereview.chromium.org/2897143003/diff/100001/tools/traffic_annotati...
tools/traffic_annotation/hash_codes.txt:1: <int value="485305"
label="data_reduction_proxy_config"/>
Is this file needed given the same info is in enums.xml?

[xunjieli, 2017-05-25 16:55:37]

Thanks!

https://codereview.chromium.org/2897143003/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2897143003/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/enums.xml:35492: +<enum name="URLRequestAnnotationType"
type="int">
On 2017/05/25 16:23:08, Alexei Svitkine (slow) wrote:
> Please add a comment here about how this is generated.

Done.

https://codereview.chromium.org/2897143003/diff/100001/tools/traffic_annotati...
File tools/traffic_annotation/hash_codes.txt (right):

https://codereview.chromium.org/2897143003/diff/100001/tools/traffic_annotati...
tools/traffic_annotation/hash_codes.txt:1: <int value="485305"
label="data_reduction_proxy_config"/>
On 2017/05/25 16:23:08, Alexei Svitkine (slow) wrote:
> Is this file needed given the same info is in enums.xml?

Done. Removed.

[xunjieli, 2017-05-25 16:55:59]

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

[commit-bot: I haz the power, 2017-05-25 16:56:19]

Dry run: CQ is trying da patch.

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

[xunjieli, 2017-05-25 16:57:04]

Description was changed from

==========
Use NetworkTrafficAnnotation in URLRequestContext UMA logging

This CL makes use of NetworkTrafficAnnotation that is associated with 
each URLRequest to log UMA histograms.

When 'Net.URLRequestContext.OutstandingRequests' is logged, we
additionally log what the URLRequest is created for. This hopefully
can tell us why we have a large number of OutstandingRequests in the
extreme scenarios.
==========

to

==========
Use NetworkTrafficAnnotation in URLRequestContext UMA logging

This CL makes use of NetworkTrafficAnnotation that is associated with 
each URLRequest to log UMA histograms.

When 'Net.URLRequestContext.OutstandingRequests' is logged, we
additionally log what the URLRequest is created for. This hopefully
can tell us why we have a large number of OutstandingRequests in the
extreme scenarios.

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

[xunjieli, 2017-05-25 17:00:30]

Matt, are you fine with me landing this now? Ramin is working on the validation
test. We can get some data from M60 which is branching today, or wait until the
validation test is checked in. What is your preference?

[mmenke, 2017-05-25 17:15:19]

On 2017/05/25 17:00:30, xunjieli wrote:
> Matt, are you fine with me landing this now? Ramin is working on the
validation
> test. We can get some data from M60 which is branching today, or wait until
the
> validation test is checked in. What is your preference?

I'm fine with landing this today

[mmenke, 2017-05-25 17:16:26]

On 2017/05/25 17:15:19, mmenke wrote:
> On 2017/05/25 17:00:30, xunjieli wrote:
> > Matt, are you fine with me landing this now? Ramin is working on the
> validation
> > test. We can get some data from M60 which is branching today, or wait until
> the
> > validation test is checked in. What is your preference?
> 
> I'm fine with landing this today

Reasoning is just that even if the tables are wrong, the code should still work,
and it's a pretty simple change.

[xunjieli, 2017-05-25 17:16:59]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-05-25 17:17:03]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-05-25 17:17:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2897143003/#ps120001
(title: "Address Alexei's comments")

[commit-bot: I haz the power, 2017-05-25 17:17:36]

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-05-25 18:11:09]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495732623656820,
"parent_rev": "7105587c22a9a0fe7e113d5a6006ba69772e46f8", "commit_rev":
"669c310279272d3202f2784c4323b392a52498d3"}

[commit-bot: I haz the power, 2017-05-25 18:11:26]

Description was changed from

==========
Use NetworkTrafficAnnotation in URLRequestContext UMA logging

This CL makes use of NetworkTrafficAnnotation that is associated with 
each URLRequest to log UMA histograms.

When 'Net.URLRequestContext.OutstandingRequests' is logged, we
additionally log what the URLRequest is created for. This hopefully
can tell us why we have a large number of OutstandingRequests in the
extreme scenarios.

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

to

==========
Use NetworkTrafficAnnotation in URLRequestContext UMA logging

This CL makes use of NetworkTrafficAnnotation that is associated with 
each URLRequest to log UMA histograms.

When 'Net.URLRequestContext.OutstandingRequests' is logged, we
additionally log what the URLRequest is created for. This hopefully
can tell us why we have a large number of OutstandingRequests in the
extreme scenarios.

BUG=711721

Review-Url: https://codereview.chromium.org/2897143003
Cr-Commit-Position: refs/heads/master@{#474706}
Committed:
https://chromium.googlesource.com/chromium/src/+/669c310279272d3202f2784c4323...
==========

[commit-bot: I haz the power, 2017-05-25 18:11:28]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/669c310279272d3202f2784c4323...