Closed
Bug 805457
Opened 13 years ago
Closed 13 years ago
Add Telemetry on Proxy use and cross ref that against websocket handshake success
Categories
(Core :: Networking: WebSockets, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: mcmanus, Assigned: mcmanus)
Details
Attachments
(1 file)
14.52 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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.
Attachment #675189 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 3•13 years ago
|
||
Comment 4•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 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.
Description
•