Closed Bug 1120372 Opened 9 years ago Closed 7 years ago

Implement the update ping (reason: "ready")

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

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/
Whiteboard: [not a blocker]
Blocks: 1120356
No longer blocks: 1120370
Blocks: 1122482
No longer blocks: 1120356
Priority: -- → P3
Whiteboard: [not a blocker] → [not a blocker][measurement:client]
Priority: P3 → P4
Whiteboard: [not a blocker][measurement:client] → [measurement:client]
Component: Client: Desktop → Telemetry
Product: Firefox Health Report → Toolkit
Priority: P4 → P3
Other code uses the pref "app.update.postupdate" for update notification pages etc.:
https://dxr.mozilla.org/mozilla-central/search?q="app.update.postupdate"
Priority: P3 → P4
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
Blocks: 1351397
No longer blocks: 1122482
Priority: P4 → P1
Summary: Implement a Telemetry upgrade ping → Implement the update ping (reason: "ready")
Assignee: nobody → alessio.placitelli
Points: --- → 3
Blocks: 1380256
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
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)?
Flags: needinfo?(robert.strong.bugs)
Matt, could you kindly take over this? (see comment 6)
Flags: needinfo?(robert.strong.bugs) → needinfo?(mhowell)
(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.
Flags: needinfo?(mhowell)
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 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#
Attachment #8885733 - Flags: review?(benjamin)
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 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?
Attachment #8885733 - Flags: review?(gfritzsche) → review+
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).
Attachment #8885733 - Flags: review?(benjamin)
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 on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

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

data-r=me
Attachment #8885733 - Flags: review?(benjamin) → review+
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.
Attachment #8885733 - Flags: review?(mhowell) → review-
Comment on attachment 8885733 [details]
Bug 1120372 - Introduce the "update" ping.

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

Looks good to me! Thanks.
Attachment #8885733 - Flags: review?(mhowell) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ac1b19c47aac
Introduce the "update" ping. r=bsmedberg,gfritzsche,mhowell
https://hg.mozilla.org/mozilla-central/rev/ac1b19c47aac
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1386619
You need to log in before you can comment on or make changes to this bug.