Introduce new SPDY Version UMA histogram.

Introduce new SPDY Version UMA histogram.

The Net.SpdyVersion UMA histogram directly tracks internal NextProto enum
values, which change, for example, when protocols are deprecated, e.g.,
https://chromium.googlesource.com/chromium/src/+/13621d68bc4096987471ec698468a972af2fa1de%5E%21/#F9.

This CL obsoletes Net.SpdyVersion, and introduces Net.SpdyVersion2, which
uses values that are specifically generated for this purpuse, with the intent
that later versions will not change or reuse them.

BUG=412495
Committed: https://crrev.com/f1c950543990d7048e2323c106a11350fe95ab15
Cr-Commit-Position: refs/heads/master@{#295053}

[Bence, 2014-09-10 13:14:20]

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

[Alexei Svitkine (slow), 2014-09-10 14:55:16]

It's unfortunate that we ended up with this mismatch. Can we make it so
it's not possible for it to change in the future? (Either by adding a
comment to the enum that disallows changing it in incompatible ways or by
using a different enum and converting).

Also, please modify the descriptions of the histograms that use this to
mention when the meaning changed.

On Wed, Sep 10, 2014 at 9:14 AM, <bnc@chromium.org> wrote:

> Reviewers: Alexei Svitkine,
>
> Description:
> Update enum ProtocolVersion to include SPYD4.
>
> Update enum ProtocolVersion in histograms.xml to match enum NextProto in
> next_proto.h, in particular, to include SPYD4.
>
> Note that enum items can change values (for example, adding SPDY4 bumped
> value
> for QUIC), therefore unfortunately one can only correctly interpret
> histograms
> release by release.
>
>
> BUG=412495
>
> Please review this at https://codereview.chromium.org/560573002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+5, -4 lines):
>   M tools/metrics/histograms/histograms.xml
>
>
> Index: tools/metrics/histograms/histograms.xml
> diff --git a/tools/metrics/histograms/histograms.xml
> b/tools/metrics/histograms/histograms.xml
> index 96a571ee1b69bd6287b396cdd7b79b8fc4abaef4..
> 9a0f64d9b705ac73d97f81d277c88595ee7e96e9 100644
> --- a/tools/metrics/histograms/histograms.xml
> +++ b/tools/metrics/histograms/histograms.xml
> @@ -48493,10 +48493,11 @@ To add a new entry, add it with any value and
> run test to compute valid value.
>  <enum name="ProtocolVersion" type="int">
>    <int value="0" label="UNKNOWN"/>
>    <int value="1" label="HTTP 1.1"/>
> -  <int value="2" label="SPDY 1.0"/>
> -  <int value="3" label="SPDY 2.0"/>
> -  <int value="4" label="SPDY 2.1"/>
> -  <int value="5" label="SPDY 3.0"/>
> +  <int value="2" label="SPDY 2.0"/>
> +  <int value="3" label="SPDY 3.0"/>
> +  <int value="4" label="SPDY 3.1"/>
> +  <int value="5" label="SPDY 4.0"/>
> +  <int value="6" label="QUIC 1.0"/>
>  </enum>
>
>  <enum name="ProvisionalSaveFailure" type="int">
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bence, 2014-09-11 17:55:46]

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

[Bence, 2014-09-11 17:55:46]

Alexei:  Thank you for your suggestions.  PTAL.

rch: Please review changes in net.

Note that we only log SPDY connections, not QUIC or HTTP/1.1, see
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess....

[Alexei Svitkine (slow), 2014-09-11 18:29:59]

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h
File net/socket/next_proto.h (right):

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h#...
net/socket/next_proto.h:41: const unsigned int spdyProtoPermanentValueBoundary =
5;
I suggest just making an enum for the PermanentValue version of these values,
then this can be part of the enum and the return value of the function can also
be the enum, so we get type safety (and no chance for someone to accidentally
add a new value but forget to update this).

[Bence, 2014-09-12 12:37:07]

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h
File net/socket/next_proto.h (right):

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h#...
net/socket/next_proto.h:41: const unsigned int spdyProtoPermanentValueBoundary =
5;
You're right, we need a proper enum here.

This is getting very complicated.  I would in fact prefer to make the values in
NextProto permanent, eliminating the need for an extra enum.    NextProto being
noncontiguous only breaks a single for cycle:
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_test....
 I would prefer to replace that by spelling out every protocol, like in
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/next_pr....
 This would also eliminate the need for a translating function, so overall
maintenance would be easier.

[Alexei Svitkine (slow), 2014-09-12 14:38:09]

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h
File net/socket/next_proto.h (right):

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h#...
net/socket/next_proto.h:41: const unsigned int spdyProtoPermanentValueBoundary =
5;
On 2014/09/12 12:37:07, Bence wrote:
> You're right, we need a proper enum here.
> 
> This is getting very complicated.  I would in fact prefer to make the values
in
> NextProto permanent, eliminating the need for an extra enum.    NextProto
being
> noncontiguous only breaks a single for cycle:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_test....
>  I would prefer to replace that by spelling out every protocol, like in
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/next_pr....
>  This would also eliminate the need for a translating function, so overall
> maintenance would be easier.

That SGTM.

[Bence, 2014-09-12 19:54:49]

Patchset #3 (id:40001) has been deleted

[Bence, 2014-09-12 20:16:56]

PTAL.

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h
File net/socket/next_proto.h (right):

https://codereview.chromium.org/560573002/diff/20001/net/socket/next_proto.h#...
net/socket/next_proto.h:41: const unsigned int spdyProtoPermanentValueBoundary =
5;
On 2014/09/12 14:38:08, Alexei Svitkine wrote:
> On 2014/09/12 12:37:07, Bence wrote:
> > You're right, we need a proper enum here.
> > 
> > This is getting very complicated.  I would in fact prefer to make the values
> in
> > NextProto permanent, eliminating the need for an extra enum.    NextProto
> being
> > noncontiguous only breaks a single for cycle:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_test....
> >  I would prefer to replace that by spelling out every protocol, like in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/next_pr....
> >  This would also eliminate the need for a translating function, so overall
> > maintenance would be easier.
> 
> That SGTM.

Done.

[Alexei Svitkine (slow), 2014-09-12 20:23:06]

https://codereview.chromium.org/560573002/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/560573002/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:723: "Net.SpdyVersion2", protocol_,
kProtoSPDYMaximumVersion + 1);
Now that you're expanded the range of the values, please use an
UMA_SPARSE_HISTOGRAM_SLOWLY() here, so that it doesn't pre-allocate 200 buckets.

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

https://codereview.chromium.org/560573002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:49897: +  <int value="103" label="HTTP/2
draft-14"/>
Is it not necessary to include kProtoQUIC1SPDY3 here?

[Bence, 2014-09-12 20:31:13]

PTAL

https://codereview.chromium.org/560573002/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/560573002/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:723: "Net.SpdyVersion2", protocol_,
kProtoSPDYMaximumVersion + 1);
On 2014/09/12 20:23:06, Alexei Svitkine wrote:
> Now that you're expanded the range of the values, please use an
> UMA_SPARSE_HISTOGRAM_SLOWLY() here, so that it doesn't pre-allocate 200
buckets.

Done.

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

https://codereview.chromium.org/560573002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:49897: +  <int value="103" label="HTTP/2
draft-14"/>
On 2014/09/12 20:23:06, Alexei Svitkine wrote:
> Is it not necessary to include kProtoQUIC1SPDY3 here?

No, it is not, because we only upload SPDY protocols, excluding QUIC.  See
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess....

[Alexei Svitkine (slow), 2014-09-12 21:14:30]

https://codereview.chromium.org/560573002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/560573002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:17384: +    The SPDY protocol version
that is used to talk to SPDY servers.
Nit: While you're here, please expand this description to mention when it's
logged. (On startup? On every request?)

[Alexei Svitkine (slow), 2014-09-12 21:14:44]

lgtm % my previous comment, thanks!

[Bence, 2014-09-15 15:39:27]

Ryan: PTAL.
Alexei: thank you.

https://codereview.chromium.org/560573002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/560573002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:17384: +    The SPDY protocol version
that is used to talk to SPDY servers.
On 2014/09/12 21:14:30, Alexei Svitkine wrote:
> Nit: While you're here, please expand this description to mention when it's
> logged. (On startup? On every request?)

Done.

[Alexei Svitkine (slow), 2014-09-15 15:53:38]

LGTM

[Bence, 2014-09-15 21:00:36]

Alexei: PTAL.  Sorry to bother you again.  Since the SPDY block within the enum
is contiguous, and is not expected to grow out of control (a handful of new
HTTP/2 drafts in the worst case, maybe SPDY5), I propose to turn back to a
regular histogram.  It makes more sense than a sparse one, just need to subtract
an offset to make it start from 0.

[Ryan Hamilton, 2014-09-15 21:37:29]

lgtm

[Alexei Svitkine (slow), 2014-09-15 21:41:19]

lgtm

[Bence, 2014-09-16 12:54:05]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-09-16 12:55:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/560573002/120001

[commit-bot: I haz the power, 2014-09-16 13:31:47]

Committed patchset #6 (id:120001) as 1829772cf8e40cc3f8786c467e1f3f101044e8ed

[commit-bot: I haz the power, 2014-09-16 13:33:31]

Patchset 6 (id:??) landed as
https://crrev.com/f1c950543990d7048e2323c106a11350fe95ab15
Cr-Commit-Position: refs/heads/master@{#295053}