Introduce UMA metrics for path MTU discovery.

Introduce UMA metrics for path MTU discovery.

R=rch@chromium.org

Committed: https://crrev.com/852748e35a26646b62cbf6cb985205c384fc2eb1
Cr-Commit-Position: refs/heads/master@{#338938}

[Ryan Hamilton, 2015-07-14 16:37:12]

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

[Ryan Hamilton, 2015-07-14 16:37:12]

lgtm. You'll need to get asvitkine to approve the histograms change.

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_connectio...
net/quic/quic_connection.h:446: size_t mtu_probe_count() const { return
mtu_probe_count_; }
You'll want to merge this change back into the internal version, too.

[Alexei Svitkine (slow), 2015-07-14 16:55:10]

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:314: connection()->mtu_probe_count());
Sparse histograms are generally not a good choice for numeric histograms.

Sparse histograms will allocate a single bucket for each unique value, which is
inefficient when you're logging numbers. It's better to use one of the COUNTS
macros, which generates a set of logarithmically increasing buckets (i.e. where
there's lots of resolution at the low range, but greater intervals at the high
range.)

[Victor Vasiliev (old), 2015-07-14 18:31:07]

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:314: connection()->mtu_probe_count());
On 2015/07/14 16:55:10, Alexei Svitkine wrote:
> Sparse histograms are generally not a good choice for numeric histograms.
> 
> Sparse histograms will allocate a single bucket for each unique value, which
is
> inefficient when you're logging numbers. It's better to use one of the COUNTS
> macros, which generates a set of logarithmically increasing buckets (i.e.
where
> there's lots of resolution at the low range, but greater intervals at the high
> range.)

Well, the thing about histograms 1 and 2 is that we want them to be exact values
(logarithmic bucketing does not work quite well), but the list of values itself
is fairly limited (we currently have four, but the list of values used is
subject to change).

I've changed field 3 to COUNTS macro.

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_connectio...
net/quic/quic_connection.h:446: size_t mtu_probe_count() const { return
mtu_probe_count_; }
On 2015/07/14 16:37:12, Ryan Hamilton wrote:
> You'll want to merge this change back into the internal version, too.

Will do.

[Alexei Svitkine (slow), 2015-07-14 18:32:54]

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:314: connection()->mtu_probe_count());
On 2015/07/14 18:31:07, Victor Vasiliev wrote:
> On 2015/07/14 16:55:10, Alexei Svitkine wrote:
> > Sparse histograms are generally not a good choice for numeric histograms.
> > 
> > Sparse histograms will allocate a single bucket for each unique value, which
> is
> > inefficient when you're logging numbers. It's better to use one of the
COUNTS
> > macros, which generates a set of logarithmically increasing buckets (i.e.
> where
> > there's lots of resolution at the low range, but greater intervals at the
high
> > range.)
> 
> Well, the thing about histograms 1 and 2 is that we want them to be exact
values
> (logarithmic bucketing does not work quite well), but the list of values
itself
> is fairly limited (we currently have four, but the list of values used is
> subject to change).
> 
> I've changed field 3 to COUNTS macro.

I see. Can you add a comment above the two sparse histograms with the above
reasoning, so that it's clear in the code why the sparse macros are used?

[Victor Vasiliev (old), 2015-07-14 18:52:31]

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1226083005/diff/20001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:314: connection()->mtu_probe_count());
On 2015/07/14 18:32:54, Alexei Svitkine wrote:
> On 2015/07/14 18:31:07, Victor Vasiliev wrote:
> > On 2015/07/14 16:55:10, Alexei Svitkine wrote:
> > > Sparse histograms are generally not a good choice for numeric histograms.
> > > 
> > > Sparse histograms will allocate a single bucket for each unique value,
which
> > is
> > > inefficient when you're logging numbers. It's better to use one of the
> COUNTS
> > > macros, which generates a set of logarithmically increasing buckets (i.e.
> > where
> > > there's lots of resolution at the low range, but greater intervals at the
> high
> > > range.)
> > 
> > Well, the thing about histograms 1 and 2 is that we want them to be exact
> values
> > (logarithmic bucketing does not work quite well), but the list of values
> itself
> > is fairly limited (we currently have four, but the list of values used is
> > subject to change).
> > 
> > I've changed field 3 to COUNTS macro.
> 
> I see. Can you add a comment above the two sparse histograms with the above
> reasoning, so that it's clear in the code why the sparse macros are used?

Done.

[Alexei Svitkine (slow), 2015-07-14 18:54:45]

lgtm % nits

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22088: +    The last MTU used by the
client before the session is closed.
Nit: Define what MTU stands for.

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22347: +    The number of MTU probes
sent by the client during the session.
Nit: Mention when this is logged.

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22508: +    The largest packet which the
client received from the server during the
Nit: Define what MTU stands for.

[Victor Vasiliev (old), 2015-07-14 19:08:09]

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22088: +    The last MTU used by the
client before the session is closed.
On 2015/07/14 18:54:45, Alexei Svitkine wrote:
> Nit: Define what MTU stands for.

Done.

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22347: +    The number of MTU probes
sent by the client during the session.
On 2015/07/14 18:54:45, Alexei Svitkine wrote:
> Nit: Mention when this is logged.

Done.

https://codereview.chromium.org/1226083005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22508: +    The largest packet which the
client received from the server during the
On 2015/07/14 18:54:45, Alexei Svitkine wrote:
> Nit: Define what MTU stands for.

Wait, MTU is not actually mentioned in this summary.

[Victor Vasiliev (old), 2015-07-15 18:29:21]

The CQ bit was checked by vasilvv@google.com

[Victor Vasiliev (old), 2015-07-15 18:29:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1226083005/#ps40002
(title: "Clarify the descriptions of UMA metrics introduced.")

[commit-bot: I haz the power, 2015-07-15 18:30:51]

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

[commit-bot: I haz the power, 2015-07-15 20:34:14]

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

[commit-bot: I haz the power, 2015-07-15 20:34:15]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Victor Vasiliev (old), 2015-07-15 21:44:51]

The CQ bit was checked by vasilvv@google.com

[commit-bot: I haz the power, 2015-07-15 21:47:16]

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

[commit-bot: I haz the power, 2015-07-15 22:28:37]

Committed patchset #5 (id:40002)

[commit-bot: I haz the power, 2015-07-15 22:30:58]

Patchset 5 (id:??) landed as
https://crrev.com/852748e35a26646b62cbf6cb985205c384fc2eb1
Cr-Commit-Position: refs/heads/master@{#338938}