Closed Bug 1352271 Opened 3 years ago Closed 2 years ago

Add telemetry about TCP Fast Open

Categories

(Core :: Networking: HTTP, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Attached patch bug_1352271_v1.patch (obsolete) — Splinter Review
Attachment #8855737 - Flags: review?(mcmanus)
Attachment #8855737 - Flags: review?(francois)
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
Depends on: 1188435
Depends on: 1352274
(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.
Attachment #8855737 - Flags: review?(mcmanus) → review+
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=..."
Attachment #8855737 - Flags: review?(francois)
Attached patch bug_1352271_v1.patch (obsolete) — Splinter Review
Attachment #8855737 - Attachment is obsolete: true
Attachment #8856965 - Flags: review?(francois)
Comment on attachment 8856965 [details] [diff] [review]
bug_1352271_v1.patch

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

datareview+
Attachment #8856965 - Flags: review?(francois) → review+
Rebased.
Attachment #8856965 - Attachment is obsolete: true
Attachment #8864056 - Flags: review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d68ae6846b2a
Add telemetry about TCP Fast Open. r=mcmanus r=francois
https://hg.mozilla.org/mozilla-central/rev/d68ae6846b2a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.