Closed
Bug 1296288
Opened 5 years ago
Closed 5 years ago
Telemetry for TLS 0RTT usage
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: dragana, Assigned: dragana)
References
Details
(Whiteboard: [necko-active])
Attachments
(1 file, 1 obsolete file)
8.11 KB,
patch
|
dragana
:
review+
francois
:
feedback-
|
Details | Diff | Splinter Review |
Add telemetry for tls1.3 early-data.
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Assignee | ||
Comment 1•5 years ago
|
||
Attachment #8788222 -
Flags: review?(hurley)
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.
Attachment #8788222 -
Flags: review?(hurley) → feedback+
Assignee | ||
Comment 3•5 years ago
|
||
(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 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!
Attachment #8788222 -
Flags: review+
Assignee | ||
Comment 5•5 years ago
|
||
Attachment #8788222 -
Attachment is obsolete: true
Attachment #8790631 -
Flags: review+
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fb2f70176a Add telemetry for TLS early-data. r=mcmanus
Keywords: checkin-needed
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7fb2f70176a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 8•5 years ago
|
||
It looks like this didn't get data review, which is required for all new data collection: https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(dd.mozilla)
Assignee | ||
Comment 9•5 years ago
|
||
There is no server that implements TLS Early Data, so we were not collecting any data till now. I will flag for a feedback.
Flags: needinfo?(dd.mozilla)
Assignee | ||
Comment 10•5 years ago
|
||
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!
Attachment #8790631 -
Flags: feedback?(francois)
Comment 11•5 years ago
|
||
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."
Attachment #8790631 -
Flags: feedback?(francois) → feedback-
Assignee | ||
Comment 12•5 years ago
|
||
Thanks for the feedback. I will open another bug to fix you comments, since this is confusing, i.e. what landed and what not.
You need to log in
before you can comment on or make changes to this bug.
Description
•