Remove Network transaction timing histograms.

Remove Network transaction timing histograms.

BUG=429300
Committed: https://crrev.com/9849a4c0fd6144fa63194a0c83b22de8e9f70c38
Cr-Commit-Position: refs/heads/master@{#302389}

[cbentzel, 2014-10-31 20:14:47]

cbentzel@chromium.org changed reviewers:
+ bnc@chromium.org, mmenke@chromium.org

[cbentzel, 2014-10-31 20:14:48]

bnc: Please do the overall review

mmenke: Do you think that some of these would be useful? Nobody claimed them but
I do wonder if we want to retain the time-to-first-byte and
time-to-response-complete measurements. That being said, there may be metrics
for these up the stack (URLRequestHttpJob or loader location) and in some ways
those are better because they capture caching, throttles, and other things.

[Bence, 2014-10-31 20:42:07]

On 2014/10/31 20:14:48, cbentzel wrote:
> bnc: Please do the overall review
> 
> mmenke: Do you think that some of these would be useful? Nobody claimed them
but
> I do wonder if we want to retain the time-to-first-byte and
> time-to-response-complete measurements. That being said, there may be metrics
> for these up the stack (URLRequestHttpJob or loader location) and in some ways
> those are better because they capture caching, throttles, and other things.

LGTM modulo nits.

[Bence, 2014-10-31 20:42:29]

https://codereview.chromium.org/695993003/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/695993003/diff/1/net/http/http_network_transa...
net/http/http_network_transaction.cc:68: using base::TimeDelta;
Please remove these two using directives.  (Seems like they were not used even
before this change.)

https://codereview.chromium.org/695993003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/695993003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:16920: +    Discontinued as of 12/31/14
1. Please consider writing out all digits of the year.
2. Please consider treating this phrase as a sentence and ending it with a full
stop.

(Same for another 7 instances below.)

[mmenke, 2014-10-31 20:51:06]

On 2014/10/31 20:14:48, cbentzel wrote:
> bnc: Please do the overall review
> 
> mmenke: Do you think that some of these would be useful? Nobody claimed them
but
> I do wonder if we want to retain the time-to-first-byte and
> time-to-response-complete measurements. That being said, there may be metrics
> for these up the stack (URLRequestHttpJob or loader location) and in some ways
> those are better because they capture caching, throttles, and other things.

I've never looked at either of these, largely because in a list of 100
histograms, these names have no clear meaning.  I've always used
Net.HttpTimeToFirstByte (Which I've claimed).  Admittedly, that histogram does
sit above the cache layer.

If we want to keep these, we should rename them to things like
Net.HttpTimeFromConnectStartToFirstByte and
Net.HttpTimeFromConnectCompleteToFirstByte, so people can actually use them
without having to figure out what their names are every time, and then after
that, try and figure out what they actually mean.

Back to whether or not we should keep them:  I think there's a such thing as too
much data - skimming over a dozen histograms can be less productive than doing a
deep dive into 2 or 3.  We already have TCP and SSL connection latency, so I'm
not sure that keeping histograms that are their sum (And adds in QUIC latency! 
And proxy tunneling latency!) really gets us a whole lot.  I also haven't been
impressed by our ability to respond usefully to Finch alerts, which further
leads me to question the marginal benefits of more histograms...

That having been said, I think it may be reasonable to keep
Net.HttpTimeFromConnectCompleteToFirstByte (Net.Priority_High_Latency_b?), and
get rid of the others.  It at least allows us to figure out if a regression in
Net.HttpTimeToFirstByte is cache related or not.

[cbentzel, 2014-11-01 10:07:12]

On 2014/10/31 20:51:06, mmenke wrote:
> On 2014/10/31 20:14:48, cbentzel wrote:
> > bnc: Please do the overall review
> > 
> > mmenke: Do you think that some of these would be useful? Nobody claimed them
> but
> > I do wonder if we want to retain the time-to-first-byte and
> > time-to-response-complete measurements. That being said, there may be
metrics
> > for these up the stack (URLRequestHttpJob or loader location) and in some
ways
> > those are better because they capture caching, throttles, and other things.
> 
> I've never looked at either of these, largely because in a list of 100
> histograms, these names have no clear meaning.  I've always used
> Net.HttpTimeToFirstByte (Which I've claimed).  Admittedly, that histogram does
> sit above the cache layer.
> 
> If we want to keep these, we should rename them to things like
> Net.HttpTimeFromConnectStartToFirstByte and
> Net.HttpTimeFromConnectCompleteToFirstByte, so people can actually use them
> without having to figure out what their names are every time, and then after
> that, try and figure out what they actually mean.
> 
> Back to whether or not we should keep them:  I think there's a such thing as
too
> much data - skimming over a dozen histograms can be less productive than doing
a
> deep dive into 2 or 3.  We already have TCP and SSL connection latency, so I'm
> not sure that keeping histograms that are their sum (And adds in QUIC latency!

> And proxy tunneling latency!) really gets us a whole lot.  I also haven't been
> impressed by our ability to respond usefully to Finch alerts, which further
> leads me to question the marginal benefits of more histograms...
> 
> That having been said, I think it may be reasonable to keep
> Net.HttpTimeFromConnectCompleteToFirstByte (Net.Priority_High_Latency_b?), and
> get rid of the others.  It at least allows us to figure out if a regression in
> Net.HttpTimeToFirstByte is cache related or not.

Thanks for the argument. I agree that we can get rid of them and use
Net.HttpTimeToFirstByte as the higher-level metric, with some of the sockets
details for helping diagnose causes of regressions.

[cbentzel, 2014-11-01 10:21:37]

https://codereview.chromium.org/695993003/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/695993003/diff/1/net/http/http_network_transa...
net/http/http_network_transaction.cc:68: using base::TimeDelta;
On 2014/10/31 20:42:29, Bence wrote:
> Please remove these two using directives.  (Seems like they were not used even
> before this change.)

Done.

https://codereview.chromium.org/695993003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/695993003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:16920: +    Discontinued as of 12/31/14
On 2014/10/31 20:42:29, Bence wrote:
> 1. Please consider writing out all digits of the year.
> 2. Please consider treating this phrase as a sentence and ending it with a
full
> stop.
> 
> (Same for another 7 instances below.)

Done, made these more consistent with the deprecation sentences in the rest of
the file.

[I also realized I used the date of 12/31 rather than 10/31, but moved to
11/2014 since date has changed]

[cbentzel, 2014-11-01 10:22:28]

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

[cbentzel, 2014-11-01 10:22:42]

asvitkine: Can you review histograms.xml?

[cbentzel, 2014-11-01 10:22:43]

asvitkine: Can you review histograms.xml?

[Alexei Svitkine (slow), 2014-11-01 14:36:40]

LGTM!

[cbentzel, 2014-11-01 17:14:49]

The CQ bit was checked by cbentzel@chromium.org

[commit-bot: I haz the power, 2014-11-01 17:15:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/695993003/20001

[commit-bot: I haz the power, 2014-11-01 18:11:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-01 18:11:58]

Patchset 2 (id:??) landed as
https://crrev.com/9849a4c0fd6144fa63194a0c83b22de8e9f70c38
Cr-Commit-Position: refs/heads/master@{#302389}