Closed
Bug 1311893
Opened 8 years ago
Closed 8 years ago
Collect telemetry on timing of HSTS priming requests
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: kmckinley, Assigned: kmckinley)
Details
Attachments
(1 file)
In the presence of HTTPS timeouts, for example when a firewall drops packets to port 443 instead of sending RST, the first priming request times out after 30 seconds before the load can continue. The following telemetry will be added to measure the time of the HSTS priming request, not including the actual load. HSTS_PRIMING_SUCCESS_TIMING - how long successful HSTS priming requests take HSTS_PRIMING_FAILURE_TIMING - how long failed HSTS priming requests take
Assignee | ||
Comment 1•8 years ago
|
||
Change to use a keyed histogram instead of 2 histograms.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8803226 [details] Bug 1311893 - HSTS Priming timing telemetry p=francois Francois, can you take a look at this for privacy / data collection review?
Attachment #8803226 -
Flags: feedback?(francois)
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8803226 [details] Bug 1311893 - HSTS Priming timing telemetry p=francois https://reviewboard.mozilla.org/r/87536/#review86510 lgtm, the nit is totally up to you to incorporate or not. I can't say anything about the low and high buckets though, but I suppose francois will look at that anyway. Code looks good, r=me ::: netwerk/protocol/http/HSTSPrimerListener.cpp:51 (Diff revision 2) > + TimeStamp channelCreationTime; > + nsresult rv = timingChannel->GetChannelCreation(&channelCreationTime); > + if (NS_SUCCEEDED(primingResult) && !channelCreationTime.IsNull()) { > + PRUint32 interval = (PRUint32) > + (TimeStamp::Now() - channelCreationTime) > + .ToMilliseconds(); nit: I would prefer PRUint32 interval = (PRUint32)(TimeStamp::Now() - channelCreationTime).ToMilliseconds();
Attachment #8803226 -
Flags: review?(ckerschb) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8803226 -
Flags: feedback?(francois)
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8803226 [details] Bug 1311893 - HSTS Priming timing telemetry p=francois https://reviewboard.mozilla.org/r/87536/#review87376 ::: netwerk/protocol/http/HSTSPrimerListener.cpp:48 (Diff revision 2) > - if (NS_FAILED(rv)) { > + nsCOMPtr<nsITimedChannel> timingChannel = > + do_QueryInterface(callback); > + if (timingChannel) { > + TimeStamp channelCreationTime; > + nsresult rv = timingChannel->GetChannelCreation(&channelCreationTime); > + if (NS_SUCCEEDED(primingResult) && !channelCreationTime.IsNull()) { Shouldn't we check for `NS_SUCCEEDED(rv)` here? ::: netwerk/protocol/http/HSTSPrimerListener.cpp:53 (Diff revision 2) > + if (NS_SUCCEEDED(primingResult) && !channelCreationTime.IsNull()) { > + PRUint32 interval = (PRUint32) > + (TimeStamp::Now() - channelCreationTime) > + .ToMilliseconds(); > + Telemetry::Accumulate(Telemetry::HSTS_PRIMING_REQUEST_DURATION, > + (NS_SUCCEEDED(rv)) ? NS_LITERAL_CSTRING("success") `rv` here is the result of `GetChannelCreation()`. Is that the right value to check? I would have thought you wanted to look at `primingResult` instead. ::: toolkit/components/telemetry/Histograms.json:7800 (Diff revision 2) > "kind": "enumerated", > "n_values": 16, > "description": "How often do we get back an HSTS priming result which upgrades the connection to HTTPS? 0=cached (no upgrade), 1=cached (do upgrade), 2=cached (blocked), 3=already upgraded, 4=priming succeeded, 5=priming succeeded (block due to pref), 6=priming succeeded (no upgrade due to pref), 7=priming failed (block), 8=priming failed (accept)" > }, > + "HSTS_PRIMING_REQUEST_DURATION": { > + "alert_emails": ["seceng@mozilla.org"], We have a separate alias for telemetry: seceng-telemetry@mozilla.com ::: toolkit/components/telemetry/Histograms.json:7802 (Diff revision 2) > "description": "How often do we get back an HSTS priming result which upgrades the connection to HTTPS? 0=cached (no upgrade), 1=cached (do upgrade), 2=cached (blocked), 3=already upgraded, 4=priming succeeded, 5=priming succeeded (block due to pref), 6=priming succeeded (no upgrade due to pref), 7=priming failed (block), 8=priming failed (accept)" > }, > + "HSTS_PRIMING_REQUEST_DURATION": { > + "alert_emails": ["seceng@mozilla.org"], > + "bug_numbers": [1311893], > + "expires_in_version": "61", If it's an exploratory measurement, we usually ask for it to be limited to 6 releases (so Fx 58 in this case). You can always extend it later if you're still using it. ::: toolkit/components/telemetry/Histograms.json:7808 (Diff revision 2) > + "kind": "exponential", > + "low": 100, > + "high": 30000, > + "n_buckets": 100, > + "keyed": true, > + "description": "The amount of time required for HSTS priming requests (ms), keyed by success or failure of the priming request. (success, fail)" The key for failed priming attempts is "failure" in the code, not "fail".
Attachment #8803226 -
Flags: review-
Updated•8 years ago
|
Attachment #8803226 -
Flags: feedback?(francois)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8803226 -
Flags: feedback?(francois)
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8803226 [details] Bug 1311893 - HSTS Priming timing telemetry p=francois https://reviewboard.mozilla.org/r/87536/#review87552 Looks good. datareview+ and r+.
Attachment #8803226 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8803226 -
Flags: feedback?(francois)
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
Pushed by kmckinley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2aa6df19f5f2 HSTS Priming timing telemetry r=ckerschb,francois p=francois
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2aa6df19f5f2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•