1352271 - Add telemetry about TCP Fast Open

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1352271_v1.patch

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8855737 [details] [diff] [review]
bug_1352271_v1.patch

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

::: netwerk/protocol/http/nsHttpConnection.h
@@ +230,5 @@
>  
>      bool TestJoinConnection(const nsACString &hostname, int32_t port);
>      bool JoinConnection(const nsACString &hostname, int32_t port);
>  
> +    void FastOpenStatus(uint8_t tfoStatus) {

SetFastOpenStatus.. by convention FastOpenStatus would be a getter

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> Comment on attachment 8855737 [details] [diff] [review]
> bug_1352271_v1.patch
> 
> Review of attachment 8855737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnection.h
> @@ +230,5 @@
> >  
> >      bool TestJoinConnection(const nsACString &hostname, int32_t port);
> >      bool JoinConnection(const nsACString &hostname, int32_t port);
> >  
> > +    void FastOpenStatus(uint8_t tfoStatus) {
> 
> SetFastOpenStatus.. by convention FastOpenStatus would be a getter

I will update it.

Comment 4

[François Marier [:francois]]

Comment on attachment 8855737 [details] [diff] [review]
bug_1352271_v1.patch

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

::: toolkit/components/telemetry/Histograms.json
@@ +2156,5 @@
>    },
> +  "TCP_FAST_OPEN": {
> +    "expires_in_version": "61",
> +    "kind": "enumerated",
> +    "n_values": 4,

I would suggest you give yourself some room for future expansion here since we cannot update `n_values` after the fact.

If you set it to a larger number than you need, say 8, then you can later send values of 4-7 if you need to.

If you're sure you won't need more than 4 values, then it's fine to leave it like this.

@@ +2157,5 @@
> +  "TCP_FAST_OPEN": {
> +    "expires_in_version": "61",
> +    "kind": "enumerated",
> +    "n_values": 4,
> +    "description": "TCP Fast Open: 0=TFO_NOT_TRIED, 1=TFO_TRIED_NEGOTIATING, 2=TFO_DATA_SENT, 3=TFO_FAILED.",

Can you please specify when the probe is sent?

Is it something like: "Whenever a new TCP connection is created, track whether or not TCP Fast Open was used (0=..."

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1352271_v1.patch

Comment 6

[François Marier [:francois]]

Comment on attachment 8856965 [details] [diff] [review]
bug_1352271_v1.patch

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

datareview+

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1352271_v1.patch

Rebased.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d68ae6846b2a
Add telemetry about TCP Fast Open. r=mcmanus r=francois

Comment 9

[Carsten Book [:Tomcat]]

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