805457 - Add Telemetry on Proxy use and cross ref that against websocket handshake success

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

I'm working on the transition mechanism for http/2. One of the interesting things there is the impact of proxies. To further that discussion I'd like to add a little telemetry to get some data on

1] how often we use explicit proxies and what type they are
2] for websockets (because it deals with a transition of a similar nature) what the pass/fail rates are in the presence of proxies and tls.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

This is all pretty straightforward. 2 things to note:

1] changed the name of mRecvdHttpOnStartRequest to be mRecvdSuccessfulHttpOnStartRequest because I'm not relying on the "successful" property it already had. (its value is unchanged in every state).

2] The telemetry enumerated values are defined with slightly more values than they really need (e.g. the proxy type has 3 values right now {none, socks, http} but the telemetry type allows 8) due to a previous bsmith insight that this lets us change the reported values without having to toss out the whole telem data set if we do something like add a new proxy protocol.

Comment 2

[Jason Duell]

Comment on attachment 675189 [details] [diff] [review]
patch 0

>+void
>+nsHttpConnectionMgr::ReportProxyTelemetry(nsConnectionEntry *ent)
>+{
>+    enum { PROXY_NONE = 1, PROXY_HTTP = 2, PROXY_SOCKS = 3 };
>+
>+    if (!ent->mConnInfo->UsingProxy())
>+        Telemetry::Accumulate(Telemetry::HTTP_PROXY_TYPE, PROXY_NONE);
>+    else if (ent->mConnInfo->UsingHttpProxy())
>+        Telemetry::Accumulate(Telemetry::HTTP_PROXY_TYPE, PROXY_HTTP);
>+    else
>+        Telemetry::Accumulate(Telemetry::HTTP_PROXY_TYPE, PROXY_SOCKS);

Seems like this would all be a touch simpler if UsingProxy (renamed GetProxyType()?) just returned the enum--no if..else.   But I'm fine with this too.  

Brace if..else?
 

>-  uint32_t                        mRecvdHttpOnStartRequest   : 1;
>+  uint32_t                        mRecvdSuccessfulHttpOnStartRequest : 1;

I find my eyes glaze over at long variable names.  How about just "mGotOnStartRequestOK" (and mGotUpgradeOK)?  I don't think we need 'http', 'successful' is just sooo long :) and I seem to remember some thread about abbreviations (Recvd) being hard for non-English speakers.  But  really anything shorter than mRecvdSuccessfulHttpOnStartRequest is good with me: I'll leave it to you.

Comment 3

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f9c4a12b50

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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