671568 - Add telemetry for HTTP connections utilization

Status: NEW

(Core :: Networking: HTTP, enhancement, P5)

Description

[Honza Bambas (:mayhemer)]

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

[Georgiana [:gia]]

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

Comment 2

[Patrick McManus [:mcmanus]]

still open

Comment 3

[Robert Bindar]

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

Comment 4

[Robert Bindar]

or maybe when the timer fires in Notify() ?

Comment 5

[Robert Bindar]

Attachment: number of backup connections vs never used ones

Comment 6

[Robert Bindar]

Attachment: number of backup connections vs never used ones

Comment 7

[Honza Bambas (:mayhemer)]

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.

Comment 8

[Robert Bindar]

Attachment: backup vs primary connections usage v2

Comment 9

[Honza Bambas (:mayhemer)]

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,'

Comment 10

[Robert Bindar]

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

[Robert Bindar]

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?

Comment 12

[Honza Bambas (:mayhemer)]

Yep, then just leave the probe as is.  Thanks.

Comment 13

[Robert Bindar]

Attachment: backup vs primary connections usage

r=honzab

Comment 14

[Robert Bindar]

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.

Comment 15

[Honza Bambas (:mayhemer)]

(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

[Robert Bindar]

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?

Comment 17

[Honza Bambas (:mayhemer)]

(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)

Comment 18

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258