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.

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.

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

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.

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.

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

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

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?

(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

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.

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

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

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

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?

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

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

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

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

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?

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

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

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