Benjamin Smedberg Reporter
	Description • 9 years ago

Calling TelemetryPing.send within the first minute of running or from an xpcshell test will currently throw an exception (null _log). This is because TelemetryPing is initialized from the TelemetrySession startup timer.

Since external code will be calling TelemetryPing, it should either self-initialize or behave correctly when TelemetryPing.send is called, no matter the circumstances.

If telemetry collection is disabled for a product, TelemetryPing.send shouldn't throw but should silently drop the payload.

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

Actually i misunderstood your issue and this problem is xpcshell-specific:
TelemetryPing is initialized from "profile-after-change" and in xpcshell we don't get it.
The proper approach right now is to do |TelemetryPing.setup()| or |yield TelemetryPing.setup()| first in xpcshell.

If this is an issue we can just make TelemetryPing.send() trigger the setup() if needed.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 2 • 9 years ago

Another point is that we don't want to make TelemetryPing.send() to actually send early during startup.

So, we should make sure that if TelemetryPing.send(somePing) before the delayed TelemetryPing init adds somePing to the pending/outgoing pings.

If, in your test, you want to check that we send the ping you will still have to wait for the delayed TelemetryPing init (|yield TelemetryPing.setup()|).

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 3 • 9 years ago

So, excuse the noise here today. Checking through this again:
1) it's trivial to fix the log issue
2) it's easy to make pings get added to the pending pings before full init
3) potential issue: we may not have the client id cached yet and the ping requires it

We could just resolve 3) by waiting (non-blockingly) on the TelemetryPing delayed init and submit it afterwards.
The only concern there would be potential dataloss in case of crashes before the delayed init finished.

	Benjamin Smedberg Reporter
	Comment 4 • 9 years ago

xpcshell fires profile-do-change but not profile-after-change: http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#1167

I'm going to start out by making TelemetryStartup and TelemetryPing use profile-do-change instead of profile-after-change, unless you know of a reason that would be bad.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 5 • 9 years ago

We do not avoid accessing e.g. prefs from init on (e.g. toolkit.telemetry.cachedClientID) and files in the profile folder, from my understanding they are not available at profile-do-change:
https://developer.mozilla.org/en-US/docs/Observer_Notifications

	Benjamin Smedberg Reporter
	Comment 6 • 9 years ago

Prefs are available from profile-do-change. profile-after-change mainly exists because there were ordering dependencies between systems receiving profile-do-change.

Of course now profile-after-change has a category key but profile-do-change doesn't, and the xpcshell profile mocking only notifies profile-do-change and not profile-after-change. What a mess :-(

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 7 • 9 years ago

Ok, maybe we could just fix the more relevant issue if it doesn't blow up all the tests.

It probably makes sense to hang Telemetry off profile-do-change in the longer run, but i wouldn't like to run into Telemetry init issues due to this now.

Maybe i'll just check if we can fire profile-after-change in xpcshell without bigger issues and make a follow-up on switching over to profile-do-change?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 8 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> Maybe i'll just check if we can fire profile-after-change in xpcshell
> without bigger issues

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

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

Bug 885389 has differing opinion on this though, bringing up concerns about always firing profile-after-change for unnecessary initialization costs and potentially unrelated failures.

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

This makes the public Telemetry APIs work properly when called before delayed Telemetry initialization.

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

Comment on attachment 8594831 [details] [diff] [review]
Part 1: Rename TelemetryPing.jsm to TelemetryController.jsm

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

r+ if this is just s/TelemetryPing/TelemetryController/g, I didn't actually audit every line for presence of zero-days ;)

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

Comment on attachment 8594832 [details] [diff] [review]
Part 2: Introduce TelemetryController.submitExternalPing

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

Can you add a check for the clientID being added after Telemetry does deferred initialization?

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1442,5 @@
>        retentionDays: RETENTION_DAYS,
>        addClientId: true,
>        addEnvironment: true,
>      };
> +    return TelemetryController.submitExternalPing(getPingType(payload), payload, options);

why is TelemetrySession.send() forwarding to submitExternalPing? it's not external is it?

@@ +1930,5 @@
>        retentionDays: RETENTION_DAYS,
>        addClientId: true,
>        addEnvironment: true,
>      };
>  

ditto for _sendDailyPing

@@ +2010,5 @@
>        addClientId: true,
>        addEnvironment: true,
>        overrideEnvironment: oldEnvironment,
>      };
> +    TelemetryController.submitExternalPing(getPingType(payload), payload, options);

and _onEnvironmentChange

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

Comment on attachment 8594833 [details] [diff] [review]
Part 3: Make the public TelemetryPing API work properly for external callers

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

-

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +492,4 @@
>     * @param {String} aType The type of the ping.
>     * @param {Object} aPayload The actual data payload for the ping.
>     * @param {Object} [aOptions] Options object.
>     * @param {Boolean} [aOptions.addClientId=false] true if the ping should contain the client

should addClientId default be true?

@@ +505,2 @@
>  
> +    const pingData = this.assemblePing(aType, aPayload, aOptions);

hmm, but if Telemetry isn't initialized addClientID won't have any effect here, right?

@@ +509,2 @@
>      // Always persist the pings if we are allowed to.
>      let archivePromise = TelemetryArchive.promiseArchivePing(pingData)

will promiseArchivePing ever rely on clientID?

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

(In reply to Vladan Djeric (:vladan), please needinfo -- PTO April 25-May 3 from comment #14)
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1442,5 @@
> >        retentionDays: RETENTION_DAYS,
> >        addClientId: true,
> >        addEnvironment: true,
> >      };
> > +    return TelemetryController.submitExternalPing(getPingType(payload), payload, options);
> 
> why is TelemetrySession.send() forwarding to submitExternalPing? it's not
> external is it?

I decided to clean up TelemetryControllers external API to submitExternalPing() here, so TelemetrySession is using it for now.
The refactor to have TelemetryController collect the payload from TelemetrySession is a bigger change and i'm leaving that to bug 1156358.

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

(In reply to Vladan Djeric (:vladan), please needinfo -- PTO April 25-May 3 from comment #14)
> Can you add a check for the clientID being added after Telemetry does
> deferred initialization?

That works better in the 3rd patch ("Make the public TelemetryPing API work properly for external callers"), i will be handling it there.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 18 • 9 years ago

(In reply to Vladan Djeric (:vladan), please needinfo -- PTO April 25-May 3 from comment #15)
> ::: toolkit/components/telemetry/TelemetryController.jsm
> @@ +492,4 @@
> >     * @param {String} aType The type of the ping.
> >     * @param {Object} aPayload The actual data payload for the ping.
> >     * @param {Object} [aOptions] Options object.
> >     * @param {Boolean} [aOptions.addClientId=false] true if the ping should contain the client
> 
> should addClientId default be true?

I think no. The clientId has additional privacy implications and i think it's better to leave callers to explicitly request this than to opt-out.

> @@ +509,2 @@
> >      // Always persist the pings if we are allowed to.
> >      let archivePromise = TelemetryArchive.promiseArchivePing(pingData)
> 
> will promiseArchivePing ever rely on clientID?

No.

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

(In reply to Vladan Djeric (:vladan), please needinfo -- PTO April 25-May 3 from comment #15)
> @@ +505,2 @@
> >  
> > +    const pingData = this.assemblePing(aType, aPayload, aOptions);
> 
> hmm, but if Telemetry isn't initialized addClientID won't have any effect
> here, right?

The clientId will be available after "profile-after-change" (unless it is not cached yet, but we already decided to live with that corner-case for now).

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

Comment on attachment 8596554 [details] [diff] [review]
Part 3: Make the public TelemetryPing API work properly for external callers

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +509,2 @@
>      // Always persist the pings if we are allowed to.
>      let archivePromise = TelemetryArchive.promiseArchivePing(pingData)

promiseArchivePing doesn't discriminate based on ping type, so this will archive all pings, i guess a TODO for later

@@ +514,5 @@
> +
> +    if (!this._initialized) {
> +      // We are still initializing and should not send yet, add this to the pending pings.
> +      this._log.trace("submitExternalPing - still initializing, ping is pending");
> +      p.push(TelemetryStorage.addPendingPing(pingData));

Telemetry should trigger the sending of this pending ping as soon as it initializes

5:56 <gfritzsche> [it would get triggered at..] either the next send or the idle-daily operations
15:56 <gfritzsche> not quite ideal

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +167,5 @@
>     * @param {String} aFilePath The path to the ping file that needs to be added to the
>     *                           saved pings directory.
>     * @return {Promise} A promise resolved when the ping is saved to the pings directory.
>     */
> +  addPendingPingFromFile: function(aPingPath) {

style: you're mixing using the "a" argument prefix (addPendingPingFromFile) and not using it (addPendingPing) in this patch

@@ -432,5 @@
> -        pendingPings.push(ping);
> -        // Since we read a ping successfully, update the related histogram.
> -        Telemetry.getHistogramById("READ_SAVED_PING_SUCCESS").add(1);
> -        // Save the ping to the saved pings directory.
> -        return this.savePing(ping, false);

why was this ping being re-saved after being loaded?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 22 • 9 years ago

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

	Pulsebot
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/e469e38d7a92
https://hg.mozilla.org/integration/fx-team/rev/0a259ce19620
https://hg.mozilla.org/integration/fx-team/rev/fb7b661c3fc8

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 24 • 9 years ago

Fixes an oversight: addPending() already saves the ping, so this removes a redundant call.

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

https://hg.mozilla.org/mozilla-central/rev/e469e38d7a92
https://hg.mozilla.org/mozilla-central/rev/0a259ce19620
https://hg.mozilla.org/mozilla-central/rev/fb7b661c3fc8

	Pulsebot
	Comment 26 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/b30b620283bd

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

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