Closed Bug 1380256 Opened 7 years ago Closed 7 years ago

Implement the update ping (reason: "success")

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(2 files)

Bug 1380256 - Implement the update ping with reason "success".
59 bytes, text/x-review-board-request
chutten
: review+
molly
: review+
Details
Update-Ping Request for Data Collection Review.md
6.14 KB, text/markdown
Details 
This bug is about implementing the "update" ping with reason "success": the one that gets sent once the update is successfully applied after the browser is restarted.

The updated spec lives at https://docs.google.com/document/d/154xjmWRcGwO0QXBgoKWhcfjFg9ty36-erfLfNClV5KM/edit
Blocks: 1351397
Points: --- → 3
Depends on: 1120372
Priority: -- → P2
Whiteboard: [measurement:client]
Priority: P2 → P1
Assignee: nobody → alessio.placitelli
@Georg, would you kindly review the Telemetry/High level parts?

@Matt, would you kindly review the update mechanics specifics? What I'm trying to do here is send a ping whenever Firefox is restarted after a successful update. Is the browser content handler the right place for that? I also left a TODO in the code with a question about the update channel.

[1] - https://docs.google.com/document/d/154xjmWRcGwO0QXBgoKWhcfjFg9ty36-erfLfNClV5KM/edit#heading=h.riv2m8is2zov
@Rebeccca: would you kindly do a data review? This patch adds 3 new fields to the "update" ping. The ping will now be sent when an update was successfully applied as well.

The attached patch contains the changes to the documentation in the .rst file.
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review175976

r+ pending a couple issues.

::: browser/components/nsBrowserContentHandler.js:523
(Diff revision 2)
>              if (prefb.prefHasUserValue("app.update.postupdate"))
>                overridePage = getPostUpdateOverridePage(overridePage);
>  
>              overridePage = overridePage.replace("%OLD_VERSION%", old_mstone);
> +            // Send the update ping to signal that the update was successful.
> +            UpdatePing.handleUpdateSuccess(old_mstone, old_buildId);

This is the right function for this code to be in, but it should be inside the "if (prefb.prefHasUserValue("app.update.postupdate"))" check. That pref is only set immediately after a successful update, so it rules out any other way we might get into here.

::: toolkit/components/telemetry/UpdatePing.jsm:54
(Diff revision 2)
> +   */
> +  handleUpdateSuccess(aPreviousVersion, aPreviousBuildId) {
> +    this._log.trace("handleUpdateSuccess");
> +
> +    // TODO: How to get the previous channel? Can we even switch channel with an
> +    // update package?

We won't apply an update that's built for a different channel, but it is possible for an update to change the channel. That means update.channel will be the previous channel in that case, because it would have to be the same as the build that update was applied to. Usually that will be the same as the current channel but it's not guaranteed.
Attachment #8899523 - Flags: review?(mhowell) → review+
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review175976

> We won't apply an update that's built for a different channel, but it is possible for an update to change the channel. That means update.channel will be the previous channel in that case, because it would have to be the same as the build that update was applied to. Usually that will be the same as the current channel but it's not guaranteed.

Thanks for the great feedback so far! I can't find a way to get the channel that was set by the update in [here](http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/components/nsBrowserContentHandler.js#510). Any suggestion on how to get it?
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review175976

> Thanks for the great feedback so far! I can't find a way to get the channel that was set by the update in [here](http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/components/nsBrowserContentHandler.js#510). Any suggestion on how to get it?

After the update has been applied, you can most reliably get the new channel from [UpdateUtils.UpdateChannel](http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/modules/UpdateUtils.jsm#54). I'm not 100% I understood the question, let me know if what you need is something different.
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review175976

> After the update has been applied, you can most reliably get the new channel from [UpdateUtils.UpdateChannel](http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/modules/UpdateUtils.jsm#54). I'm not 100% I understood the question, let me know if what you need is something different.

Sorry if I wasn't clear. I'm aware of the UpdateUtils, but that would give us the *current* channel. So, if an update had changed the channel, we would get the one set by the update blob, not the one that was set before the update. I'm interested in latter: the channel that was set before an update that changes channel... changes it :)
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review175976

> Sorry if I wasn't clear. I'm aware of the UpdateUtils, but that would give us the *current* channel. So, if an update had changed the channel, we would get the one set by the update blob, not the one that was set before the update. I'm interested in latter: the channel that was set before an update that changes channel... changes it :)

Oh, okay, I see. At this point, the update manager's activeUpdate property will still point to the update that was just applied, so you can get that channel by exactly the same method that the "ready" ping uses gets the channel that it sends; that update's channel property will be the channel it was meant to apply to, which means it's the previous channel.
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

I'm not sure why ReviewBoard didn't set up the review flag. Rebecca, please see comment 4 for the data-review request.
Attachment #8899523 - Flags: review?(rweiss)
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review176834

A few comments.

::: commit-message-7dddb:2
(Diff revision 4)
> +Bug 1380256 - Implement the update ping with reason "success". r?chutten,mhowell,rweiss
> +

commit message needs explanatory paragraph detailing how/why.

::: toolkit/components/telemetry/UpdatePing.jsm:69
(Diff revision 4)
> +   * @param {String} aPreviousBuildId The browser build id we updated from.
> +   */
> +  handleUpdateSuccess(aPreviousVersion, aPreviousBuildId) {
> +    this._log.trace("handleUpdateSuccess");
> +
> +    // Even if unlikely, un update could potentially change the update channel. Moreover,

Not too unlikely. Happened when we moved WinXP users to ESR52.

::: toolkit/components/telemetry/docs/data/update-ping.rst:23
(Diff revision 4)
>          targetChannel: <string>, // "nightly"
>          targetVersion: <string>, // "56.01a"
>          targetBuildId: <string>, // "20080811053724"
> +        previousChannel: <string>, // "nightly" or null
> +        previousVersion: <string>, // "55.01a"
> +        previousBuildId: <string>, // "20080810053724"

Do we need any notation here to say that only one of target\* or previous\* will be sent?

Here it looks as though the payload will always contain 7 fields, one of which might be null.

::: toolkit/components/telemetry/tests/browser/browser_UpdatePingSuccess.js:31
(Diff revision 4)
> +function writeUpdatesToXMLFile(aText) {
> +  let file = Cc["@mozilla.org/file/directory_service;1"].
> +             getService(Ci.nsIProperties).
> +             get("UpdRootD", Ci.nsIFile);
> +  file.append("updates.xml");
> +  if (!file.exists()) {

Ugh. I hate tests that require using the filesystem. Is there no way to mock out the update service so that we can just tell it what it should pretend?
Attachment #8899523 - Flags: review?(chutten) → review-
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review176834

> Do we need any notation here to say that only one of target\* or previous\* will be sent?
> 
> Here it looks as though the payload will always contain 7 fields, one of which might be null.

I've made it clearer in the sample. However, it was already reported in the extended field description at the bottom of the docs.

> Ugh. I hate tests that require using the filesystem. Is there no way to mock out the update service so that we can just tell it what it should pretend?

On a closer look, turns out we don't even need that part. Isn't it a good day when you drop your code?
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

Sorry Rebecca, ReviewBoard keeps cancelling my review request for your. See comment 4.
Attachment #8899523 - Flags: review?(rweiss)
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review176904
Attachment #8899523 - Flags: review?(chutten) → review+
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

data-r?François (in case Rebecca doesn't get here soon enough) - Any chance you could review this? See comment 4 for context: we're scheduling to send the "update" ping also when an update is correctly applied, in addition to when the update is ready to be applied.
Attachment #8899523 - Flags: review?(francois)
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

I don't have any context/knowledge of this change to have an informed opinion. Deferring to Rebecca.
Attachment #8899523 - Flags: review?(francois)
Blocks: 1394454
:dexter, can you please attach a file to this bug with the questions (and your answers!) located here: https://docs.google.com/document/d/1SSn5w8DfCSkHWJS8DNTd7ya82diWRxaDUFM5aL4UDDo/edit ?  Review will follow quickly after that.
Flags: needinfo?(alessio.placitelli)
(In reply to Rebecca Weiss from comment #21)
> :dexter, can you please attach a file to this bug with the questions (and
> your answers!) located here:
> https://docs.google.com/document/d/
> 1SSn5w8DfCSkHWJS8DNTd7ya82diWRxaDUFM5aL4UDDo/edit ?  Review will follow
> quickly after that.

Sure, thanks Rebecca! Here's the file, converted to markdown, with the answers. Please note that the other patch attached to this bug contains the relevant changes to the update ping documentation that lives at: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/update-ping.html

Please let me know if you need anything else!
Flags: needinfo?(alessio.placitelli) → needinfo?(rweiss)
Attachment #8902174 - Attachment mime type: application/x-genesis-rom → text/markdown
Comment on attachment 8899523 [details]
Bug 1380256 - Implement the update ping with reason "success".

https://reviewboard.mozilla.org/r/170802/#review180210

1) Is there documentation that describes the schema for the ultimate data set available publicly, complete and accurate?
Yes, these have tracking bugs (1380256) and will follow the same documentation standards as other Telemetry pings.

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism)  
Yes, this will follow the same protocol for Telemetry data in general.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?
Yes, Ben Hearsum.

4) Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?  
These are all category 1 measurements.

5) Is the data collection default-on or default-off? 
Default on, all channels and locales

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? If unsure: escalate to legal.
Yes

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)
No
Attachment #8899523 - Flags: review?(rweiss) → review+
Flags: needinfo?(rweiss)
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4349fda0e3f2
Implement the update ping with reason "success". r=chutten,mhowell,rweiss+418169
https://hg.mozilla.org/mozilla-central/rev/4349fda0e3f2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: