1440701 - Telemetry for browser upgrade failure rates

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P1)

Description

[Jonathan Kingston [:jkt] he/him]

We should implement more telemetry for measuring how successful the upgrading of content is happening within Firefox implemented in Bug 1435733.

I think to measure the following data points would be helpful:

- Failure of non upgrade HTTPS
- Failure of browser upgrade HTTPS
- Success of non upgrade HTTPS
- Success of browser upgrade HTTPS

Questions:
- Should timeouts be measured separately?
- Should CSP and HSTS upgrades be measured separately?
- Would implementing this on a channel be the right direction?
- Do we have other existing telemetry that might be useful to monitor here?

Initial telemetry suggests these loads are over 0.37% of requests:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2018-02-22&keys=__none__!__none__!__none__&max_channel_version=nightly%252F60&measure=HTTP_SCHEME_UPGRADE_TYPE&min_channel_version=null&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2018-01-29&table=0&trim=1&use_submission_date=0

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

We should get that telemtry update landed ASAP, otherwise the whole study does not provide any results.

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1440701 - Adding in telemetry for upgrading display content.

Review commit: https://reviewboard.mozilla.org/r/224878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224878/

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

https://reviewboard.mozilla.org/r/224878/#review230858

::: netwerk/protocol/http/nsHttpChannel.cpp:7298
(Diff revision 1)
> +            "security.mixed_content.upgrade_display_content", false);
> +        }
> +
> +        if (mLoadInfo && sIsUpgradableDisplayContentPrefEnabled) {
> +            ChannelDisposition upgradeChanDisposition = chanDisposition;
> +            if (mLoadInfo->GetBrowserUpgradeInsecureRequests()) {

Add a comment explaining what we are doing here and why.

::: toolkit/components/telemetry/Histograms.json:2884
(Diff revision 1)
>      "n_values": 16,
>      "releaseChannelCollection": "opt-out",
>      "description": "Channel Disposition: 0=Cancel, 1=Disk, 2=NetOK, 3=NetEarlyFail, 4=NetlateFail, +8 for HTTPS"
>    },
> +  "HTTP_CHANNEL_DISPOSITION_UPGRADE" : {
> +    "record_in_processes": ["main", "content"],

nsHttpChannel only lives in the main channel. You may want to not record in the content process.

Comment 4

[Jonathan Kingston [:jkt] he/him]

Thanks Valentin!

I'm thinking we might want to use a keyed histogram here similar to the HTTP redirects one. This would allow different scenarios to be separated into histograms.

My thought for this was that I think it would be worth having failure telemetry on display content that would have been upgraded if the pref was off, it looks like http failure rate is actually much higher than https anyway. However we might want to move adding this into another bug as it will require more keys on the loadInfo etc.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

https://reviewboard.mozilla.org/r/224878/#review230912

Hey, besides my comments about the patch I don't understand how that patch actually works. Maybe I just don't understand what we are trying to do. Please summarize what you would try to measure to make sure we are not missing anything. Once document and my suggestions incoroporated, please flag me again.

::: netwerk/protocol/http/nsHttpChannel.cpp:7293
(Diff revision 1)
> +        static bool sIsUpgradableDisplayContentPrefEnabled;
> +        static bool sIsInited = false;
> +        if (!sIsInited) {
> +            sIsInited = true;
> +            Preferences::AddBoolVarCache(&sIsUpgradableDisplayContentPrefEnabled,
> +            "security.mixed_content.upgrade_display_content", false);

Ideally that would be encapsulated within a static function within nsMixedContentBlocker which would then allow to remove sIsUpgradableDisplayContentPrefEnabled from nsContentUtils.cpp so we have that info within on place and do not need to call AddBoolVarCache twice.

::: toolkit/components/telemetry/Histograms.json:2887
(Diff revision 1)
>    },
> +  "HTTP_CHANNEL_DISPOSITION_UPGRADE" : {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail]", "[e-mail]", "[e-mail]"],
> +    "bug_numbers": [1440701],
> +    "expires_in_version": "never",

I thought we don't allow 'never' anymore. I am personally fine with it, but you need to get a data-review from a peer anyway. I think you can flag francois.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224878/diff/1-2/

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224878/diff/2-3/

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

https://reviewboard.mozilla.org/r/224878/#review231662

yep, that looks good to me. Please don't forget to get a data peer sign off on it!

Comment 9

[Jonathan Kingston [:jkt] he/him]

Attachment: request-1440701.md

Could I get a data review francois?

Comment 10

[François Marier [:francois]]

Comment on attachment 8958983 [details]
request-1440701.md

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.json.

2) Is there a control mechanism that allows the user to turn the data collection on and off? 

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1

5) Is the data collection request for default-on or default-off?

Default-on everywhere.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, telemetry alerts are fine.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f61cd15b68a0
Adding in telemetry for upgrading display content. r=ckerschb,valentin

Comment 12

[Stefan Hindli [:stefan_hindli]]

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