Add telemetry for HTTP connections utilization

NEW
Unassigned

Status

()

Core
Networking: HTTP
P5
enhancement
6 years ago
a month ago

People

(Reporter: mayhemer, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [engineering-friend][necko-would-take])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
I noticed we open a significant number of connections that are never used.  Also this could be a basic measure for how many (real) transactions a connection handles.

To sum, with T+S patch in mind, I want to measure:
- How often we create a backup connection (results caused by happy eyeballs should be ignored)
- How often the backup connection was never used
- nsHttpTransaction per nsHttpConnection cumulative utilization (through pipeline objects)
- nsHttpTransaction per nsHttpConnection per class utilization
- transaction dispatch result rates: create a connection / reuse an idle connection / dispatch to a pipeline / queue

The first 3 might be implemented right now.  The rest after the T+S patch lands.

I will start writing a patch based on m-c.  I know if we land this we'll need to merge Larch against it but on the other hand we might have a useful numbers to compare to after pipelining stuff gets in.

Comments, suggestions welcome!

Comment 1

5 years ago
Hey, is this bug still open? I want to work on it.
still open

Comment 3

4 years ago
Hi guys, first telemetry should accumulate in nsHttpConnectionMgr::nsHalfOpenSocket::SetupBackupStreams() ?

Comment 4

4 years ago
or maybe when the timer fires in Notify() ?

Comment 5

4 years ago
Created attachment 785226 [details] [diff] [review]
number of backup connections vs never used ones
Attachment #785226 - Flags: feedback?(honzab.moz)

Comment 6

4 years ago
Created attachment 785232 [details] [diff] [review]
number of backup connections vs never used ones
Attachment #785226 - Attachment is obsolete: true
Attachment #785226 - Flags: feedback?(honzab.moz)
Attachment #785232 - Flags: feedback?(honzab.moz)
(Reporter)

Comment 7

4 years ago
Comment on attachment 785232 [details] [diff] [review]
number of backup connections vs never used ones

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

Thanks for the effort!

This needs a little tune up.  I'd rather have a telemetry like this:

Have an enum of 4 values where:
 0 = a utilized primary connection
 1 = a never used primary connection
 2 = a utilized backup connection
 3 = a never used backup connection

Report this in the connection's destructor only.  

Have an actual enum { UTILIZED_PRIMARY_CONNECTION = 1, NEVER_USED_PRIMARY_CONNECTION = 2, ... } to make clear what you are reporting.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +107,5 @@
>                                Telemetry::HTTP_KBREAD_PER_CONN,
>                                totalKBRead);
>      }
> +
> +    if (this->IsBackupConnection() && !this->GetConnectionEverUsed())

use the member flags directly. also no need for this->.

::: netwerk/protocol/http/nsHttpConnection.h
@@ +112,5 @@
>                             nsIAsyncOutputStream **);
>      void     GetSecurityInfo(nsISupports **);
>      bool     IsPersistent() { return IsKeepAlive(); }
>      bool     IsReused();
> +    bool     GetConnectionEverUsed() { return mConnectionEverUsed; }

You don't need this getter.

@@ +113,5 @@
>      void     GetSecurityInfo(nsISupports **);
>      bool     IsPersistent() { return IsKeepAlive(); }
>      bool     IsReused();
> +    bool     GetConnectionEverUsed() { return mConnectionEverUsed; }
> +    bool     IsBackupConnection() { return mIsBackupConnection; }

This one as well.

::: toolkit/components/telemetry/Histograms.json
@@ +650,5 @@
>      "description": "HTTP: requests per connection"
>    },
> +  "HTTP_BACKUP_CONN": {
> +    "kind": "enumerated",
> +    "n_values": 3,

I see you only report 0 and 1.
Attachment #785232 - Flags: feedback?(honzab.moz) → feedback-

Updated

4 years ago
Whiteboard: [engineering-friend]

Comment 8

4 years ago
Created attachment 788973 [details] [diff] [review]
backup vs primary connections usage v2
Attachment #785232 - Attachment is obsolete: true
Attachment #788973 - Flags: review?(honzab.moz)
(Reporter)

Comment 9

4 years ago
Comment on attachment 788973 [details] [diff] [review]
backup vs primary connections usage v2

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

r=honzab

::: netwerk/protocol/http/nsHttpConnection.h
@@ +282,1 @@
>  };

Just put this enum into the nsHttpConnection class declaration and give it a name (like EUtilizationTelemetry).

::: toolkit/components/telemetry/Histograms.json
@@ +651,5 @@
>    },
> +  "HTTP_CONNECTIONS_USAGE": {
> +    "kind": "enumerated",
> +    "n_values": 4,
> +    "description": "utilized primary connection. never used primary connection, utilized backup connection, never used backup connection"

I think you may add '"extended_statistics_ok": true,'
Attachment #788973 - Flags: review?(honzab.moz) → review+

Comment 10

4 years ago
Thanks, I will resend soon. Until then, could you explain me please what "extended_statistics_ok" does? (the name is suggestive of more detailed data, not enough for me)

Comment 11

4 years ago
All the histograms in Histograms.json with this "extended_statistics_ok" option are exponential, I also get an error adding this ("KeyError: u'extended_statistics_ok not permitted for HTTP_CONNECTIONS_USAGE'"), are you sure it's suitable for this kind of histogram?
(Reporter)

Comment 12

4 years ago
Yep, then just leave the probe as is.  Thanks.

Comment 13

4 years ago
Created attachment 789591 [details] [diff] [review]
backup vs primary connections usage

r=honzab
Attachment #788973 - Attachment is obsolete: true

Comment 14

4 years ago
I want to implement the others too, could you be a little more specific please? Necko is still huge for me, I will do my best to understand as much as I could, therefore some starting points would be great.
(Reporter)

Comment 15

4 years ago
(In reply to Robert Bindar from comment #14)
> I want to implement the others too, 

What do you mean exactly?

> could you be a little more specific
> please? 

On what?

> Necko is still huge for me, I will do my best to understand as much
> as I could, therefore some starting points would be great.

I'll gladly help, just be more specific please :)

Comment 16

4 years ago
There are 3 more subtasks to do as I can see.

> - nsHttpTransaction per nsHttpConnection cumulative utilization (through
> pipeline objects)

I don't understand the "through pipeline objects" part.

> - nsHttpTransaction per nsHttpConnection per class utilization

Per class utilization? What does this mean, how many nsHttpTransactions were attached to a nsHttpConnection?

> - transaction dispatch result rates: create a connection / reuse an idle
> connection / dispatch to a pipeline / queue
 
I didn't get that at all. Can you give me a use-case or something to clarify the task please?
(Reporter)

Comment 17

4 years ago
(In reply to Robert Bindar from comment #16)
> There are 3 more subtasks to do as I can see.
> 
> > - nsHttpTransaction per nsHttpConnection cumulative utilization (through
> > pipeline objects)
> 
> I don't understand the "through pipeline objects" part.

Don't bother right now.  This was for case we allow pipelining by default.  I don't recall now what that means anyway ;)

> 
> > - nsHttpTransaction per nsHttpConnection per class utilization
> 
> Per class utilization? What does this mean, how many nsHttpTransactions were
> attached to a nsHttpConnection?

No. We have so called "class" that classifies a transaction by its type (image/html/css or js/etc).

> 
> > - transaction dispatch result rates: create a connection / reuse an idle
> > connection / dispatch to a pipeline / queue
>  
> I didn't get that at all. Can you give me a use-case or something to clarify
> the task please?

An http transaction is created by an http channel and is then responsible to do the http request and parse the http response.  It goes to the http transaction scheduler where we:
- attempt to request important stuff sooner then less important
- obey per host and global connection limits

So, a transaction on it's first attempt to do the request to the server may end up in one of the following states:
- be dispatched on an idle connection (or added to a pipeline)
- creates a new connection and is dispatched on that new connection
- queued for later since we are on the limits (no idle connection and cannot create new since we would go over the limit)
Whiteboard: [engineering-friend] → [engineering-friend][necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.