Add UMA histograms to AsyncResourceHandler to track content size.

Add UMA histograms to AsyncResourceHandler to track content size.

This CL add UMA histograms, which are used to record how many times
content size is known and if it is precise enough to be used.
What is more, total size of read body is compared to the size of
allocated buffer, which is 512kb, to record how many times its size is
less than buffer's size.

BUG=622363
Review-Url: https://codereview.chromium.org/2624123002
Cr-Commit-Position: refs/heads/master@{#445016}
Committed: https://chromium.googlesource.com/chromium/src/+/18491e4f87ce2e5e53f64dad2a3da0a1f5688e01

[maksims (do not use this acc), 2017-01-11 10:24:09]

Patchset #1 (id:1) has been deleted

[maksims (do not use this acc), 2017-01-11 10:24:25]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 10:24:47]

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

[maksims (do not use this acc), 2017-01-11 10:26:18]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 10:26:37]

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-01-11 11:10:57]

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

[commit-bot: I haz the power, 2017-01-11 11:10:58]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2017-01-11 11:12:41]

Patchset #2 (id:40001) has been deleted

[maksims (do not use this acc), 2017-01-11 11:12:58]

Patchset #1 (id:20001) has been deleted

[maksims (do not use this acc), 2017-01-11 11:13:12]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 11:13:18]

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

[maksims (do not use this acc), 2017-01-11 11:13:21]

Description was changed from

==========
Add UMA histograms to AsyncResourceHandler to track content size.

This CL add UMA histograms, which are used to record how many times
content size is known and if it is precise enough to be used.
What is more, total size of read body is compared to the size of
allocated buffer, which is 512kb, to record how many times its size is
less than buffer's size.

BUG=622363
==========

to

==========
Add UMA histograms to AsyncResourceHandler to track content size.

This CL add UMA histograms, which are used to record how many times
content size is known and if it is precise enough to be used.
What is more, total size of read body is compared to the size of
allocated buffer, which is 512kb, to record how many times its size is
less than buffer's size.

BUG=622363
==========

[maksims (do not use this acc), 2017-01-11 11:13:22]

maksim.sisov@intel.com changed reviewers:
+ eroman@chromium.org, mmenke@chromium.org

[maksims (do not use this acc), 2017-01-11 11:15:04]

Do you think I should add, for example, a mean size value by which the buffer
could be shrank, which could be based on size of bodies?

[commit-bot: I haz the power, 2017-01-11 12:50:31]

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

[commit-bot: I haz the power, 2017-01-11 12:50:35]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[maksims (do not use this acc), 2017-01-11 12:59:52]

Some observations: 

I've surfed the net a bit using my desktop with fast connection and got the
following figures - total resource loaded - 1201.
Expected content size is known for 59.5% of resources.
Expected content size was equal to actual size of 95% of resources.

And the last one - Size of 99.6% of resource was less than allocated buffer's
size.

But, InliningHelper is not subtracted from the metrics. Should it be subtracted?

[mmenke, 2017-01-11 20:25:20]

Looks pretty good!

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:75: enum ExpectedContentSize {
"enum class" should generally be preferred to just enums

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:76:
CORRESPONDS_TO_RESPONSE_BODY = 0,
EQ_RESPONSE_BODY?

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:79: EXPECTED_CONTENT_SIZE,
nit:  EXPECTED_CONTENT_MAX is more common

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:565: // Record if content size
is known in advance.
nit:  is -> was (Since this is recorded at the end of a request).

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:567: if (expected_content_size
> 0) {
nit:  >= 0 (We use -1 to mean unknown, 0 means the body is known to be empty)

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:572: if (expected_content_size
== total_read_bytes_) {
Should we separate out those where expected_content_size is >= kBufferSize?

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:573: UMA_HISTOGRAM_ENUMERATION(
These UMA_HISTOGRAM_ENUMERATION are deceivingly expensive.  It would reduce
binary size to decide on the correct value, and then only have one call to add
an entry to the Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize
histogram.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:591: false);
Do we really need two histograms?  Seems like we could just make an
ExpectedContentSize::UNKNOWN and add it to the previous histogram.  This reducse
histogram upload size, and reduces data on the servers (Each recorded datapoint
is a database entry, I believe).  So unless we get something from additional
histograms we can't get from just using a calculator and a smaller number of
histograms, best to use fewer histograms.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:598: total_read_bytes_ <
kBufferSize);
I don't think we need this - can just add up the values of
ExpectedContentSize::CORRESPONDS_TO_RESPONSE_BODY and
ExpectedContentSize::LT_RESPONSE_BODY.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
File content/browser/loader/async_resource_handler.h (right):

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.h:83: int64_t total_read_bytes_;
Maybe total_read_body_bytes_?

[maksims (do not use this acc), 2017-01-12 09:03:03]

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:75: enum ExpectedContentSize {
On 2017/01/11 20:25:19, mmenke wrote:
> "enum class" should generally be preferred to just enums

invalid operands to binary expression ('content::(anonymous
namespace)::ExpectedContentSize' and 'int')

Macro includes arithmetic operations, which cannot be done on enums.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:572: if (expected_content_size
== total_read_bytes_) {
On 2017/01/11 20:25:19, mmenke wrote:
> Should we separate out those where expected_content_size is >= kBufferSize?

Do you think I should add another enum entry, which will be something like
"EQ_RESPONSE_BODY_MT_EQ_BUFFER_SIZE"?

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:591: false);
On 2017/01/11 20:25:19, mmenke wrote:
> Do we really need two histograms?  Seems like we could just make an
> ExpectedContentSize::UNKNOWN and add it to the previous histogram.  This
reducse
> histogram upload size, and reduces data on the servers (Each recorded
datapoint
> is a database entry, I believe).  So unless we get something from additional
> histograms we can't get from just using a calculator and a smaller number of
> histograms, best to use fewer histograms.

sounds reasonable.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:598: total_read_bytes_ <
kBufferSize);
On 2017/01/11 20:25:20, mmenke wrote:
> I don't think we need this - can just add up the values of
> ExpectedContentSize::CORRESPONDS_TO_RESPONSE_BODY and
> ExpectedContentSize::LT_RESPONSE_BODY.

But we'll loose data, which tells us how accurate expected content size is
comparing to total body size, then.

[mmenke, 2017-01-12 18:04:11]

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:75: enum ExpectedContentSize {
On 2017/01/12 09:03:03, maksims wrote:
> On 2017/01/11 20:25:19, mmenke wrote:
> > "enum class" should generally be preferred to just enums
> 
> invalid operands to binary expression ('content::(anonymous
> namespace)::ExpectedContentSize' and 'int')
> 
> Macro includes arithmetic operations, which cannot be done on enums.

Ah, right, good point.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:572: if (expected_content_size
== total_read_bytes_) {
On 2017/01/12 09:03:02, maksims wrote:
> On 2017/01/11 20:25:19, mmenke wrote:
> > Should we separate out those where expected_content_size is >= kBufferSize?
> 
> Do you think I should add another enum entry, which will be something like
> "EQ_RESPONSE_BODY_MT_EQ_BUFFER_SIZE"?

I'm fine with that.  I'm not sure how much it gets us, but it can't hurt, and
better to have it and not need it than need it and not have it.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:598: total_read_bytes_ <
kBufferSize);
On 2017/01/12 09:03:02, maksims wrote:
> On 2017/01/11 20:25:20, mmenke wrote:
> > I don't think we need this - can just add up the values of
> > ExpectedContentSize::CORRESPONDS_TO_RESPONSE_BODY and
> > ExpectedContentSize::LT_RESPONSE_BODY.
> 
> But we'll loose data, which tells us how accurate expected content size is
> comparing to total body size, then.

Sorry, I wasn't clear.  I mean keep them as separate buckets in the histogram,
but if we ever want to know what this histogram would look like, we can just add
those two buckets in the
"Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize" histogram, so we
don't need a separate histogram for them.

[maksims (do not use this acc), 2017-01-13 07:24:36]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 07:24:51]

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-01-13 08:22:53]

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

[commit-bot: I haz the power, 2017-01-13 08:22:54]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2017-01-13 12:05:18]

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:76:
CORRESPONDS_TO_RESPONSE_BODY = 0,
On 2017/01/11 20:25:20, mmenke wrote:
> EQ_RESPONSE_BODY?

Done.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:79: EXPECTED_CONTENT_SIZE,
On 2017/01/11 20:25:19, mmenke wrote:
> nit:  EXPECTED_CONTENT_MAX is more common

Done.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:565: // Record if content size
is known in advance.
On 2017/01/11 20:25:20, mmenke wrote:
> nit:  is -> was (Since this is recorded at the end of a request).

Done.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:567: if (expected_content_size
> 0) {
On 2017/01/11 20:25:19, mmenke wrote:
> nit:  >= 0 (We use -1 to mean unknown, 0 means the body is known to be empty)

Done.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:572: if (expected_content_size
== total_read_bytes_) {
On 2017/01/12 18:04:10, mmenke wrote:
> On 2017/01/12 09:03:02, maksims wrote:
> > On 2017/01/11 20:25:19, mmenke wrote:
> > > Should we separate out those where expected_content_size is >=
kBufferSize?
> > 
> > Do you think I should add another enum entry, which will be something like
> > "EQ_RESPONSE_BODY_MT_EQ_BUFFER_SIZE"?
> 
> I'm fine with that.  I'm not sure how much it gets us, but it can't hurt, and
> better to have it and not need it than need it and not have it.

Done.

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:573: UMA_HISTOGRAM_ENUMERATION(
On 2017/01/11 20:25:19, mmenke wrote:
> These UMA_HISTOGRAM_ENUMERATION are deceivingly expensive.  It would reduce
> binary size to decide on the correct value, and then only have one call to add
> an entry to the Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize
> histogram.

I see. Do you think I should use something else?

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:591: false);
On 2017/01/11 20:25:19, mmenke wrote:
> Do we really need two histograms?  Seems like we could just make an
> ExpectedContentSize::UNKNOWN and add it to the previous histogram.  This
reducse
> histogram upload size, and reduces data on the servers (Each recorded
datapoint
> is a database entry, I believe).  So unless we get something from additional
> histograms we can't get from just using a calculator and a smaller number of
> histograms, best to use fewer histograms.

Done.

[mmenke, 2017-01-13 16:01:36]

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/60001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:573: UMA_HISTOGRAM_ENUMERATION(
On 2017/01/13 12:05:18, maksims wrote:
> On 2017/01/11 20:25:19, mmenke wrote:
> > These UMA_HISTOGRAM_ENUMERATION are deceivingly expensive.  It would reduce
> > binary size to decide on the correct value, and then only have one call to
add
> > an entry to the Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize
> > histogram.
> 
> I see. Do you think I should use something else?

No, but you should only have one instance of "UMA_HISTOGRAM_ENUMERATION".

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:568: if (expected_content_size
>= 0) {
Having ExpectedContentSize be an enum that's not a size is a bit confusing,
particularly when we have a local variable expected_content_size that is a size.

Maybe rename ExpectedContentSize to ExpectedContentSizeResult or
ExpectedContentSizeCategory or somesuch.

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:573:
"Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize",
Suggest just replacing "ExpectedContentSize.CorrespondsBodySize" with whatever
we use for the above enumeration (Now that we only have one histogram, don't
need to subdivide it further).

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:575:
ExpectedContentSize::EXPECTED_CONTENT_MAX);
You only need one UMA_HISTOGRAM_ENUMERATION call here.

ExpectedContentSize expected_size = ExpectedContentSize::UNKNOWN;
if (expected_content_size >= 0) {
  if (...) {
    expected_size = ...;
  } else {
    ...
  }
}
UMA_HISTOGRAM_ENUMERATION(...);

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:598: total_read_body_bytes_ =
0;
Not needed.  This class is only used to read one response body before
destruction, to don't need to clear this.

[maksims (do not use this acc), 2017-01-16 07:25:15]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[maksims (do not use this acc), 2017-01-16 07:25:22]

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:568: if (expected_content_size
>= 0) {
On 2017/01/13 16:01:36, mmenke wrote:
> Having ExpectedContentSize be an enum that's not a size is a bit confusing,
> particularly when we have a local variable expected_content_size that is a
size.
> 
> Maybe rename ExpectedContentSize to ExpectedContentSizeResult or
> ExpectedContentSizeCategory or somesuch.

Done.

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:573:
"Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize",
On 2017/01/13 16:01:36, mmenke wrote:
> Suggest just replacing "ExpectedContentSize.CorrespondsBodySize" with whatever
> we use for the above enumeration (Now that we only have one histogram, don't
> need to subdivide it further).

Done.

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:575:
ExpectedContentSize::EXPECTED_CONTENT_MAX);
On 2017/01/13 16:01:36, mmenke wrote:
> You only need one UMA_HISTOGRAM_ENUMERATION call here.
> 
> ExpectedContentSize expected_size = ExpectedContentSize::UNKNOWN;
> if (expected_content_size >= 0) {
>   if (...) {
>     expected_size = ...;
>   } else {
>     ...
>   }
> }
> UMA_HISTOGRAM_ENUMERATION(...);

Done.

https://codereview.chromium.org/2624123002/diff/80001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:598: total_read_body_bytes_ =
0;
On 2017/01/13 16:01:36, mmenke wrote:
> Not needed.  This class is only used to read one response body before
> destruction, to don't need to clear this.

Done.

[commit-bot: I haz the power, 2017-01-16 07:25:24]

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-01-16 08:29:27]

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

[commit-bot: I haz the power, 2017-01-16 08:29:28]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-17 15:58:27]

Suggest you add an owner of tools/metrics/histograms/histograms.xml - think
we're pretty much done (And sorry for the extra day delay, yesterday was a US
Holiday)

https://codereview.chromium.org/2624123002/diff/100001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/100001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:576:
ExpectedContentSizeResult::EQ_RESPONSE_BODY_MT_EQ_BUFFER_SIZE;
I think we should have another bucket for other "expected_content_size >=
kBufferSize" cases - they basically don't matter when deciding if we can do
better setting buffer size, unless we want to increase it in some cases (Which
we don't)

https://codereview.chromium.org/2624123002/diff/100001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:585:
ExpectedContentSizeResult::MT_RESPONSE_BODY;
nit:  GT's more common.

[maksims (do not use this acc), 2017-01-18 07:50:25]

maksim.sisov@intel.com changed reviewers:
+ jochen@chromium.org

[maksims (do not use this acc), 2017-01-18 07:50:26]

jochen@, would you please review changes in histograms.xml?

https://codereview.chromium.org/2624123002/diff/100001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/100001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:576:
ExpectedContentSizeResult::EQ_RESPONSE_BODY_MT_EQ_BUFFER_SIZE;
On 2017/01/17 15:58:27, mmenke wrote:
> I think we should have another bucket for other "expected_content_size >=
> kBufferSize" cases - they basically don't matter when deciding if we can do
> better setting buffer size, unless we want to increase it in some cases (Which
> we don't)

I see. Added!

https://codereview.chromium.org/2624123002/diff/100001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:585:
ExpectedContentSizeResult::MT_RESPONSE_BODY;
On 2017/01/17 15:58:27, mmenke wrote:
> nit:  GT's more common.

Done.

[maksims (do not use this acc), 2017-01-18 07:50:43]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 07:51:03]

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-01-18 08:48:07]

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

[commit-bot: I haz the power, 2017-01-18 08:48:08]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-18 12:31:02]

I can only approve UseCounter additoins. For this histograms.xml change, please
ask somebody from tools/metrics/OWNERS!

[maksims (do not use this acc), 2017-01-18 12:34:01]

maksim.sisov@intel.com changed reviewers:
- jochen@chromium.org

[maksims (do not use this acc), 2017-01-18 12:34:31]

maksim.sisov@intel.com changed reviewers:
+ jwd@chromium.org

[maksims (do not use this acc), 2017-01-18 12:34:53]

jwd@, would you please review tools/metrics/histograms/histograms.xml

[mmenke, 2017-01-18 16:28:40]

LGTM

[jwd, 2017-01-18 20:03:06]

https://codereview.chromium.org/2624123002/diff/120001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:76: enum
ExpectedContentSizeResult {
Please add comment saying this enum is used for logging a histogram and should
not be reordered.

https://codereview.chromium.org/2624123002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2624123002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:34771: +<histogram
name="Net.ResourceLoader.TotalBodySize.LT_AsyncHandlerBufferSize"
Where is this being logged?

https://codereview.chromium.org/2624123002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:34772: +    enum="Boolean">
Can you use a more precise enum, maybe add BooleanLessThan.

[maksims (do not use this acc), 2017-01-19 05:56:42]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[maksims (do not use this acc), 2017-01-19 05:56:50]

https://codereview.chromium.org/2624123002/diff/120001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2624123002/diff/120001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:76: enum
ExpectedContentSizeResult {
On 2017/01/18 20:03:06, jwd wrote:
> Please add comment saying this enum is used for logging a histogram and should
> not be reordered.

Done.

https://codereview.chromium.org/2624123002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2624123002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:34771: +<histogram
name="Net.ResourceLoader.TotalBodySize.LT_AsyncHandlerBufferSize"
On 2017/01/18 20:03:06, jwd wrote:
> Where is this being logged?

I'm sorry, I've forgotten to remove these.

[commit-bot: I haz the power, 2017-01-19 05:57:08]

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-01-19 06:57:24]

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

[commit-bot: I haz the power, 2017-01-19 06:57:25]

Dry run: This issue passed the CQ dry run.

[jwd, 2017-01-19 21:21:36]

Histograms LGTM with follow up about
Net.ResourceLoader.TotalBodySize.LT_AsyncHandlerBufferSize.

https://codereview.chromium.org/2624123002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2624123002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:34771: +<histogram
name="Net.ResourceLoader.TotalBodySize.LT_AsyncHandlerBufferSize"
Are you going to remove this?

[maksims (do not use this acc), 2017-01-20 05:51:01]

https://codereview.chromium.org/2624123002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2624123002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:34771: +<histogram
name="Net.ResourceLoader.TotalBodySize.LT_AsyncHandlerBufferSize"
On 2017/01/19 21:21:36, jwd wrote:
> Are you going to remove this?

Oh, sorry. Yes. thanks for pointing to it.

[maksims (do not use this acc), 2017-01-20 06:43:11]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2017-01-20 06:43:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2624123002/#ps160001
(title: "remove not used entry in histogram.xml")

[commit-bot: I haz the power, 2017-01-20 06:43:35]

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-01-20 08:19:59]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484894591745300,
"parent_rev": "be437ee31e429d47e9c66da37e6dc889ac015440", "commit_rev":
"18491e4f87ce2e5e53f64dad2a3da0a1f5688e01"}

[commit-bot: I haz the power, 2017-01-20 08:20:28]

Description was changed from

==========
Add UMA histograms to AsyncResourceHandler to track content size.

This CL add UMA histograms, which are used to record how many times
content size is known and if it is precise enough to be used.
What is more, total size of read body is compared to the size of
allocated buffer, which is 512kb, to record how many times its size is
less than buffer's size.

BUG=622363
==========

to

==========
Add UMA histograms to AsyncResourceHandler to track content size.

This CL add UMA histograms, which are used to record how many times
content size is known and if it is precise enough to be used.
What is more, total size of read body is compared to the size of
allocated buffer, which is 512kb, to record how many times its size is
less than buffer's size.

BUG=622363

Review-Url: https://codereview.chromium.org/2624123002
Cr-Commit-Position: refs/heads/master@{#445016}
Committed:
https://chromium.googlesource.com/chromium/src/+/18491e4f87ce2e5e53f64dad2a3d...
==========

[commit-bot: I haz the power, 2017-01-20 08:20:30]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/18491e4f87ce2e5e53f64dad2a3d...