Benjamin Smedberg Reporter
	Description • 9 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 1 • 9 years ago

Talked about it with Alessio when coordinating, he has it on his radar.

	Alessio Placitelli [:Dexter] Assignee
	Comment 2 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 3 • 9 years ago

Sorry, |Preferences.ignore| was missing the third parameter.

	Alessio Placitelli [:Dexter] Assignee
	Comment 4 • 9 years ago

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

	Benjamin Smedberg Reporter
	Comment 5 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 6 • 9 years ago

(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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 7 • 9 years ago

Only allow sending pings if FHR uploads are enabled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1398adaded4c

	Vladan Djeric (:vladan)
	Comment 8 • 9 years ago

Should I review this patch?

	Alessio Placitelli [:Dexter] Assignee
	Comment 9 • 9 years ago

(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

	Vladan Djeric (:vladan)
	Comment 10 • 9 years ago

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

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 9 years ago

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?

	Georg Fritzsche [:gfritzsche]
	Comment 12 • 9 years ago

(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)

	Benjamin Smedberg Reporter
	Comment 13 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 14 • 9 years ago

What about other products though?

	Alessio Placitelli [:Dexter] Assignee
	Comment 15 • 9 years ago

(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?

	Georg Fritzsche [:gfritzsche]
	Comment 16 • 9 years ago

(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.

	Georg Fritzsche [:gfritzsche]
	Comment 17 • 9 years ago

Is this WIP with that question resolved then?

	Benjamin Smedberg Reporter
	Comment 18 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 19 • 9 years ago

So... we want it whenever we have FHR, enabled or not?

	Benjamin Smedberg Reporter
	Comment 20 • 9 years ago

Yes. Do we have a pref for "the FHR feature is enabled" that we could use/reuse for this purpose?

	Georg Fritzsche [:gfritzsche]
	Comment 21 • 9 years ago

We have |datareporting.healthreport.service.enabled|.
To handle DRS and FHR being stripped from builds we have |"@mozilla.org/datareporting/service;1" in Cc|.

	Alessio Placitelli [:Dexter] Assignee
	Comment 22 • 9 years ago

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).

	Alessio Placitelli [:Dexter] Assignee
	Comment 23 • 9 years ago

Also, Telemetry is now always enabled.

	Alessio Placitelli [:Dexter] Assignee
	Comment 24 • 9 years ago

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)

	Georg Fritzsche [:gfritzsche]
	Comment 25 • 9 years ago

(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/...?

	Vladan Djeric (:vladan)
	Comment 26 • 9 years ago

(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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 27 • 9 years ago

Clearing n? as Vladan already provided the required information.

	Georg Fritzsche [:gfritzsche]
	Comment 28 • 9 years ago

(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?

	Georg Fritzsche [:gfritzsche]
	Comment 29 • 9 years ago

(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.

	Vladan Djeric (:vladan)
	Comment 30 • 9 years ago

(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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 31 • 9 years ago

(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.

	Georg Fritzsche [:gfritzsche]
	Comment 32 • 9 years ago

(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).

	Georg Fritzsche [:gfritzsche]
	Comment 33 • 9 years ago

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

	Alessio Placitelli [:Dexter] Assignee
	Comment 34 • 9 years ago

This patch adds ping archiving and allows to send pings if only base data recording is enabled.

	Alessio Placitelli [:Dexter] Assignee
	Comment 35 • 9 years ago

This patch adds test coverage for pings archiving.

	Alessio Placitelli [:Dexter] Assignee
	Comment 36 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 37 • 9 years ago

(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.

	Georg Fritzsche [:gfritzsche]
	Comment 38 • 9 years ago

(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.

	Georg Fritzsche [:gfritzsche]
	Comment 39 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 40 • 9 years ago

Thanks Georg. As we discussed over IRC, this patch renames:

- addPendingPing to addPendingPingFromFile
- savePing to addPendingPing
- writePing to savePing

	Alessio Placitelli [:Dexter] Assignee
	Comment 41 • 9 years ago

Updated because of the changed function names.

	Vladan Djeric (:vladan)
	Comment 42 • 9 years ago

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) :)

	Alessio Placitelli [:Dexter] Assignee
	Comment 43 • 9 years ago

(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 :-)

	Vladan Djeric (:vladan)
	Comment 44 • 9 years ago

(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

	Alessio Placitelli [:Dexter] Assignee
	Comment 45 • 9 years ago

Comment on attachment 8589114 [details] [diff] [review]
part 1 - Allow pings to be archived - v7

r+ as per IRC talk.

	Vladan Djeric (:vladan)
	Comment 46 • 9 years ago

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) :)

	Vladan Djeric (:vladan)
	Comment 47 • 9 years ago

Sorry, somehow my last review got duplicated in addition to my latest questions :(

	Vladan Djeric (:vladan)
	Comment 48 • 9 years ago

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?

	Vladan Djeric (:vladan)
	Comment 49 • 9 years ago

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?

	Alessio Placitelli [:Dexter] Assignee
	Comment 50 • 9 years ago

(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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 51 • 9 years ago

Minor: refactors ping archive filename generation in an helper function to remove code duplication in tests.

	Alessio Placitelli [:Dexter] Assignee
	Comment 52 • 9 years ago

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

	Alessio Placitelli [:Dexter] Assignee
	Comment 53 • 9 years ago

Added the archiving preferences to all.js.

	Georg Fritzsche [:gfritzsche]
	Comment 55 • 9 years ago

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?

	Georg Fritzsche [:gfritzsche]
	Comment 57 • 9 years ago

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."?

	Alessio Placitelli [:Dexter] Assignee
	Comment 58 • 9 years ago

Thanks for the review!

	Georg Fritzsche [:gfritzsche]
	Comment 59 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/1e0fa4ab7c6f
https://hg.mozilla.org/integration/fx-team/rev/0c3d670f0c14
https://hg.mozilla.org/integration/fx-team/rev/bbb1b526cb56

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 60 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1e0fa4ab7c6f
https://hg.mozilla.org/mozilla-central/rev/0c3d670f0c14
https://hg.mozilla.org/mozilla-central/rev/bbb1b526cb56

	Ryan VanderMeulen [:RyanVM]
	Comment 61 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 9 years ago

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/6f35b7d9755a

	Alessio Placitelli [:Dexter] Assignee
	Comment 63 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 64 • 9 years ago

The try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f052da9837b2

	Vladan Djeric (:vladan)
	Comment 65 • 9 years ago

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

	Alessio Placitelli [:Dexter] Assignee
	Comment 66 • 9 years ago

Thanks, good point. I've restored the behaviour.

	Alessio Placitelli [:Dexter] Assignee
	Comment 67 • 9 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc48327ef694

	Alessio Placitelli [:Dexter] Assignee
	Comment 68 • 9 years ago

Please note that this needs to land together with bug 1149980.

	Carsten Book [:Tomcat]
	Comment 69 • 9 years ago

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 ?

	Alessio Placitelli [:Dexter] Assignee
	Comment 70 • 9 years ago

Rebased the patch.

	Pulsebot
	Comment 71 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/810cafba6ece
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49e32375924
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8c80a675d3

	Carsten Book [:Tomcat]
	Comment 72 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/810cafba6ece
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49e32375924
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8c80a675d3

	Ryan VanderMeulen [:RyanVM]
	Comment 73 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/810cafba6ece
https://hg.mozilla.org/mozilla-central/rev/a49e32375924
https://hg.mozilla.org/mozilla-central/rev/0c8c80a675d3