1120372 - Implement the update ping (reason: "ready")

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

Per the Telemetry/FHR unification plan [0], we need to add an upgrade ping:
> After a Firefox upgrade, the client should immediately send a ping containing the new installation data.

[0] https://docs.google.com/a/mozilla.com/document/d/1IGpzsYGi_sq3YFQDAPyKOkU_BKvXAC95fZYA2i4ceVs/

Comment 1

[Georg Fritzsche [:gfritzsche]]

Other code uses the pref "app.update.postupdate" for update notification pages etc.:
https://dxr.mozilla.org/mozilla-central/search?q="app.update.postupdate"

Comment 2

[Robert Strong (they/them - no direct email)]

Note: recording this in the following file will provide the ability to record as well as differentiate an update (via the postUpdate pref), upgrades and downgrades, and likely new installs.

https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js

Comment 3

[Alessio Placitelli [:Dexter]]

This bug is about implementing the "update" ping with reason "ready": the one that gets sent once the update is ready to be applied. Bug 1380256 will take care of implementing the ping sent once the browser is restarted.

The updated spec lives at https://docs.google.com/document/d/154xjmWRcGwO0QXBgoKWhcfjFg9ty36-erfLfNClV5KM/edit

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1120372 - Introduce the "update" ping.

This ping will be sent as soon as an update is ready to be applied, after
it's downloaded. This is currently enabled but protected behind the
'toolkit.telemetry.updatePing.enabled' preference.

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

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

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

Comment 6

[Alessio Placitelli [:Dexter]]

Robert, could you kindly give feedback at the UpdatePing.jsm file in the attached patch (MozReview)?

I'm particularly interested in:

- Is this the right way to detect the "ready" state mentioned in the spec (comment 3, meaning that "user is ready to apply the update")?
- Is the data from the |activeUpdate| for the version we want to update to?
- Are xpcshell tests enough for testing this? I need to trigger a staged update and a "non staged" update to see if the ping is generated and contains the correct data. Can you point me to any simple example that does that (minus the ping part)?

Comment 7

[Alessio Placitelli [:Dexter]]

Matt, could you kindly take over this? (see comment 6)

Comment 8

[Molly Howell (she/her) (mostly inactive)]

(In reply to Alessio Placitelli [:Dexter] from comment #6)
> - Is this the right way to detect the "ready" state mentioned in the spec
> (comment 3, meaning that "user is ready to apply the update")?

The "pending" states mean that the download of the update has finished, but nothing has been done with it yet. If staging is being used, then that's when the staging step is started. When the staging finishes successfully it will update the state to STATE_APPLIED[_SERVICE], and that's when the restart prompt is shown. Without staging, the prompt is shown while the state is at pending and the "applied" states are not used. I don't think there's any better way to distinguish those cases than to check nsIApplicationUpdateService.canStageUpdates.

> - Is the data from the |activeUpdate| for the version we want to update to?

It's for the update that's being installed. If that version is a watershed, then there might be more updates needed before arriving at the final version being updated to. There isn't any way to detect that because the server (balrog) will not offer the later versions for updating to until the watershed is installed.

> - Are xpcshell tests enough for testing this? I need to trigger a staged
> update and a "non staged" update to see if the ping is generated and
> contains the correct data. Can you point me to any simple example that does
> that (minus the ping part)?

We don't have xpcshell tests that test actually applying updates, only mochitests (which live in /toolkit/mozapps/update/tests/browser and tests/chrome). I don't know of a reason why xpcshell wouldn't work though, this might just be an artifact of how our tests ended up being organized. I think you might specifically want to look at /toolkit/mozapps/update/tests/chrome/test_0101_background_restartNotification.xul and test_0102_background_restartNotification_staging.xul.

Comment 9

[Robert Strong (they/them - no direct email)]

There are xpcshell tests that apply a staged update as well as launch the application (Firefox, SeaMonkey, and Thunderbird) to apply an update and then emulate the remainder of app update startup after an update has been applied. You can probably just get away with emulating an update has been applied during startup and here is a simple xpcshell test that does that.
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_aus_update/cleanupSuccessLogMove.js

There might be a way to emulate a successfully applying a staged update but I'm not sure. Here is an xpcshell test that stages an update though it does quite a few more things than what you will need to do including emulating the first startup after applying an update so you could use it for both.
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_base_updater/marStageSuccessComplete.js

For downloading there is this xpcshell test that also tests a few more things than you need to test.
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_aus_update/downloadCompleteAfterPartialFailure.js

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

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

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

data-r?

Benjamin, this patch adds the opt-out "update" ping with reason "ready", which is sent as soon as an update is downloaded and ready to be applied. This patch also lands the in-tree docs.

The spec is available at https://docs.google.com/document/d/154xjmWRcGwO0QXBgoKWhcfjFg9ty36-erfLfNClV5KM/edit#

Comment 12

[Alessio Placitelli [:Dexter]]

Thank you Matt and Robert for the precious directions from comment 8 and comment 9. Unfortunately, I had to resort to adding a test in the toolkit/mozapps/update/test/browser as I wasn't able to replicate the test behaviour inside toolkit/components/telemetry due to all the dependencies. What I tried:

- Adding the files from toolkit/mozapps/* as support-files to the test in toolkit/components/telemetry. However, that failed halfway due to the build system complaining that some dependencies were not advertised.
- Doing some copy/paste surgery by importing the functions that I needed in my test in toolkit/components/telemetry (both for xpcshell and browser tests). That became too big (and too problematic), too quick :)

Matt, would you reviewing the bits of the attached patch that interact with the update system?

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

https://reviewboard.mozilla.org/r/156538/#review166736

This looks good to me from the Telemetry perspective (with the below addressed).

::: toolkit/components/telemetry/UpdatePing.jsm:41
(Diff revision 3)
> +
> +    if (!this._enabled) {
> +      return;
> +    }
> +
> +    Services.obs.addObserver(this, UPDATE_DOWNLOADED_TOPIC);

Is this guaranteed to be early enough?

::: toolkit/components/telemetry/UpdatePing.jsm:69
(Diff revision 3)
> +      return;
> +    }
> +
> +    let update = updateManager.activeUpdate;
> +
> +    const PAYLOAD = {

This is not a classic constant, please use camelCase.

::: toolkit/components/telemetry/UpdatePing.jsm:76
(Diff revision 3)
> +      targetChannel: update.channel,
> +      targetVersion: update.appVersion,
> +      targetBuildId: update.buildID,
> +    };
> +
> +    const OPTIONS = {

Ditto.

::: toolkit/components/telemetry/docs/data/update-ping.rst:5
(Diff revision 3)
> +
> +"update" ping
> +==================
> +
> +This opt-out ping is sent from Firefox Desktop when a browser update is ready to be applied. There is a

Is this sent exactly once per update?
Or are there scenarios where any rollback/retry mechanism or others might lead to >1 update ping per update?

Lets call this out in the docs.

::: toolkit/components/telemetry/docs/data/update-ping.rst:27
(Diff revision 3)
> +This field only supports the value ``ready``, meaning that the ping was generated after an update was downloaded
> +and marked as ready to be processed. For *non-staged* updates this happens as soon as the download
> +finishes and is verified while for *staged* updates this happens before the staging step is started.

Do you need to tell staged and non-staged updates apart from the ping data?

::: toolkit/components/telemetry/tests/browser/browser.ini:1
(Diff revision 3)
>  # This Source Code Form is subject to the terms of the Mozilla Public

"This file contains only whitespace changes." ?
Revert this?

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

https://reviewboard.mozilla.org/r/156538/#review166736

> Is this guaranteed to be early enough?

I think so, as this is happening right when the browser starts (early setup).

> Is this sent exactly once per update?
> Or are there scenarios where any rollback/retry mechanism or others might lead to >1 update ping per update?
> 
> Lets call this out in the docs.

AFAICT, the topic is notified after the update has been verified, so that should happen only once. @mhowell, any clue about this?

> Do you need to tell staged and non-staged updates apart from the ping data?

I don't think so, at this stage knowing that an update is ready to be applied is enough (as per spec).

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

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

Comment 16

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

https://reviewboard.mozilla.org/r/156538/#review166736

> AFAICT, the topic is notified after the update has been verified, so that should happen only once. @mhowell, any clue about this?

Yes, the topic you're listening for is only sent when the restart prompt really is about to be shown. Any download retries or other fallbacks that occur before then will not trigger it.

Comment 17

[Benjamin Smedberg]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

https://reviewboard.mozilla.org/r/156538/#review166808

data-r=me

Comment 18

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

https://reviewboard.mozilla.org/r/156538/#review166806

r- for the incomplete determination of valid update states; overall I think we're in good shape here.

::: toolkit/components/telemetry/UpdatePing.jsm:52
(Diff revision 4)
> +   *
> +   * @param {String} aUpdateState The state of the downloaded patch. See
> +   *        nsIUpdateService.idl for a list of possible values.
> +   */
> +  _handleUpdateReady(aUpdateState) {
> +    const ALLOWED_STATES = [ "pending", "pending-service", "pending-elevate" ];

This set of states is correct when staging updates is disabled, but if it's enabled then the state here should be either 'applied' or 'applied-service'.

::: toolkit/mozapps/update/tests/browser/browser_TelemetryUpdatePing.js:5
(Diff revision 4)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/
> +*/
> +
> +Cu.import("resource://testing-common/TelemetryArchiveTesting.jsm", this);

I'm okay with this test being here (especially with the component notation added to the moz.build, thanks for doing that), but I would appreciate a comment at the top explaining that this is really a telemetry test and not an update UI test like the rest of this directory, and why it's here despite that.

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156538/diff/4-5/

Comment 20

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

https://reviewboard.mozilla.org/r/156538/#review166850

Looks good to me! Thanks.

Comment 21

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156538/diff/5-6/

Comment 22

[Alessio Placitelli [:Dexter]]

Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156538/diff/6-7/

Comment 23

[Alessio Placitelli [:Dexter]]

Storage impact estimate doc: https://docs.google.com/document/d/1CD0VZnnyx-Wcm4-bqlsRDtLdYn6ENySeMNqCNaoKWag/edit# (LDAP)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac1b19c47aac
Introduce the "update" ping. r=bsmedberg,gfritzsche,mhowell

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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