1149754 - TelemetryPing API should work externally without special initialization

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Benjamin Smedberg]

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.

Comment 1

[Georg Fritzsche [:gfritzsche]]

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.

Comment 2

[Georg Fritzsche [:gfritzsche]]

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

Comment 3

[Georg Fritzsche [:gfritzsche]]

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.

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Georg Fritzsche [:gfritzsche]]

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

Comment 6

[Benjamin Smedberg]

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

Comment 7

[Georg Fritzsche [:gfritzsche]]

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?

Comment 8

[Georg Fritzsche [:gfritzsche]]

(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

Comment 9

[Georg Fritzsche [:gfritzsche]]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Rename TelemetryPing.jsm to TelemetryController.jsm

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2: Introduce TelemetryController.submitExternalPing

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3: Make the public TelemetryPing API work properly for external callers

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

Comment 13

[Vladan Djeric (:vladan)]

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 14

[Vladan Djeric (:vladan)]

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 15

[Vladan Djeric (:vladan)]

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?

Comment 16

[Georg Fritzsche [:gfritzsche]]

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

Comment 17

[Georg Fritzsche [:gfritzsche]]

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

Comment 18

[Georg Fritzsche [:gfritzsche]]

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

Comment 19

[Georg Fritzsche [:gfritzsche]]

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

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3: Make the public TelemetryPing API work properly for external callers

Comment 21

[Vladan Djeric (:vladan)]

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?

Comment 22

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3: Make the public TelemetryPing API work properly for external callers

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

Comment 23

[Pulsebot]

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

Comment 24

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 4: Fix duplicated ping saving

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

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Pulsebot]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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