Add histograms for network activity, and total/cumulative

Add histograms for network activity, and total/cumulative
histograms for cache/network activity.
R=rvargas@chromium.org, mmenke@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152398

[mmenke, 2012-08-15 15:09:53]

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:748: ReportNetworkActionStart();
I'm a bit concerned about mismatched Report*() calls, especially as the code
gets modified (Not that this is a particularly often modified file).

Have two suggestions for alleviating these concerns:

1)  Have HttpTransactionDelegateImpl DCHECK on unexpected FINISH events.
2)  Or have a HttpCache::Transaction::SetNextState(State) function, which may
calls a Finish() function based on the old value of next_state_, and/or call a
Start() function based on the new value.

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:2098: OK);
optional nit:  Can just use EndEvent.  "OK" is ignored, to avoid spamming it all
over the log.

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:84: virtual void OnNetworkActionFinish() {}
Should add EXPECTs for these, too.

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:88: bool action_in_progress_;
cache_action_in_progess_?

[tburkard, 2012-08-15 16:50:46]

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
File net/http/http_cache_transaction.cc (right):

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
net/http/http_cache_transaction.cc:748: ReportNetworkActionStart();
I can add that to the HttpTransactionDelegateImpl, however, it will need to keep
some extra state to keep track of it.  Last time, you asked me to remove such
extra state.. So are you ok with that extra state being added to the delegate
this time?  Please advise.

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
net/http/http_cache_transaction.cc:2098: OK);
On 2012/08/15 15:09:53, Matt Menke wrote:
> optional nit:  Can just use EndEvent.  "OK" is ignored, to avoid spamming it
all
> over the log.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
File net/http/http_cache_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
net/http/http_cache_unittest.cc:84: virtual void OnNetworkActionFinish() {}
On 2012/08/15 15:09:53, Matt Menke wrote:
> Should add EXPECTs for these, too.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
net/http/http_cache_unittest.cc:88: bool action_in_progress_;
On 2012/08/15 15:09:53, Matt Menke wrote:
> cache_action_in_progess_?

Done.

[mmenke, 2012-08-15 18:45:41]

I completely forgot to look at cache stats last time.  Not completely done with
it yet.

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
File net/http/http_cache_transaction.cc (right):

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
net/http/http_cache_transaction.cc:748: ReportNetworkActionStart();
On 2012/08/15 16:50:46, tburkard wrote:
> I can add that to the HttpTransactionDelegateImpl, however, it will need to
keep
> some extra state to keep track of it.  Last time, you asked me to remove such
> extra state.. So are you ok with that extra state being added to the delegate
> this time?  Please advise.

When the state was solely for use elsewhere, I thought it belonged where it was
being used.  When it's being used to ensure correctness, I'm fine with it.  In
case I was being unclear, those were intended to be either-or options.  Doing
both would be overkill.

I'm open to other ideas, or just leaving it as-is, but I do think regressions
here are a real concern, and could easily go unnoticed.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData {
Think a comment stating that this records the time taken by all requests while
they have a certain status would make this code clearer.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:146: }
suggest a blank line after every function to improve readability.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:148: base::TimeTicks current_start_time;
Think last_update_time would be more accurate.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:151: };
Inner classes/structs go before constructors.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:153: PerRequestStatusData
per_status_stats[REQUEST_STATUS_MAX];
Your variable name and struct names should be more consistent.  I think that
PerStatusData or PerStatusStats is clearest.  "PerRequestStatusData" sounds a
bit like it's per request, which confused me at first.  May want to remove the
"Per" as well.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:208: RequestStatus status =
static_cast<RequestStatus>(status_int);
It looks to me like we don't need this.  Can just rename status_int to status
(Or maybe, request_status), and use it directly.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:212: HistogramType type =
static_cast<HistogramType>(type_int);
Same as above.  I'd also suggest using histogram_type rather than type, as it's
a little too ambiguous.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:304: RequestStatus status =
static_cast<RequestStatus>(status_int);
Can just use status again.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:310: status_stats->num_active--;
Why not just mess with the stats for old_status and new_status?

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:322: stats->active_requests[&request] =
new_status;
This could be put in the above if statement, right?

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:333: RequestStatus status =
static_cast<RequestStatus>(status_int);
Again, don't need status_int and status.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
File chrome/browser/net/cache_stats.h (right):

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.h:69: };
Neither of these two new enumerations is needed in the header file.

[mmenke, 2012-08-15 19:08:34]

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:748: ReportNetworkActionStart();
On 2012/08/15 18:45:41, Matt Menke wrote:
> On 2012/08/15 16:50:46, tburkard wrote:
> > I can add that to the HttpTransactionDelegateImpl, however, it will need to
> keep
> > some extra state to keep track of it.  Last time, you asked me to remove
such
> > extra state.. So are you ok with that extra state being added to the
delegate
> > this time?  Please advise.
> 
> When the state was solely for use elsewhere, I thought it belonged where it
was
> being used.  When it's being used to ensure correctness, I'm fine with it.  In
> case I was being unclear, those were intended to be either-or options.  Doing
> both would be overkill.
> 
> I'm open to other ideas, or just leaving it as-is, but I do think regressions
> here are a real concern, and could easily go unnoticed.

Actually, my real objection is that it was in net/ and assumed the cache
behavior was completely different from what it really was - which seemed to me
like it might be a reasonable assumption in chrome/, but not net/.

[tburkard, 2012-08-15 20:42:23]

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
File net/http/http_cache_transaction.cc (right):

https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cach...
net/http/http_cache_transaction.cc:748: ReportNetworkActionStart();
On 2012/08/15 15:09:53, Matt Menke wrote:
> I'm a bit concerned about mismatched Report*() calls, especially as the code
> gets modified (Not that this is a particularly often modified file).
> 
> Have two suggestions for alleviating these concerns:
> 
> 1)  Have HttpTransactionDelegateImpl DCHECK on unexpected FINISH events.
> 2)  Or have a HttpCache::Transaction::SetNextState(State) function, which may
> calls a Finish() function based on the old value of next_state_, and/or call a
> Start() function based on the new value.

Done.

[tburkard, 2012-08-15 21:05:56]

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData {
On 2012/08/15 18:45:41, Matt Menke wrote:
> Think a comment stating that this records the time taken by all requests while
> they have a certain status would make this code clearer.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:146: }
On 2012/08/15 18:45:41, Matt Menke wrote:
> suggest a blank line after every function to improve readability.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:148: base::TimeTicks current_start_time;
On 2012/08/15 18:45:41, Matt Menke wrote:
> Think last_update_time would be more accurate.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:151: };
On 2012/08/15 18:45:41, Matt Menke wrote:
> Inner classes/structs go before constructors.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:153: PerRequestStatusData
per_status_stats[REQUEST_STATUS_MAX];
On 2012/08/15 18:45:41, Matt Menke wrote:
> Your variable name and struct names should be more consistent.  I think that
> PerStatusData or PerStatusStats is clearest.  "PerRequestStatusData" sounds a
> bit like it's per request, which confused me at first.  May want to remove the
> "Per" as well.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:208: RequestStatus status =
static_cast<RequestStatus>(status_int);
On 2012/08/15 18:45:41, Matt Menke wrote:
> It looks to me like we don't need this.  Can just rename status_int to status
> (Or maybe, request_status), and use it directly.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:212: HistogramType type =
static_cast<HistogramType>(type_int);
On 2012/08/15 18:45:41, Matt Menke wrote:
> Same as above.  I'd also suggest using histogram_type rather than type, as
it's
> a little too ambiguous.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:304: RequestStatus status =
static_cast<RequestStatus>(status_int);
On 2012/08/15 18:45:41, Matt Menke wrote:
> Can just use status again.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:310: status_stats->num_active--;
On 2012/08/15 18:45:41, Matt Menke wrote:
> Why not just mess with the stats for old_status and new_status?

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:322: stats->active_requests[&request] =
new_status;
On 2012/08/15 18:45:41, Matt Menke wrote:
> This could be put in the above if statement, right?

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.cc:333: RequestStatus status =
static_cast<RequestStatus>(status_int);
On 2012/08/15 18:45:41, Matt Menke wrote:
> Again, don't need status_int and status.

Done.

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
File chrome/browser/net/cache_stats.h (right):

https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/...
chrome/browser/net/cache_stats.h:69: };
Yes, they are needed for the declaration of histograms_ below.

[rvargas (doing something else), 2012-08-15 22:36:49]

I should have sent this yesterday. I'm not done with cache_stats.cc

http://codereview.chromium.org/10834313/diff/1/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10834313/diff/1/net/base/network_delegate.h#ne...
net/base/network_delegate.h:231: // CACHE_WAIT_STATE_RESET indicates for a given
URLRequest
stale

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData {
This should be a class, and it needs a description.

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:47: // This class collects UMA stats about
cache performance.
This is now stale. And the class name is outdated also... and the file name.

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:155: callback_.Reset();
We should reset the delegate at this point, and add a unit test that verifies
that. (delete the transaction and verify that the delegate is not called).

[tburkard, 2012-08-15 23:00:47]

http://codereview.chromium.org/10834313/diff/1/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10834313/diff/1/net/base/network_delegate.h#ne...
net/base/network_delegate.h:231: // CACHE_WAIT_STATE_RESET indicates for a given
URLRequest
On 2012/08/15 22:36:49, rvargas wrote:
> stale

Done.

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData {
Description has been added.  This only contains POD, along with two helper
methods.  I think struct is appropriate, to avoid having to write excess code
for accessors/modifiers for each of the data elements.

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:47: // This class collects UMA stats about
cache performance.
What's a good new name?  CacheNetworkStats?  PageCacheNetworkStats? 
LoadTimeStats?

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:155: callback_.Reset();
Done.  The test is already verifying that the exact (correct) # of callbacks
happens, and no additional ones after the destruction.

[rvargas (doing something else), 2012-08-16 01:18:40]

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData {
On 2012/08/15 23:00:47, tburkard wrote:
> Description has been added.  This only contains POD, along with two helper
> methods.  I think struct is appropriate, to avoid having to write excess code
> for accessors/modifiers for each of the data elements.

The type of members is actually not important when going for a class vs struct
(but it may weight the result towards a class). For everything but trivial
collections of data classes should be used. This one already quacks :)

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:47: // This class collects UMA stats about
cache performance.
On 2012/08/15 23:00:47, tburkard wrote:
> What's a good new name?  CacheNetworkStats?  PageCacheNetworkStats? 
> LoadTimeStats?

LoadTimeStats or NetStats sound good to me.

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:235: 0, 10000, 500,
base::Histogram::kUmaTargetedHistogramFlag));
What I really want is still a linear, 100 bucket histogram where the denominator
is not the wall time but the total cumulative time.

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:284: old_status == REQUEST_STATUS_ACTIVE);
I don't think we need both of these states.

The way I see it, a request is in the NONE state before we see it (so nothing to
do about it), and after the transaction finishes or it is restarted.

The restart case is interesting because the request itself is still alive and we
are doing work for it (creating a new job etc), so it should really be marked as
active at that time.

I guess it's basically the same thing, except that in one case there's a very
small under-counting of the ACTIVE number (don't really care about it), and we
need to track another state. So it's not that big of a deal.

http://codereview.chromium.org/10834313/diff/10030/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10834313/diff/10030/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:65: explicit TestHttpTransactionDelegate(int
num_cache_actions_to_observe,
not explicit

http://codereview.chromium.org/10834313/diff/10030/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:5062: }
There is no test for request cancellation. That was a bug released to the field
so it deserves a regression test.

[tburkard, 2012-08-16 22:37:43]

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData {
On 2012/08/16 01:18:40, rvargas wrote:
> On 2012/08/15 23:00:47, tburkard wrote:
> > Description has been added.  This only contains POD, along with two helper
> > methods.  I think struct is appropriate, to avoid having to write excess
code
> > for accessors/modifiers for each of the data elements.
> 
> The type of members is actually not important when going for a class vs struct
> (but it may weight the result towards a class). For everything but trivial
> collections of data classes should be used. This one already quacks :)

Done.

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:47: // This class collects UMA stats about
cache performance.
On 2012/08/16 01:18:40, rvargas wrote:
> On 2012/08/15 23:00:47, tburkard wrote:
> > What's a good new name?  CacheNetworkStats?  PageCacheNetworkStats? 
> > LoadTimeStats?
> 
> LoadTimeStats or NetStats sound good to me.

Done.

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:235: 0, 10000, 500,
base::Histogram::kUmaTargetedHistogramFlag));
I'm not sure I understand -- you want absolute times?  But that'll be hard to
compare then, because the data points will all belong to varying page load
times, making them hardly comparable?  That's why I find percentages more
meaningful in this case.

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:284: old_status == REQUEST_STATUS_ACTIVE);
I think it works fine the way it is.  In the case of a restart that you mention
-- I verified the code, the restart happens instantly, so there is no pending
callback or anything like that lost (just a few instructions).  I'll leave it
the way it is.

http://codereview.chromium.org/10834313/diff/10030/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10834313/diff/10030/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:65: explicit TestHttpTransactionDelegate(int
num_cache_actions_to_observe,
On 2012/08/16 01:18:40, rvargas wrote:
> not explicit

Done.

http://codereview.chromium.org/10834313/diff/10030/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:5062: }
On 2012/08/16 01:18:40, rvargas wrote:
> There is no test for request cancellation. That was a bug released to the
field
> so it deserves a regression test.

Done.

[rvargas (doing something else), 2012-08-17 02:36:36]

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:235: 0, 10000, 500,
base::Histogram::kUmaTargetedHistogramFlag));
On 2012/08/16 22:37:43, tburkard wrote:
> I'm not sure I understand -- you want absolute times?  But that'll be hard to
> compare then, because the data points will all belong to varying page load
> times, making them hardly comparable?  That's why I find percentages more
> meaningful in this case.

No, I don't want absolute times.

What I want is still the percentage of the cumulative time that is used in each
part. So for example, if the elapsed time is 2 seconds, and we tracked 10
requests, the total cumulative time would be something close to 20 seconds
(maybe 17). If the cumulative cache time is 5 seconds I want a histogram to say
29% (5/17) not 250% (5/2). I have no idea how to compare 1500% numbers, and the
histogram itself is more complex.

[tburkard, 2012-08-17 20:21:00]

Matt: ping, do you any more concerns?  if not, please lgtm.

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:235: 0, 10000, 500,
base::Histogram::kUmaTargetedHistogramFlag));
I think what you are talking about is just what the third histogram,
CUMULATIVE_PERCENTAGE, expresses:
For all requests, it takes the total time (including duplication) used for
cache, network, other stuff while requests were active, adds them, and takes the
ratio of each cache, network, and other stuff to that total time.  Isn't that
what you are asking for?

[mmenke, 2012-08-17 20:34:42]

Sorry, was waiting for you to finish responding to ricardo's comments.  I'll
carefully go over it tonight / tomorrow morning, and either sign off on it, or
be ready to sign off on it once you address any more comments I may have.

On 2012/08/17 20:21:00, tburkard wrote:
> Matt: ping, do you any more concerns?  if not, please lgtm.
> 
>
http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
> File chrome/browser/net/cache_stats.cc (right):
> 
>
http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
> chrome/browser/net/cache_stats.cc:235: 0, 10000, 500,
> base::Histogram::kUmaTargetedHistogramFlag));
> I think what you are talking about is just what the third histogram,
> CUMULATIVE_PERCENTAGE, expresses:
> For all requests, it takes the total time (including duplication) used for
> cache, network, other stuff while requests were active, adds them, and takes
the
> ratio of each cache, network, and other stuff to that total time.  Isn't that
> what you are asking for?

[rvargas (doing something else), 2012-08-17 22:05:00]

Did something else actually changed between 6 and 7, other than
http_cache_unittest?

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:235: 0, 10000, 500,
base::Histogram::kUmaTargetedHistogramFlag));
On 2012/08/17 20:21:00, tburkard wrote:
> I think what you are talking about is just what the third histogram,
> CUMULATIVE_PERCENTAGE, expresses:
> For all requests, it takes the total time (including duplication) used for
> cache, network, other stuff while requests were active, adds them, and takes
the
> ratio of each cache, network, and other stuff to that total time.  Isn't that
> what you are asking for?

So we have 6 types of histograms, and four(*) of those types are actually 500
bucket, log scaled histograms from 0 to 10k?. That doesn't sound right to me.
They don't look really useful to me. (and I had to go back to this line when
reading how values to be placed on the histograms were computed, I guess I
actually didn't reach the last two).

I actually have an issue with writing 500 bucket histograms.

(*) yes two, not four. Please invert the logic and use x == a || x == b if we
end up leaving this type of histogram. The logic is actually wrong because in
the xx_percentage case is going to the else.

(**) The second reason I got confused here is that when I reviewed the code last
time (previous CL), you were using base::Histogram::kNoFlags for all the
intermediate ones, and I missed when that changed to report everything... so at
first this appeared to be the same split of reported vs not reported.

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:425: fraction_percentage = 100 *
Can we just eliminate these 60 histograms?

http://codereview.chromium.org/10834313/diff/6008/net/http/http_cache_unittes...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10834313/diff/6008/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:3971: delegate->OnNetworkActionFinish();
It's better to have a way to explicitly check that the number of callbacks
doesn't change across c->trans.reset():

delegate->MyPrivateMethod() == x

[tburkard, 2012-08-17 22:10:43]

What changed from 6 to 7 besides the cache unit test:
- remove the explicit from the constructor
- struct -> class
- rename from cache_stats to load_time_stats
all per your request.

[tburkard, 2012-08-17 22:39:55]

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:235: 0, 10000, 500,
base::Histogram::kUmaTargetedHistogramFlag));
First, there was an error in the code, I had intended to make 500 bucket
histograms for the non-percentage histograms (CUMULATIVE, not
CUMULATIVE_PERCENTAGE, as the code incorrectly stated).  However, thinking about
this some more, I agree with you that these are not all that useful, so I
removed them altogether, so there are no more 500 bucket histograms now.

http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:425: fraction_percentage = 100 *
On 2012/08/17 22:05:00, rvargas wrote:
> Can we just eliminate these 60 histograms?

Done.

http://codereview.chromium.org/10834313/diff/6008/net/http/http_cache_unittes...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10834313/diff/6008/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:3971: delegate->OnNetworkActionFinish();
On 2012/08/17 22:05:00, rvargas wrote:
> It's better to have a way to explicitly check that the number of callbacks
> doesn't change across c->trans.reset():
> 
> delegate->MyPrivateMethod() == x

Done.

[rvargas (doing something else), 2012-08-18 02:01:01]

lgtm

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
File chrome/browser/net/load_time_stats.cc (right):

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
chrome/browser/net/load_time_stats.cc:334:
status_stats->set_num_active(status_stats->num_active() - 1);
nit: this (and the previous) line should be a dedicated method

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
chrome/browser/net/load_time_stats.cc:342:
status_stats->set_num_active(status_stats->num_active() + 1);
nit: IncrementFoo()

[mmenke, 2012-08-20 16:15:44]

LGTM.  Sorry I didn't get to this earlier.  I should know by now that I never
get around to doing codereviews at home anymore.

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
File chrome/browser/net/load_time_stats.cc (right):

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
chrome/browser/net/load_time_stats.cc:116: void UpdateTotalTimes() {
Suggest a DCHECK_GE(num_active_, 0)

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
chrome/browser/net/load_time_stats.cc:413: RequestStatus status =
static_cast<RequestStatus>(status_int);
Don't think you need status and status_int in either of these two loops.

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:101: virtual void OnNetworkActionFinish() {
nit:  All these virtual functions should be OVERRIDE.

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:143: num_network_delegate_actions !=
kNoDelegateTransactionCheck) {
I suggest making it fail here when only one is kNoDelegateTransactionCheck,
since it's not supported, though suppose we'll have an EXPECT failure later,
anyways.

[tburkard, 2012-08-20 19:23:38]

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
File chrome/browser/net/load_time_stats.cc (right):

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
chrome/browser/net/load_time_stats.cc:334:
status_stats->set_num_active(status_stats->num_active() - 1);
On 2012/08/18 02:01:02, rvargas wrote:
> nit: this (and the previous) line should be a dedicated method

Done.

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
chrome/browser/net/load_time_stats.cc:342:
status_stats->set_num_active(status_stats->num_active() + 1);
On 2012/08/18 02:01:02, rvargas wrote:
> nit: IncrementFoo()

Done.

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
File chrome/browser/net/load_time_stats.cc (right):

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
chrome/browser/net/load_time_stats.cc:116: void UpdateTotalTimes() {
On 2012/08/20 16:15:45, Matt Menke wrote:
> Suggest a DCHECK_GE(num_active_, 0)

Done.

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
chrome/browser/net/load_time_stats.cc:413: RequestStatus status =
static_cast<RequestStatus>(status_int);
On 2012/08/20 16:15:45, Matt Menke wrote:
> Don't think you need status and status_int in either of these two loops.

Done.

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:101: virtual void OnNetworkActionFinish() {
On 2012/08/20 16:15:45, Matt Menke wrote:
> nit:  All these virtual functions should be OVERRIDE.

Done.

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:143: num_network_delegate_actions !=
kNoDelegateTransactionCheck) {
Choosing to keep it the way it is.