Telemetry for browser upgrade failure rates

RESOLVED FIXED in Firefox 61



2 years ago
Last year


(Reporter: jkt, Assigned: jkt)


(Blocks 1 bug)

60 Branch
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)


(Whiteboard: [domsecurity-active])


(2 attachments)

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

- 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:!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
Assignee: nobody → jkt
Priority: P2 → P1
Whiteboard: [domsecurity-active]
We should get that telemtry update landed ASAP, otherwise the whole study does not provide any results.
Comment on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

::: 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"
>    },
> +    "record_in_processes": ["main", "content"],

nsHttpChannel only lives in the main channel. You may want to not record in the content process.
Attachment #8955902 - Flags: review?(valentin.gosu) → review+
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 on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

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)
>    },
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["", "", ""],
> +    "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.
Attachment #8955902 - Flags: review?(ckerschb)
Comment on attachment 8955902 [details]
Bug 1440701 - Adding in telemetry for upgrading display content.

yep, that looks good to me. Please don't forget to get a data peer sign off on it!
Attachment #8955902 - Flags: review?(ckerschb) → review+
Posted file
Could I get a data review francois?
Attachment #8958983 - Flags: review?(francois)
Comment on attachment 8958983 [details]

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](** 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)?


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


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

No, telemetry alerts are fine.
Attachment #8958983 - Flags: review?(francois) → review+
Pushed by
Adding in telemetry for upgrading display content. r=ckerschb,valentin
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1450726
You need to log in before you can comment on or make changes to this bug.