Collect telemetry on timing of HSTS priming requests

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kmckinley, Assigned: kmckinley)

Tracking

51 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

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
Change to use a keyed histogram instead of 2 histograms.
Comment hidden (mozreview-request)
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

3 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+
Attachment #8803226 - Flags: feedback?(francois)

Comment 6

3 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-
Attachment #8803226 - Flags: feedback?(francois)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8803226 - Flags: feedback?(francois)

Comment 9

3 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+
Attachment #8803226 - Flags: feedback?(francois)
Comment hidden (mozreview-request)

Comment 11

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2aa6df19f5f2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.