Closed Bug 805457 Opened 7 years ago Closed 7 years ago

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


(Core :: Networking: WebSockets, defect)

16 Branch
Not set





(Reporter: mcmanus, Assigned: mcmanus)



(1 file)

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.
Attached patch patch 0Splinter Review
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.
Attachment #675189 - Flags: review?(jduell.mcbugs)
Comment on attachment 675189 [details] [diff] [review]
patch 0

>+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.
Attachment #675189 - Flags: review?(jduell.mcbugs) → review+
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.