Closed Bug 1137252 Opened 5 years ago Closed 5 years ago

Telemetry should retain pings even if sending is disabled

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: benjamin, Assigned: Dexter)

References

Details

Attachments

(3 files, 16 obsolete files)

2.38 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
38.17 KB, patch
vladan
: review+
Details | Diff | Splinter Review
9.05 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
Even if the user has disabled sending FHR data, the client should still collect and save the pings locally on the hard drive. This is what powers about:healthreport and the future self-repair project.

NI?gfritzsche just to make sure you're aware of this.
Flags: needinfo?(gfritzsche)
Talked about it with Alessio when coordinating, he has it on his radar.
Flags: needinfo?(gfritzsche)
Assignee: nobody → alessio.placitelli
Blocks: 1134279
Attached patch bug1137252.patch (obsolete) — Splinter Review
This patch makes TelemetryPing persist pings to the disk even if Telemetry is disabled through preferences.

It also adds a new test to test_TelemetryPing.js to verify that pings get persisted to disk and not sent when Telemetry is disabled.
Attachment #8571836 - Flags: review?(vdjeric)
Attached patch bug1137252.patch (obsolete) — Splinter Review
Sorry, |Preferences.ignore| was missing the third parameter.
Attachment #8571836 - Attachment is obsolete: true
Attachment #8571836 - Flags: review?(vdjeric)
Attachment #8571839 - Flags: review?(vdjeric)
Attachment #8571839 - Flags: review?(vdjeric)
Attached patch bug1137252.patch - v2 (obsolete) — Splinter Review
This patch makes Telemetry persist pings to disk if both FHR and Telemetry are disabled.

If one of them is enabled, pings are sent. The patch also changes the test suites by providing dummy server URLs where needed.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b661b35cf768
Attachment #8571839 - Attachment is obsolete: true
When you say "if one of them is enabled" what does that mean?

If FHR is disabled, that should disable all sending, to match what the UX checkboxes do. Telemetry is now a sub-feature of FHR.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> When you say "if one of them is enabled" what does that mean?
> 
> If FHR is disabled, that should disable all sending, to match what the UX
> checkboxes do. Telemetry is now a sub-feature of FHR.

Good point. Disabling FHR also disables Telemetry, but I think I should enforce this constraint in TelemetryPing as well.
Attached patch bug1137252.patch - v3 (obsolete) — Splinter Review
Only allow sending pings if FHR uploads are enabled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1398adaded4c
Attachment #8573304 - Attachment is obsolete: true
Should I review this patch?
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #8)
> Should I review this patch?

No, I was waiting before requesting a review due to the 8bytes leaks in Mochitests/reftests this patch introduces.

Yesterday I was finally able to track down the issue to nsTerminator.cpp [1]. Since we're always recording pings to disk, |canRecord| is true. The logic before was a bit different, as [2] was basically disabling "canRecord" in debug builds (Telemetry.canSend is false on desktop debug builds, since TELEMETRY_REPORTING is not defined).

I'm not sure what's the best way to deal with this, other than bailing out on when |Telemetry.canSend| is false instead of |Telemetry.canRecord|. I should probably talk to :Yoric (who worked on bug 1044020). Do you have any suggestion?

[1] - https://hg.mozilla.org/mozilla-central/annotate/d9b06c673f80/toolkit/components/terminator/nsTerminator.cpp#l395
[2] - https://hg.mozilla.org/mozilla-central/annotate/fecf1afb0830/toolkit/components/telemetry/TelemetryPing.jsm#l642
Flags: needinfo?(vdjeric)
We need to rethink CanRecord and CanSend. Before unification, CanSend was true when Telemetry was enabled and the sender was an official (released) build. CanRecord indicated whether Telemetry was enabled.

Now that "Telemtry" = FHR + Telemetry, the meanings have changed. Additionally, we now record data for self-support regardless of optin status.

We need a new set of Telemetry.can* methods and we need to convert all the callers of canRecord/canSend
Flags: needinfo?(vdjeric)
No longer blocks: 1134279
Depends on: 1134279
Blocks: 1122481
No longer blocks: 1120356
As discussed via email, ping retention should work as follows:

- FHR enabled, Telemetry disabled -> Only send pings with FHR-level data. If sending fails, pings will be stored in the "pings" directory and get sent again on startup.
- FHR enabled, Telemetry enabled -> send pings with extended stats. As in the previous case, pings are stored in the "pings" directory when sending them fails.
- FHR disabled (and Telemetry disabled), SelfSupport Disabled -> Don't send pings, store them in a separate "archive pings" directory. They won't be sent when Firefox starts up. They will just live in the archive directory.

Since SelfSupport will make use of pings in the archive directory, should we *always* save a copy of the ping there if SelfSupport is enabled?
Flags: needinfo?(vdjeric)
(In reply to Alessio Placitelli [:Dexter] from comment #11)
> Since SelfSupport will make use of pings in the archive directory, should we
> *always* save a copy of the ping there if SelfSupport is enabled?

Yes, we will always need them.
(The only alternative would be to keep track of outgoing pings too that selfsupport should see and archive them after submission etc. - that seems overly complex with no real wins)
Flags: needinfo?(vdjeric)
There is no flag to enable/disable self support.

The only case I can think of is some enterprise administrators disable FHR data collection and may want to disable this style of collection as well.
What about other products though?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> There is no flag to enable/disable self support.

The pref (browser.selfsupport.enabled) introduced in bug 1111022 doesn't have that purpose?
(In reply to Alessio Placitelli [:Dexter] from comment #15)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> > There is no flag to enable/disable self support.
> 
> The pref (browser.selfsupport.enabled) introduced in bug 1111022 doesn't
> have that purpose?

It does and we have to consider different products, e.g. Thunderbird probably doesn't want that (or should opt into it if needed).

Benjamin, let us know if you disagree.
Flags: needinfo?(benjamin)
Is this WIP with that question resolved then?
Thunderbird may not want any of this... it doesn't have FHR at all.

The selfsupport.enabled pref isn't intended to be a user pref: it's a feature flag.
Flags: needinfo?(benjamin)
So... we want it whenever we have FHR, enabled or not?
Yes. Do we have a pref for "the FHR feature is enabled" that we could use/reuse for this purpose?
We have |datareporting.healthreport.service.enabled|.
To handle DRS and FHR being stripped from builds we have |"@mozilla.org/datareporting/service;1" in Cc|.
Attached patch bug1137252.patch - v4 (obsolete) — Splinter Review
This patch introduces the following behaviour:

- Checks if we can send pings (if FHR upload is enabled AND we are in tests or in an official build).
- Always persists pings to the archived pings directory (datareporting/archived) if we are allowed to (that's if FHR is enabled and DRS is available).
Attachment #8573313 - Attachment is obsolete: true
Attached patch bug1137252.patch - v5 (obsolete) — Splinter Review
Also, Telemetry is now always enabled.
Attachment #8583758 - Attachment is obsolete: true
Attachment #8583763 - Flags: review?(vdjeric)
Blocks: 1120356
No longer blocks: 1122481
Attachment #8583763 - Flags: review?(vdjeric)
After discussing the design with :Vladan over IRC, this should be slightly changed.

- TelemetryPing should archive pings if sending them through |TelemetryPing.send| succeeds;
- Telemetry should also archive pings when removing them from the pending pings directory. Pings can be in that folder because they failed to be sent once or because saved directly there, as for the session-shutdown and aborted-session pings.
- Pings should be archived in subdirectories based on the date they were generated (e.g. archived/2015-06)
Flags: needinfo?(gfritzsche)
(In reply to Alessio Placitelli [:Dexter] from comment #24)
> After discussing the design with :Vladan over IRC, this should be slightly
> changed.
> 
> - TelemetryPing should archive pings if sending them through
> |TelemetryPing.send| succeeds;
> - Telemetry should also archive pings when removing them from the pending
> pings directory. Pings can be in that folder because they failed to be sent
> once or because saved directly there, as for the session-shutdown and
> aborted-session pings.

What is the rationale for that?
This will make it less straight-forward to track pings for about:healthreport and about:telemetry (i have to get to that soon & we have to track them to avoid hitting the disk a lot).
Is this about the disk-space-win while the pings are pending?

We originally considered always archiving them right away for the simpler design.

> - Pings should be archived in subdirectories based on the date they were
> generated (e.g. archived/2015-06)

Any rationale for that? To make it easier to look through them on disk for dev/QA/...?
Flags: needinfo?(vdjeric)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> (In reply to Alessio Placitelli [:Dexter] from comment #24)
> > - TelemetryPing should archive pings if sending them through
> > |TelemetryPing.send| succeeds;
> > - Telemetry should also archive pings when removing them from the pending
> > pings directory. 
> 
> What is the rationale for that?
> This will make it less straight-forward to track pings for
> about:healthreport and about:telemetry (i have to get to that soon & we have
> to track them to avoid hitting the disk a lot).
> Is this about the disk-space-win while the pings are pending?

It's not about disk space wins. 
Unless we choose to always save pings in two locations (shutdown pings, aborted session pings, etc), self-support has to look at both the archived/ directory and the saved-telemetry-pings directory.
This approach just ensures there are no lost pings and there are no duplicated pings across dirs.

> > - Pings should be archived in subdirectories based on the date they were
> > generated (e.g. archived/2015-06)
> 
> Any rationale for that? To make it easier to look through them on disk for
> dev/QA/...?

To make it easier to clean up old archived pings (pings > 180 days old).
If we store all the pings in a single "archive" dir, we have to read and parse 180+ archived pings every time we do maintenance.
With this approach, we'll only need to check 30 pings in one dir at most.
Flags: needinfo?(vdjeric)
Clearing n? as Vladan already provided the required information.
Flags: needinfo?(alessio.placitelli)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #26)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> > (In reply to Alessio Placitelli [:Dexter] from comment #24)
> > > - TelemetryPing should archive pings if sending them through
> > > |TelemetryPing.send| succeeds;
> > > - Telemetry should also archive pings when removing them from the pending
> > > pings directory. 
> > 
> > What is the rationale for that?
> > This will make it less straight-forward to track pings for
> > about:healthreport and about:telemetry (i have to get to that soon & we have
> > to track them to avoid hitting the disk a lot).
> > Is this about the disk-space-win while the pings are pending?
> 
> It's not about disk space wins. 
> Unless we choose to always save pings in two locations (shutdown pings,
> aborted session pings, etc), self-support has to look at both the archived/
> directory and the saved-telemetry-pings directory.
> This approach just ensures there are no lost pings and there are no
> duplicated pings across dirs.

Always storing it in two locations was the original plan for simplicity - are there any concerns here?
Otherwise i'd prefer us to stick to that.

> > > - Pings should be archived in subdirectories based on the date they were
> > > generated (e.g. archived/2015-06)
> > 
> > Any rationale for that? To make it easier to look through them on disk for
> > dev/QA/...?
> 
> To make it easier to clean up old archived pings (pings > 180 days old).
> If we store all the pings in a single "archive" dir, we have to read and
> parse 180+ archived pings every time we do maintenance.
> With this approach, we'll only need to check 30 pings in one dir at most.

Ok, i share that concern and didn't consider that this hits this bug already.
To avoid scanning all pings for bug 1122479 & bug 1122480 we will need to keep track of the archived pings anyway, so i'm now planning to add a ping index file (tracking at least uuid, type & submission date).
We could start doing that here or i'll address it in a separate patch?
Flags: needinfo?(vdjeric)
Depends on: 1150030
(In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> > > > - Pings should be archived in subdirectories based on the date they were
> > > > generated (e.g. archived/2015-06)
> > > 
> > > Any rationale for that? To make it easier to look through them on disk for
> > > dev/QA/...?
> > 
> > To make it easier to clean up old archived pings (pings > 180 days old).
> > If we store all the pings in a single "archive" dir, we have to read and
> > parse 180+ archived pings every time we do maintenance.
> > With this approach, we'll only need to check 30 pings in one dir at most.
> 
> Ok, i share that concern and didn't consider that this hits this bug already.
> To avoid scanning all pings for bug 1122479 & bug 1122480 we will need to
> keep track of the archived pings anyway, so i'm now planning to add a ping
> index file (tracking at least uuid, type & submission date).
> We could start doing that here or i'll address it in a separate patch?

Note that this part is tracked in bug 1150030, which has a little discussion too.
(In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> Always storing it in two locations was the original plan for simplicity -
> are there any concerns here?
> Otherwise i'd prefer us to stick to that.

It just seems wasteful to write out data in duplicate when we can just move pings from one directory to another. On shutdown, we'd double the amount of data we write out. Indiscriminately mirroring pings to archive/ is particularly bad for aborted-session pings because it's wasted effort 99.9% of the time and we'd have to remove the aborted ping from archive/ during shutdowns.

> Ok, i share that concern and didn't consider that this hits this bug already.
> To avoid scanning all pings for bug 1122479 & bug 1122480 we will need to
> keep track of the archived pings anyway, so i'm now planning to add a ping
> index file (tracking at least uuid, type & submission date).
> We could start doing that here or i'll address it in a separate patch?

I'd prefer we use directories (directory names really) to track the age of archived pings. The index is another file to maintain and if the index code has a bug or the index gets corrupted during update (e.g. Windows is shutting down and kills Firefox mid-shutdown), we end up with orphaned pings and overlooked Self-Support data. Or we write code to rebuild the index by parsing orphan files (more complexity).
Named subfolders seem a lot simpler.
Flags: needinfo?(vdjeric)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #30)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> > Always storing it in two locations was the original plan for simplicity -
> > are there any concerns here?
> > Otherwise i'd prefer us to stick to that.
> 
> It just seems wasteful to write out data in duplicate when we can just move
> pings from one directory to another. On shutdown, we'd double the amount of
> data we write out. Indiscriminately mirroring pings to archive/ is
> particularly bad for aborted-session pings because it's wasted effort 99.9%
> of the time and we'd have to remove the aborted ping from archive/ during
> shutdowns.

What if we provided two ways of saving pings through TelemetryPing (this came out during a discussion with Georg):

- a savePendingPing function, which saves to the pending pings directory and also archives (so that there's no need to scan two directories)
- a writePing function, which is only responsible of writing a ping to a particular location on the disk, without archiving or doing anything else?

This would solve the issue with aborted sessions (and future similar pings) and would make it possible to keep the archiving logic within TelemetryPing (without spreading it to TelemetryFile). This would also make it simpler to deal with expired pings pruning as well.
Flags: needinfo?(vdjeric)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #30)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> > Always storing it in two locations was the original plan for simplicity -
> > are there any concerns here?
> > Otherwise i'd prefer us to stick to that.
> 
> It just seems wasteful to write out data in duplicate when we can just move
> pings from one directory to another. On shutdown, we'd double the amount of
> data we write out. Indiscriminately mirroring pings to archive/ is
> particularly bad for aborted-session pings because it's wasted effort 99.9%
> of the time and we'd have to remove the aborted ping from archive/ during
> shutdowns.

Yes, we should of course never do that for the temporary aborted-session (e.g. as described above).
Per todays meeting we will:
* always archive pings submitted to TelemetryPing (with the exception of e.g. temporary aborted-session pings as discussed above)
* use the following naming & sub-directory scheme for archived pings: YYYY-MM/<TIMESTAMP>.UUID.type.json
Flags: needinfo?(vdjeric)
This patch adds ping archiving and allows to send pings if only base data recording is enabled.
Attachment #8583763 - Attachment is obsolete: true
Attached patch part 2 - Add test coverage. (obsolete) — Splinter Review
This patch adds test coverage for pings archiving.
Comment on attachment 8588534 [details] [diff] [review]
part 1 - Allow pings to be archived - v6

Review of attachment 8588534 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +469,5 @@
>      let pingData = this.assemblePing(aType, aPayload, aOptions);
> +    // Always persist the pings if we are allowed to.
> +    let archivePromise = this._archivePing(pingData)
> +      .catch(e => this._log.error("send - Failed to archive ping " + pingData.id, e));
> +

Georg, what do you think about this error handling for pings archiving?

The basic idea is that pings archiving errors should not block sending pings. So we catch the errors end report them into the log.
Attachment #8588534 - Flags: feedback?(gfritzsche)
(In reply to Alessio Placitelli [:Dexter] from comment #36)
> Comment on attachment 8588534 [details] [diff] [review]
> part 1 - Allow pings to be archived - v6
> 
> Review of attachment 8588534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryPing.jsm
> @@ +469,5 @@
> >      let pingData = this.assemblePing(aType, aPayload, aOptions);
> > +    // Always persist the pings if we are allowed to.
> > +    let archivePromise = this._archivePing(pingData)
> > +      .catch(e => this._log.error("send - Failed to archive ping " + pingData.id, e));
> > +
> 
> Georg, what do you think about this error handling for pings archiving?
> 
> The basic idea is that pings archiving errors should not block sending
> pings. So we catch the errors end report them into the log.

That seems fine.
(In reply to Georg Fritzsche [:gfritzsche] from comment #37)
> > The basic idea is that pings archiving errors should not block sending
> > pings. So we catch the errors end report them into the log.
> 
> That seems fine.

To be more specific, i'm not sure we can do much better in case of failing to save and we don't expect it to fail, so let's go with that.
Comment on attachment 8588534 [details] [diff] [review]
part 1 - Allow pings to be archived - v6

Review of attachment 8588534 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +242,2 @@
>     */
>    savePing: function(aType, aPayload, aOptions = {}) {

Thought:
We also have addPendingPing() - shouldn't we just unify the two under that name, adding options as needed (say fromFilePath, sendNow, ...)?

Then what you call writePing() can just be named savePing().

I also wouldn't mention anything about a pending pings directory etc. in the user-facing docs, those are implementation details.
What matters it that pings are persisted and optionally sent ("soon" or "later" - we shouldn't be much more specific than that either for now).

@@ +250,5 @@
>      return Impl.savePing(aType, aPayload, options);
>    },
>  
>    /**
> +   * Write a ping to a specified location on the disk.

Explicitly document that this does not add it to the pending pings etc. and is specifically just for saving ping data somewhere for later use.
Attachment #8588534 - Flags: feedback?(gfritzsche)
Thanks Georg. As we discussed over IRC, this patch renames:

- addPendingPing to addPendingPingFromFile
- savePing to addPendingPing
- writePing to savePing
Attachment #8588534 - Attachment is obsolete: true
Attached patch part 2 - Add test coverage - v2 (obsolete) — Splinter Review
Updated because of the changed function names.
Attachment #8588535 - Attachment is obsolete: true
Attachment #8589114 - Flags: review?(vdjeric)
Attachment #8589115 - Flags: review?(vdjeric)
No longer depends on: 1150030
Status: NEW → ASSIGNED
Comment on attachment 8589114 [details] [diff] [review]
part 1 - Allow pings to be archived - v7

Review of attachment 8589114 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +470,5 @@
>                      ", aOptions " + JSON.stringify(aOptions));
>  
>      let pingData = this.assemblePing(aType, aPayload, aOptions);
> +    // Always persist the pings if we are allowed to.
> +    let archivePromise = this._archivePing(pingData)

won't this archive files twice? "shutdown" pings for example will be archived on browser exit and on send during the next session

@@ +775,3 @@
>      // Enable base Telemetry recording, if needed.
>  #if !defined(MOZ_WIDGET_ANDROID)
> +    Telemetry.canRecordBase = Preferences.get(PREF_FHR_ENABLED, false);

hmm, how come?

@@ +994,5 @@
> +   * @return {Boolean} True if pings can be send to the servers, false otherwise.
> +   */
> +  _canSend: function() {
> +    return (Telemetry.isOfficialTelemetry || this._testMode) &&
> +           Preferences.get(PREF_FHR_UPLOAD_ENABLED, false);

can you document these criteria somwhere, in the tree docs or a wiki page? people will need to know which prefs to flip to test their telemetry probes

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ -1477,5 @@
>  
>    /**
> -   * Perform telemetry initialization for either chrome or content process.
> -   */
> -  enableTelemetryRecording: function enableTelemetryRecording(testing) {

Nice to see this logic unified in one place (TelemetryPing) :)
Attachment #8589114 - Flags: review?(vdjeric)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #42)
> Comment on attachment 8589114 [details] [diff] [review]
> part 1 - Allow pings to be archived - v7
> 
> Review of attachment 8589114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryPing.jsm
> @@ +470,5 @@
> >                      ", aOptions " + JSON.stringify(aOptions));
> >  
> >      let pingData = this.assemblePing(aType, aPayload, aOptions);
> > +    // Always persist the pings if we are allowed to.
> > +    let archivePromise = this._archivePing(pingData)
> 
> won't this archive files twice? "shutdown" pings for example will be
> archived on browser exit and on send during the next session

Thank you for your review, Vladan!

No, pings won't get archived twice: pending pings get sent automatically by |sendPersistedPings|, which directly calls |doPing| and doesn't pass again through |send|.

> @@ +775,3 @@
> >      // Enable base Telemetry recording, if needed.
> >  #if !defined(MOZ_WIDGET_ANDROID)
> > +    Telemetry.canRecordBase = Preferences.get(PREF_FHR_ENABLED, false);
> 
> hmm, how come?

The self-support preference was removed because it was not meant to be used there in the first place, I was wrong (see bug 1150975).

> @@ +994,5 @@
> > +   * @return {Boolean} True if pings can be send to the servers, false otherwise.
> > +   */
> > +  _canSend: function() {
> > +    return (Telemetry.isOfficialTelemetry || this._testMode) &&
> > +           Preferences.get(PREF_FHR_UPLOAD_ENABLED, false);
> 
> can you document these criteria somwhere, in the tree docs or a wiki page?
> people will need to know which prefs to flip to test their telemetry probes

I will be updating the in tree docs in a separate patch in this bug.

> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ -1477,5 @@
> >  
> >    /**
> > -   * Perform telemetry initialization for either chrome or content process.
> > -   */
> > -  enableTelemetryRecording: function enableTelemetryRecording(testing) {
> 
> Nice to see this logic unified in one place (TelemetryPing) :)

Yeah, that was misleading :-)
(In reply to Alessio Placitelli [:Dexter] from comment #43)
> The self-support preference was removed because it was not meant to be used
> there in the first place, I was wrong (see bug 1150975).

Oh I see I was confusing this pref with the datareporting.healthreport.uploadEnabled pref. So we'll have one pref to disable FHR and self-support features? That's fine, they're closely tied features anyways
Attachment #8589115 - Flags: review?(vdjeric)
No longer blocks: 1120380
Attachment #8589115 - Flags: review?(vdjeric)
Comment on attachment 8589114 [details] [diff] [review]
part 1 - Allow pings to be archived - v7

r+ as per IRC talk.
Attachment #8589114 - Flags: review+
Comment on attachment 8589114 [details] [diff] [review]
part 1 - Allow pings to be archived - v7

Review of attachment 8589114 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +470,5 @@
>                      ", aOptions " + JSON.stringify(aOptions));
>  
>      let pingData = this.assemblePing(aType, aPayload, aOptions);
> +    // Always persist the pings if we are allowed to.
> +    let archivePromise = this._archivePing(pingData)

won't this archive files twice? "shutdown" pings for example will be archived on browser exit and on send during the next session

@@ +775,3 @@
>      // Enable base Telemetry recording, if needed.
>  #if !defined(MOZ_WIDGET_ANDROID)
> +    Telemetry.canRecordBase = Preferences.get(PREF_FHR_ENABLED, false);

hmm, how come?

@@ +994,5 @@
> +   * @return {Boolean} True if pings can be send to the servers, false otherwise.
> +   */
> +  _canSend: function() {
> +    return (Telemetry.isOfficialTelemetry || this._testMode) &&
> +           Preferences.get(PREF_FHR_UPLOAD_ENABLED, false);

can you document these criteria in the tree docs or a wiki page? people will need to know which prefs to flip to test their telemetry probes

@@ +1003,5 @@
> +   * to do that.
> +   * @return {Boolean} True if pings should be archived, false otherwise.
> +   */
> +  _shouldArchivePings: function() {
> +    return Preferences.get(PREF_ARCHIVE_ENABLED, true);

can you confirm my understanding of the rules for archiving & recording?

- We record base Telemetry data if the FHR feature is enabled in this build
---> i.e. datareporting.healthreport.service.enabled

- We record extended Telemetry data if user has consented to Telemetry and the FHR feature is enabled in this build and (this is an official build or we're  in test mode)
---> i.e. toolkit.telemetry.enabled AND datareporting.healthreport.service.enabled AND (isOfficialTeleemtry || TelemetryPing._testMode)

- We send the data we record if user has consented to FHR
---> i.e. datareporting.healthreport.uploadEnabled

- We archive pings if the FHR feature is enabled in this build (implicitly) and if toolkit.telemetry.archive.enabled is set?
---> i.e. datareporting.healthreport.service.enabled + toolkit.telemetry.archive.enabled

Btw, did you declare the default value of toolkit.telemetry.archive.enabled in prefs.js/all.js?

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ -1477,5 @@
>  
>    /**
> -   * Perform telemetry initialization for either chrome or content process.
> -   */
> -  enableTelemetryRecording: function enableTelemetryRecording(testing) {

Nice to see this logic unified in one place (TelemetryPing) :)
Sorry, somehow my last review got duplicated in addition to my latest questions :(
Just the new stuff:

can you confirm my understanding of the rules for archiving & recording?

- We record base Telemetry data if the FHR feature is enabled in this build
---> i.e. datareporting.healthreport.service.enabled

- We record extended Telemetry data if user has consented to Telemetry and the FHR feature is enabled in this build and (this is an official build or we're  in test mode)
---> i.e. toolkit.telemetry.enabled AND datareporting.healthreport.service.enabled AND (isOfficialTeleemtry || TelemetryPing._testMode)

- We send the data we record if user has consented to FHR
---> i.e. datareporting.healthreport.uploadEnabled

- We archive pings if the FHR feature is enabled in this build (implicitly) and if toolkit.telemetry.archive.enabled is set?
---> i.e. datareporting.healthreport.service.enabled + toolkit.telemetry.archive.enabled

Btw, did you declare the default value of toolkit.telemetry.archive.enabled in prefs.js/all.js?
Comment on attachment 8589115 [details] [diff] [review]
part 2 - Add test coverage - v2

Review of attachment 8589115 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +267,5 @@
> +
> +  let now = new Date(2009, 10, 18, 12, 0, 0);
> +  fakeNow(now);
> +
> +  let getArchiveFilename = (uuid, date, type) => {

you should break this out into a helper function in TelemetryPing.jsm and just call it from the test, no point duplicating code

@@ +296,5 @@
> +  Preferences.set(PREF_ARCHIVE_ENABLED, false);
> +  pingId = yield sendPing(true, true);
> +  pingPath = getArchiveFilename(pingId, now, TEST_PING_TYPE);
> +  Assert.ok(!(yield OS.File.exists(pingPath)),
> +            "TelemetryPing must not archive pings if FHR is disabled.");

this message is wrong "TelemetryPing must not archive pings if the archive pref is disabled"

@@ +305,5 @@
> +
> +  // Make sure we don't assert anymore when receiving pings.
> +  now = new Date(2014, 06, 18, 22, 0, 0);
> +  fakeNow(now);
> +  gRequestIterator = Iterator(new Request());

how does the handler passed to registerPingHandler get cleared?
Attachment #8589115 - Flags: review?(vdjeric) → review+
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #48)
> Just the new stuff:
> 
> can you confirm my understanding of the rules for archiving & recording?
> 
> - We record base Telemetry data if the FHR feature is enabled in this build
> ---> i.e. datareporting.healthreport.service.enabled

Yes. To be more precise, we basically mirror that preference to canRecordBase.

> - We record extended Telemetry data if user has consented to Telemetry and
> the FHR feature is enabled in this build and (this is an official build or
> we're  in test mode)
> ---> i.e. toolkit.telemetry.enabled AND
> datareporting.healthreport.service.enabled AND (isOfficialTeleemtry ||
> TelemetryPing._testMode)

Yes.

> - We send the data we record if user has consented to FHR
> ---> i.e. datareporting.healthreport.uploadEnabled

Yes.

> - We archive pings if the FHR feature is enabled in this build (implicitly)
> and if toolkit.telemetry.archive.enabled is set?
> ---> i.e. datareporting.healthreport.service.enabled +
> toolkit.telemetry.archive.enabled
> Btw, did you declare the default value of toolkit.telemetry.archive.enabled
> in prefs.js/all.js?

Yes, that's how it behaves. I've not added a pref to prefs.js/all.js: if no "toolkit.telemetry.archive.enabled" is available |_shouldArchive| defaults to true.
Minor: refactors ping archive filename generation in an helper function to remove code duplication in tests.
Attachment #8589114 - Attachment is obsolete: true
Attachment #8591529 - Flags: review+
Attached patch part 2 - Add test coverage - v3 (obsolete) — Splinter Review
I've removed the duplicated filename generation code.

> how does the handler passed to registerPingHandler get cleared?

By the |Request| object, but I've clarified that in a comment.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=117dabcf445e
Attachment #8589115 - Attachment is obsolete: true
Attachment #8591530 - Flags: review+
Added the archiving preferences to all.js.
Attachment #8591529 - Attachment is obsolete: true
Attachment #8591676 - Flags: review+
Attached patch part 3 - Update the docs. (obsolete) — Splinter Review
Attachment #8591677 - Flags: review?(gfritzsche)
Comment on attachment 8591677 [details] [diff] [review]
part 3 - Update the docs.

Review of attachment 8591677 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/docs/pings.rst
@@ +13,5 @@
>  
>  Submission
>  ==========
>  
> +Pings are submitted via a common API on ``TelemetryPing``. They are sent to the server on official builds and during tests only if the data upload is enabled through the datareporting.healthreport.uploadEnabled preference. The API allows callers to choose a custom retention period that determines how long pings are kept on disk if submission wasn't successful.

Let's leave this documentation section high-level and add a later section/paragraph that details the exact circumstances.

Comment 48 looks like good points for structuring that, how about we add a "preferences" section at the end of this document?
Attachment #8591677 - Flags: review?(gfritzsche)
Attached patch part 3 - Update the docs. v2 (obsolete) — Splinter Review
Attachment #8591677 - Attachment is obsolete: true
Attachment #8591744 - Flags: review?(gfritzsche)
Comment on attachment 8591744 [details] [diff] [review]
part 3 - Update the docs. v2

Review of attachment 8591744 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/docs/pings.rst
@@ +41,5 @@
> +
> +To allow for cheaper lookup of archived pings, storage follows a specific naming scheme for both the directory and the ping file name: `<YYYY-MM>/<timestamp>.<UUID>.<type>.json`.
> +
> +* ``<YYYY-MM>`` - The subdirectory name, generated from the ping creation date.
> +* ``<timestamp>`` - The ping creation timestamp.

"Timestamp of the ping creation date."?
Attachment #8591744 - Flags: review?(gfritzsche) → review+
Thanks for the review!
Attachment #8591744 - Attachment is obsolete: true
Attachment #8591749 - Flags: review+
Keywords: checkin-needed
Depends on: 1154177
Depends on: 1154154
This was backed out for being the cause of bug 1154154 and very-likely the cause of bug 1154177 as well (based on retriggers).
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f35b7d9755a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla40 → ---
In this patch |enableTelemetryRecording| is also called when "app-startup" is notified, so that Telemetry.canRecord* flags are also updated for the content process (this was done by TelemetrySession before, and I mistakenly removed it in the previous patch). I've moved this bit to TelemetryPing to avoid exposing/duplicating the |enableTelemetryRecording| function in TelemetrySession again.
Attachment #8591676 - Attachment is obsolete: true
Attachment #8593410 - Flags: review?(vdjeric)
Comment on attachment 8593410 [details] [diff] [review]
part 1 - Allow pings to be archived - v9

Review of attachment 8593410 [details] [diff] [review]:
-----------------------------------------------------------------

Why would only want to report child-process measurements if canRecordExtended=true? child telemetry should initialize on the same criteria as the parent and it should report individual histograms based on whether they are base/extended and user's opt-in/opt-out choices
Attachment #8593410 - Flags: review?(vdjeric) → review-
Thanks, good point. I've restored the behaviour.
Attachment #8593410 - Attachment is obsolete: true
Attachment #8593442 - Flags: review?(vdjeric)
Attachment #8593442 - Flags: review?(vdjeric) → review+
Please note that this needs to land together with bug 1149980.
Keywords: checkin-needed
Hi Dexter, seems part 2 failed to apply:

adding 1137252 to series file
renamed 1137252 -> bug1137252_tests.patch
applying bug1137252_tests.patch
patching file toolkit/components/telemetry/tests/unit/head.js
Hunk #1 FAILED at 128
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/tests/unit/head.js.rej
patching file toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js
Hunk #1 FAILED at 12
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug1137252_tests.patch

maybe need a rebase or so ?
Flags: needinfo?(alessio.placitelli)
Keywords: checkin-needed
Rebased the patch.
Attachment #8591530 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Attachment #8593797 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/810cafba6ece
https://hg.mozilla.org/mozilla-central/rev/a49e32375924
https://hg.mozilla.org/mozilla-central/rev/0c8c80a675d3
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.