1296288 - Telemetry for TLS 0RTT usage

Status: RESOLVED FIXED

Description

[Dragana Damjanovic [:dragana]]

Add telemetry for tls1.3 early-data.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1296288.patch

Comment 2

[u408661]

Comment on attachment 8788222 [details] [diff] [review]
bug_1296288.patch

Review of attachment 8788222 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good to me, but before r+ing, I'd like to understand better what's going on - especially in the 'goto' case I comment on below.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +12,5 @@
>  #define LOG(args) LOG5(args)
>  #undef LOG_ENABLED
>  #define LOG_ENABLED() LOG5_ENABLED()
>  
> +#define TLS_EARLY_DATA_NO_AVAILABLE 0

nit: TLS_EARLY_DATA_NOT_AVAILABLE sounds better

@@ +402,4 @@
>              if (NS_FAILED(rv) ||
>                  NS_FAILED(mTransaction->Finish0RTT(!ealyDataAccepted))) {
>                  mTransaction->Close(NS_ERROR_NET_RESET);
>                  goto npnComplete;

Hrm, could be wrong here, but it looks like we might end up skipping telemetry for some cases. Not sure if it's useful to have telemetry then or not.

@@ +422,5 @@
> +                Telemetry::Accumulate(Telemetry::TLS_EARLY_DATA_BYTES_WRITTEN,
> +                                      mContentBytesWritten0RTT);
> +            }
> +        }
> +        mWaitingFor0RTTResponse = false;

This line seems extra (or at least unrelated to the telemetry) since it's changing a value that existed before this patch.

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #2)
> Comment on attachment 8788222 [details] [diff] [review]
> bug_1296288.patch
> 
> Review of attachment 8788222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good to me, but before r+ing, I'd like to understand
> better what's going on - especially in the 'goto' case I comment on below.
> 
> ::: netwerk/protocol/http/nsHttpConnection.cpp
> @@ +12,5 @@
> >  #define LOG(args) LOG5(args)
> >  #undef LOG_ENABLED
> >  #define LOG_ENABLED() LOG5_ENABLED()
> >  
> > +#define TLS_EARLY_DATA_NO_AVAILABLE 0
> 
> nit: TLS_EARLY_DATA_NOT_AVAILABLE sounds better
> 
> @@ +402,4 @@
> >              if (NS_FAILED(rv) ||
> >                  NS_FAILED(mTransaction->Finish0RTT(!ealyDataAccepted))) {
> >                  mTransaction->Close(NS_ERROR_NET_RESET);
> >                  goto npnComplete;
> 
> Hrm, could be wrong here, but it looks like we might end up skipping
> telemetry for some cases. Not sure if it's useful to have telemetry then or
> not.

This should never happen. It can happen only if mRequestStream (from nsHttpTransaction) is not seekable, but that should not happen and we are in bigger trouble.

I am not collecting telemetry if something breaks. If 0rtt breaks connections, because of tls error on the other. At the end we will find out about that in a different way.

> 
> @@ +422,5 @@
> > +                Telemetry::Accumulate(Telemetry::TLS_EARLY_DATA_BYTES_WRITTEN,
> > +                                      mContentBytesWritten0RTT);
> > +            }
> > +        }
> > +        mWaitingFor0RTTResponse = false;
> 
> This line seems extra (or at least unrelated to the telemetry) since it's
> changing a value that existed before this patch.

I just moved it down so that I can use mWaitingFor0RTTResponse info for telemetry.

Comment 4

[u408661]

Comment on attachment 8788222 [details] [diff] [review]
bug_1296288.patch

Review of attachment 8788222 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dragana Damjanovic [:dragana] from comment #3)
> (In reply to Nicholas Hurley [:nwgh][:hurley] from comment #2)
> > Comment on attachment 8788222 [details] [diff] [review]
> > bug_1296288.patch
> > 
> > Review of attachment 8788222 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks pretty good to me, but before r+ing, I'd like to understand
> > better what's going on - especially in the 'goto' case I comment on below.
> > 
> > ::: netwerk/protocol/http/nsHttpConnection.cpp
> > @@ +12,5 @@
> > >  #define LOG(args) LOG5(args)
> > >  #undef LOG_ENABLED
> > >  #define LOG_ENABLED() LOG5_ENABLED()
> > >  
> > > +#define TLS_EARLY_DATA_NO_AVAILABLE 0
> > 
> > nit: TLS_EARLY_DATA_NOT_AVAILABLE sounds better
> > 
> > @@ +402,4 @@
> > >              if (NS_FAILED(rv) ||
> > >                  NS_FAILED(mTransaction->Finish0RTT(!ealyDataAccepted))) {
> > >                  mTransaction->Close(NS_ERROR_NET_RESET);
> > >                  goto npnComplete;
> > 
> > Hrm, could be wrong here, but it looks like we might end up skipping
> > telemetry for some cases. Not sure if it's useful to have telemetry then or
> > not.
> 
> This should never happen. It can happen only if mRequestStream (from
> nsHttpTransaction) is not seekable, but that should not happen and we are in
> bigger trouble.
> 
> I am not collecting telemetry if something breaks. If 0rtt breaks
> connections, because of tls error on the other. At the end we will find out
> about that in a different way.

Sounds good!

> > 
> > @@ +422,5 @@
> > > +                Telemetry::Accumulate(Telemetry::TLS_EARLY_DATA_BYTES_WRITTEN,
> > > +                                      mContentBytesWritten0RTT);
> > > +            }
> > > +        }
> > > +        mWaitingFor0RTTResponse = false;
> > 
> > This line seems extra (or at least unrelated to the telemetry) since it's
> > changing a value that existed before this patch.
> 
> I just moved it down so that I can use mWaitingFor0RTTResponse info for
> telemetry.

Ah, totally missed that deletion. Looks good to me!

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1296288.patch

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fb2f70176a
Add telemetry for TLS early-data. r=mcmanus

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7fb2f70176a

Comment 8

[Georg Fritzsche [:gfritzsche]]

It looks like this didn't get data review, which is required for all new data collection:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 9

[Dragana Damjanovic [:dragana]]

There is no server that implements TLS Early Data, so we were not collecting any data till now. I will flag for a feedback.

Comment 10

[Dragana Damjanovic [:dragana]]

Comment on attachment 8790631 [details] [diff] [review]
bug_1296288.patch

Can you please take a look at Telemetry data collected. It is telemetry of use of TLS Early Data (0RTT). Thanks!

Comment 11

[François Marier [:francois]]

Comment on attachment 8790631 [details] [diff] [review]
bug_1296288.patch

Review of attachment 8790631 [details] [diff] [review]:
-----------------------------------------------------------------

These telemetry probes appear to be exploratory and our usual guidelines for this is to make it expire in 6 releases (58).

If it's still useful and being looked at, it's no problem to extend it before it expires.

::: toolkit/components/telemetry/Histograms.json
@@ +1719,5 @@
> +  "TLS_EARLY_DATA_NEGOTIATED": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 3,
> +    "description": "TLS early data was available: 0 - not available, 1 - avaialable but not used, 2 - available and used.",

nit: typo ("avaialable")

Also, by "available", do you mean that it's being advertised by the server or is that a client-side thing?

@@ +1726,5 @@
> +  },
> +  "TLS_EARLY_DATA_ACCEPTED": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "TLS early data was used and it was accepted or rejected by the remote host.",

nit: "TLS early data was used and it was accepted (true) or rejected (false) by the remote host." would be a little easier to read.

@@ +1735,5 @@
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": 60000,
> +    "n_buckets": 100,
> +    "description": "Amount of bytes sent using TLS early data.",

Could you please clarify which bytes we are counting?

I suspect it's something like: "Amount of bytes sent using TLS early data at the start of a TLS connection for a given channel."

As opposed to: "Amount of bytes sent using TLS early data per browser session."

Comment 12

[Dragana Damjanovic [:dragana]]

Thanks for the feedback. I will open another bug to fix you comments, since this is confusing, i.e. what landed and what not.