Closed Bug 1122061 Opened 5 years ago Closed 5 years ago

Give TelemetryPing a common API for sending pings

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

(Whiteboard: [ready])

Attachments

(2 files, 20 obsolete files)

29.73 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
78.02 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
We want to have a proper API for sending pings based on the format in bug 1120981.

This should take:
* type - a string naming the type of the ping
* addClientId - bool on whether the ping should add the client id
* addEnvironment - bool on whether the ping should add the environment data
(In reply to Georg Fritzsche [:gfritzsche] [slow to respond until Jan 19] from comment #0)
> This should take:

* type - a string naming the type of the ping
* addClientId - bool on whether the ping should add the client id
* addEnvironment - bool on whether the ping should add the environment data
* payload - the actual data payload for the ping
Also think about how you're going to specify retention: some kinds of pings will have longer retention periods than others, and some kinds of pings may have browsing history in them which should be deleted if you clear history.
So, assuming you mean local retention:
We should probably default to "no retention" for all ping types and require explicit mention of longer ones.

A resulting question from that:
How long can we keep pings cached on submission failure for retrying later?
I think we should require a retention period: it will never be 0. I think we should keep pings as long as the retention period for retry on submission failure.
Duplicate of this bug: 1122063
Right now we build the submission path/URL based on the |reason| (among other things).
The |reason| will now be a payload-specific field and hence be opaque for that part of the code.

We can include the ping type instead ("main", "activaction", "upgrade", "delete", ...).

Mark, does that sound good? Any other input or changes for the submission path?
Flags: needinfo?(mreid)
Yes, we started overloading "reason" to mean ping type for some data already (FxOS First time use ping, Flash video ping, etc), so I think this is a good idea.

One thing that's been a bit annoying is that the set of allowed values for that field is a fixed list, anything not enumerated in the server config gets bucketed into "OTHER".  We should probably change that to just allow any value for that field, and monitor the data to make sure we're not getting tons of bogus values for it.
Flags: needinfo?(mreid)
It should definitely be the ping type.

However, I don't think we should store unrecognized data. If we receive unknown ping types I recommend that we throw them away and instead log a warning. If we start receiving large numbers of unknown pings, it should alert.
Side-note: I think we should use the opportunity of the test-updates here to split them into test_TelemetryPing.js and test_TelemetrySession.js.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Another detail that came up about the submission path:
Currently we use the "slug" as part of the submission path, which is persistent over a session. [0]
However, we want to send different, potentially more privacy-sensitive pings with the same API (e.g. CertViolation).

Can we get rid of the slug in the submission path? Or just use the ping id in its place?

[0]: https://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/toolkit/components/telemetry/TelemetryPing.jsm#l210
Flags: needinfo?(mreid)
"slug"? The submission path contains the ping ID but not the profile ID, so I don't think there's anything privacy-leaking about it.
Right now it contains a "slug" from the current implementation. It is a effectively a uuid that persists per browser session, not a ping id:
https://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/toolkit/components/telemetry/TelemetrySession.jsm#l272
https://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/toolkit/components/telemetry/TelemetrySession.jsm#l842

We'll not continue to use that. The question is whether we can just stop having a id in the path or not.
The ID that we should be sending is the ping ID, primarily to make sure that we're dealing with deduplication usefully.
The ID in the URL should be a document ID, not a session ID.  As Benjamin stated, the intent is to be able to recognize on the server when the client sends the same document more than once (if it missed the server's "OK" response and thus resubmitted, for example).
Flags: needinfo?(mreid)
Rebased the patch. Introduced |sendPersistedPings| and changed the serialization logic.
Attachment #8562025 - Attachment is obsolete: true
Attachment #8562664 - Flags: review?(gfritzsche)
Split tests from test_TelemetryPings.js in two files, leaving only the ones related to the common Ping API in  test_TelemetryPings.js.
Attachment #8562047 - Attachment is obsolete: true
Attachment #8562665 - Flags: review?(gfritzsche)
New test_TelemetryPing.js vs old test_TelemetryPing.js diff to ease reviewing.
New test_TelemetrySession.js vs old test_TelemetryPing.js diff to ease reviewing.
Attachment #8562666 - Attachment is obsolete: true
Comment on attachment 8562664 [details] [diff] [review]
part 1 - Give TelemetryPing a common API to handle pings (v2).

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +137,5 @@
>  
>    /**
>     * Send payloads to the server.
>     */
> +  send: function(aType, aPayload, aRetention, aAddClientId, aAddEnvironment) {

Document the arguments.

Also, i think it would be nice if we switch to an optional params object instead, i.e. enabling:
TelemetryPing.send("main", payload, { retention: 180, addClientId: true });
... with retention, addClientId, addEnvironment being optional in the params object and defaulting to reasonable values.

That would make it easier to e.g. add params and make the call-sites more clear.

@@ +144,5 @@
> +
> +  /**
> +   * Add the ping to the pending ping list and save all pending pings.
> +   */
> +  savePendingPings: function(aType, aPayload, aRetention, aAddClientId, aAddEnvironment) {

Params object too?
Document the arguments.

@@ +151,5 @@
> +
> +  /**
> +   * Save a ping to disk.
> +   */
> +  savePing: function(aType, aPayload, aRetention, aAddClientId, aAddEnvironment, aOverwrite) {

Params object too for all except type & payload?
Document the arguments.

@@ +158,5 @@
> +
> +  /**
> +   * Only used for testing. Saves a ping to disk with a specific file name and path.
> +   */
> +  testSavePingToFile: function(aType, aPayload, aAddClientId, aAddEnvironment, aOverwrite,

Params object too for all except type & payload?
Document the arguments.

@@ +182,5 @@
>    // The previous build ID, if this is the first run with a new build.
>    // Undefined if this is not the first run, or the previous build ID is unknown.
>    _previousBuildID: undefined,
>    _clientID: null,
> +  

Trailing whitespace.

@@ +184,5 @@
>    _previousBuildID: undefined,
>    _clientID: null,
> +  
> +  /**
> +   * Assemble a complete ping following the common ping format from bug 1120981.

Don't point to a specific bug here, this will become outdated. Also, we will have in-tree docs now.

@@ +221,5 @@
> +      id: generateUUID(),
> +      creationDate: (new Date()).toISOString(),
> +      version: PING_FORMAT_VERSION,
> +
> +      application: {

We can move those details to a separate function for readability.

@@ +241,5 @@
> +    }
> +
> +    if (aAddEnvironment) {
> +      return new Promise((resolve) => {
> +        TelemetryEnvironment.getEnvironmentData().then((environment) => {

Just:
return getEnvironmentData().then(environment => {
  pingData.environment = environment;
  return pingData;
});

What about rejections?
If we say getEnvironmentData() shouldn't reject, we should at least log an error on rejection.

@@ +288,5 @@
> +    this._log.trace("send - Type " + aType + ", Server " + this._server +
> +                    ", Retention " + aRetentionDays + ", aAddClientId " + aAddClientId +
> +                    ", aAddEnvironment " + aAddEnvironment);
> +
> +    return this.assemblePing(aType, aPayload, aAddClientId, aAddEnvironment)

Even if assemblePing should not reject, we need to at least log an error on rejection.

@@ +294,5 @@
> +          // Once ping is assembled, send it along with the persisted ping in the backlog.
> +          let p = [
> +            // Persist the ping if sending it fails.
> +            this.doPing(pingData, false)
> +                .then(null, () => TelemetryFile.savePing(pingData, true)),

Just use: .catch(rejectionHandler)

@@ +307,5 @@
> +   */
> +  sendPersistedPings: function sendPersistedPings() {
> +    this._log.trace("sendPersistedPings");
> +    let pingsIterator = Iterator(this.popPayloads());
> +    let p = [data for (data in pingsIterator)].map((data) => this.doPing(data, true));

We talked about this part, but i don't remember the details anymore - did we check that really all the pings except the currently submitted one are persisted to disk already?
It would be bad if we start losing pings here.

@@ +329,5 @@
> +                    ", Retention " + aRetentionDays + ", aAddClientId " + aAddClientId +
> +                    ", aAddEnvironment " + aAddEnvironment);
> +
> +    return this.assemblePing(aType, aPayload, aAddClientId, aAddEnvironment)
> +        .then((pingData) => { return TelemetryFile.savePendingPings(pingData); });

* No need for return: .then(pingData => TelemetryFile.save...);
* This needs a rejection handler.

@@ +351,5 @@
> +                    ", Retention " + aRetentionDays + ", aAddClientId " + aAddClientId +
> +                    ", aAddEnvironment " + aAddEnvironment + ", aOverwrite " + aOverwrite);
> +
> +    return this.assemblePing(aType, aPayload, aAddClientId, aAddEnvironment)
> +        .then((pingData) => { return TelemetryFile.savePing(pingData, aOverwrite); });

* No need for the {} and return.
* This needs a rejection handler.

@@ +374,5 @@
> +                    aAddEnvironment + ", aOverwrite " + aOverwrite + ", Name " + aFilePath);
> +
> +    return this.assemblePing(aType, aPayload, aAddClientId, aAddEnvironment)
> +        .then((pingData) => {
> +          return TelemetryFile.savePingToFile(pingData, aFilePath, aOverwrite);

* No need for the {} and return.
* This needs a rejection handler.

@@ +395,5 @@
>      }
>    },
>  
>    submissionPath: function submissionPath(ping) {
> +    this._log.trace("submissionPath - Ping available " + !!ping);

Less tracing.

@@ +433,3 @@
>        return function(event) {
> +        this.finishPingRequest(success, startTime, ping, isPersisted)
> +          .then(() => handleCompletion(event), (error) => {

Readability: Put the error handler on a new line.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +26,5 @@
>  })();
>  
>  // When modifying the payload in incompatible ways, please bump this version number
>  const PAYLOAD_VERSION = 1;
> +const PING_TYPE = "Main-Ping";

We're just calling it "main". See bug 1124212 / pings.rst for the ping types we want to use.

@@ +758,5 @@
>    send: function send(reason) {
>      this._log.trace("send - Reason " + reason);
>      // populate histograms one last time
>      this.gatherMemory();
> +    return TelemetryPing.send(PING_TYPE, this.getSessionPayload(reason),

Nit: store payload in temporary var.

@@ +935,5 @@
>  
>    savePendingPings: function savePendingPings() {
>      this._log.trace("savePendingPings");
> +    return TelemetryPing.savePendingPings(PING_TYPE,
> +      this.getSessionPayload("saved-session"), RETENTION_DAYS, true, false);

Nit: Store payload in temporary var?

@@ +940,4 @@
>    },
>  
>    testSaveHistograms: function testSaveHistograms(file) {
> +    this._log.trace("testSaveHistograms - File " + file.path);

"... - Path: " + file.path)

@@ +942,5 @@
>    testSaveHistograms: function testSaveHistograms(file) {
> +    this._log.trace("testSaveHistograms - File " + file.path);
> +    let payload = this.getSessionPayload("saved-session");
> +    return TelemetryPing.testSavePingToFile(PING_TYPE, payload, true, false, true,
> +                                            file.path);

This also needs to pass a retention.

@@ +1092,5 @@
>      //    it on the next backgrounding). Not deleting it is faster, so that's what we do.
>      case "application-background":
>        if (Telemetry.canSend) {
> +        let ping = this.getSessionPayload("saved-session");
> +        TelemetryPing.savePing(PING_TYPE, ping, RETENTION_DAYS, true, false, true);

s/ping/payload/
Attachment #8562664 - Flags: review?(gfritzsche)
This patch allows to pass an options object to the exposed method in TelemetryPing instead of a series of arguments.

This also deals with handling rejections as pointed out by the review.
Attachment #8562664 - Attachment is obsolete: true
Attachment #8563541 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> Comment on attachment 8562664 [details] [diff] [review]
> part 1 - Give TelemetryPing a common API to handle pings (v2).

Thank you for reviewing this, Georg!

> Review of attachment 8562664 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +307,5 @@
> > +   */
> > +  sendPersistedPings: function sendPersistedPings() {
> > +    this._log.trace("sendPersistedPings");
> > +    let pingsIterator = Iterator(this.popPayloads());
> > +    let p = [data for (data in pingsIterator)].map((data) => this.doPing(data, true));
> 
> We talked about this part, but i don't remember the details anymore - did we
> check that really all the pings except the currently submitted one are
> persisted to disk already?
> It would be bad if we start losing pings here.

Yes, we did check that: when sending a Ping TelemetryPing.jsm [1] iterates through the pending ping list loaded by TelemetryFile.jsm [2] at startup [3]. If |TelemetryPing.doPing()| fails (network down?), the Ping is yet again written to the disk, overwriting itself if it was already persisted [1]. When sending a ping succeeds, they get removed from disk [4].



[1] - https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/components/telemetry/TelemetryPing.jsm#l191
[2] - https://hg.mozilla.org/mozilla-central/annotate/34a66aaaca81/toolkit/components/telemetry/TelemetryFile.jsm#l248
[3] - https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/components/telemetry/TelemetryPing.jsm#l349
[4] - https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/components/telemetry/TelemetryPing.jsm#l204
Rebases the patch and fixes the ping type to match part 1.
Attachment #8562665 - Attachment is obsolete: true
Attachment #8562667 - Attachment is obsolete: true
Attachment #8562665 - Flags: review?(gfritzsche)
Attachment #8563558 - Flags: review?(gfritzsche)
Component: Client: Desktop → Telemetry
Product: Firefox Health Report → Toolkit
Comment on attachment 8563541 [details] [diff] [review]
part 1 - Give TelemetryPing a common API to handle pings (v3)

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

This looks good, not too many changes needed now.
I would prefer to take a last look though after the below changes, so cancelling review for now.

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +141,5 @@
> +   * @param aType A string naming the type of the ping
> +   * @param aPayload The actual data payload for the ping.
> +   * @param aOptions.retentionDays The number of days to keep the ping on disk if sending
> +   *        fails.
> +   * @param aOptions.addClientId true if the ping should add the client id, false otherwise.

Nit: s/add/contain/, for addEnvironment and in the comments below as well.

@@ +142,5 @@
> +   * @param aPayload The actual data payload for the ping.
> +   * @param aOptions.retentionDays The number of days to keep the ping on disk if sending
> +   *        fails.
> +   * @param aOptions.addClientId true if the ping should add the client id, false otherwise.
> +   * @param aOptions.addEnvironment true if the ping should add the environment data.

We should document that as one param aOptions and list the (optional) options after that:
@param aOptions ...
       * addClientId: ...

Also document default values.

@@ +149,3 @@
>     */
> +  send: function(aType, aPayload, aOptions) {
> +    let options = aOptions || {

This means i need to provide either all options or none. We would want each individually to be optional, so i could e.g. do |.send(t, p, {retentionDays:42})|.

E.g.:
send: function(aType, aPayload, aOptions = {}) {
  options.retentionDays = options.retentionDays || DEFAULT_RETENTION_DAYS;
  ...

@@ +149,4 @@
>     */
> +  send: function(aType, aPayload, aOptions) {
> +    let options = aOptions || {
> +      retentionDays: 180,

This needs to be a const. We should not increase the default retention now, we should default to what TelemetryFile uses:
https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/components/telemetry/TelemetryFile.jsm#l28

@@ +168,5 @@
> +   * @param aOptions.addEnvironment true if the ping should add the environment data.
> +   *
> +   * @returns {Promise} A promise that resolves when the pings are saved.
> +   */
> +  savePendingPings: function(aType, aPayload, aOptions) {

Ok, sorry for not bringing this up last round:
I don't think we should expose when to save pings, pending or otherwise.
When TelemetryPing shuts down, it should take care of persisting any pending pings itself.

That should leave us with only savePing() (and test functions as needed).

Please file a follow-up for that one (not blocking, so phase 4):
TelemetryPing should be able to decide internally whether it can still send a ping.
TelemetrySession and other callers should be able to just use TelemetryPing.send() and have the right thing happen.
That means that we have to make sure that TelemetrySession needs to properly synchronize with TelemetryPing. We may want to give TelemetryPing an AsyncShutdown barrier and hang TelemetrySession off that.

@@ +294,5 @@
> +  assemblePing: function assemblePing(aType, aPayload, aOptions) {
> +    let options = aOptions || {};
> +    this._log.trace("assemblePing - Type " + aType + ", Server " + this._server +
> +                    ", addClientId " + options.addClientId + ", addEnvironment " +
> +                    options.addEnvironment);

Can't we just dump the stringified options object?

@@ +359,5 @@
>     */
> +  send: function send(aType, aPayload, aOptions) {
> +    this._log.trace("send - Type " + aType + ", Server " + this._server +
> +                    ", Retention " + aOptions.retentionDays + ", aAddClientId " +
> +                    aOptions.addClientId + ", aAddEnvironment " + aOptions.addEnvironment);

Why not just stringify the options object?

@@ +403,5 @@
> +                    aOptions.addClientId + ", aAddEnvironment " + aOptions.addEnvironment);
> +
> +    return this.assemblePing(aType, aPayload, aOptions)
> +        .then(pingData => TelemetryFile.savePendingPings(pingData),
> +              (error) => this._log.error("savePendingPings - Rejection", error));

Nit: please be consistent, either |a => ...| or |(a) => ...|.
Ditto the uses below.

@@ +473,5 @@
>      }
>    },
>  
>    submissionPath: function submissionPath(ping) {
> +    let info = ping.application;

Nit: Lets rename this to e.g. app.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +27,5 @@
>  
>  // When modifying the payload in incompatible ways, please bump this version number
>  const PAYLOAD_VERSION = 1;
> +const PING_TYPE = "main";
> +const RETENTION_DAYS = 180;

Lets stay with the current 14 days here until bug 1122481.
Attachment #8563541 - Flags: review?(gfritzsche)
Comment on attachment 8563560 [details] [diff] [review]
test_TelemetrySession.js_vs_test_test_TelemetryPing.js.diff

Hm, can you please do that in the usual patch format (unified with 8 lines, -U 8) and so that this shows up in the splinter review view with
* test_Telemetry.js.old on the left
* test_Telemetry.js.session on the right
(In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> * test_Telemetry.js.old on the left
> * test_Telemetry.js.session on the right

* test_TelemetryPing.js.old on the left
* test_TelemetrySession.js.new on the right
Comment on attachment 8563558 [details] [diff] [review]
part 2 - Split test_telemetryPing.js and test_telemetrySession.js (v2)

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

This looks ok, but holding on for the diff mentioned above to review test_TelemetrySession.js with less effort.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +133,5 @@
>    };
>  
> +  // Check that the ping contains all the mandatory fields.
> +  for (let f of MANDATORY_PING_FIELDS) {
> +    Assert.ok(f in aPing);

Either:
* test each field explicitly instead of iterating through lists or
* put the field names in the Assert message.

Otherwise we need to change and re-run the test first to figure out which field is broken (i.e. we can't just tell from a first look at an automation log etc.).

@@ +141,5 @@
> +  Assert.equal(aPing.version, PING_FORMAT_VERSION, "The ping must have the correct version.");
> +
> +  // Test the application section.
> +  for (let f in APPLICATION_TEST_DATA) {
> +    Assert.equal(aPing.application[f], APPLICATION_TEST_DATA[f]);

Ditto.

@@ +154,4 @@
>  
> +  // Check the clientId and environment fields, as needed.
> +  Assert.ok(("clientId" in aPing) == aHasClientId);
> +  Assert.ok(("environment" in aPing) == aHasEnvironment);

Assert.equal for both.

@@ +232,4 @@
>    }
>  });
>  
> +add_task(function* test_pingHasEnvironment() {

Also add a test_pingHasEnvironmentAndClientId().

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +72,4 @@
>  
> +      // We need to stub it this way, as we can't use TelemetryPing without
> +      // initialising. This tests require to initialise TelemetryPing after
> +      // creating the pings to send persisted ones.

I don't think we need to stub it this way.
Can't we just TelemetryPing.setup() & TelemetryPing.testSavePingToFile()?
Then they should get picked up by the other startTelemetry() calls below as before.
Attachment #8563558 - Flags: review?(gfritzsche)
Thanks for your time reviewing this.

I've changed the way default values are handled, modified the comments to conform to JSDoc as we discussed over IRC, changed TelemetrySession to always add Environment.
Attachment #8563541 - Attachment is obsolete: true
Attachment #8564982 - Flags: review?(gfritzsche)
Comment on attachment 8564982 [details] [diff] [review]
part 1 - Give TelemetryPing a common API to handle pings (v4)

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

This looks ok to me now.

Vladan, this is a pretty central change - you might want to take a look over this to spot check.
This doesn't really change the semantics (apart from required details like the submission path), instead it mostly refactors things so that we end up with a generic ping handling in TelemetryPing.jsm and have the Telemetry payload specific details contained in TelemetrySession.jsm.

Retention changes will come in bug 1122481.

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +142,5 @@
> +   *
> +   * @param {String} aType The type of the ping.
> +   * @param {Object} aPayload The actual data payload for the ping.
> +   * @param {Object} [aOptions] Options object.
> +   * @param {Number} [aOptions.retentionDays=DEFAULT_RETENTION_DAYS] The number of days to

Document the actual default number of days here... or document the value of the constant somewhere.
Attachment #8564982 - Flags: review?(gfritzsche)
Attachment #8564982 - Flags: review+
Attachment #8564982 - Flags: feedback?(vdjeric)
This patch adds more descriptive assertion messages and |test_pingHasEnvironmentAndClientId()|. It also fixes the |assertNotSaved| function in test_TelemetrySendOldPings.js not correctly checking for file existence on disk.

> I don't think we need to stub it this way.
> Can't we just TelemetryPing.setup() & TelemetryPing.testSavePingToFile()?
> Then they should get picked up by the other startTelemetry() calls below as before.

Among other things, we need to control the path the pings are written to. This is easy with |testSavePingToFile|, as we can specify the file path. Unfortunately, TelemetryFile.jsm relies on the pingId being the file name in order to cleanup persisted pings when they are successfully received by the server. If we don't use PingIds as ping file names, unless we change TelemetryFile.jsm, tests in test_TelemetrySendOldPings.js will fail.

One way to prevent this, is to manually control the "pingId" (which is generated, internally, by TelemetryPing.jsm). One approach could be to pass a "pingId" override to |testSavePingToFile|. Should we change the test function as mentioned or leave things as they are?
Attachment #8563558 - Attachment is obsolete: true
Attachment #8565059 - Flags: review?(gfritzsche)
I hope this is what you requested :)
Attachment #8563560 - Attachment is obsolete: true
Compared to v3, this changes |test_TelemetrySendOldPings.js| to use TelemetryPing to serialise pings on the disk, without stubbing them manually.
Attachment #8565059 - Attachment is obsolete: true
Attachment #8565063 - Attachment is obsolete: true
Attachment #8565059 - Flags: review?(gfritzsche)
Attachment #8565092 - Flags: review?(gfritzsche)
This patches fixes the bustages from this try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a693874a5b21

Changes:
* TelemetrySession.shutdown allows to choose if pings need to be serialized in tests.
* Tests outside of the Telemetry suite are not serializing pings on disk.
Attachment #8565092 - Attachment is obsolete: true
Attachment #8565092 - Flags: review?(gfritzsche)
Attachment #8565354 - Flags: review?(gfritzsche)
Changed an |Assert.ok(.. == ...)| to use Assert.equal (minor).
Attachment #8565354 - Attachment is obsolete: true
Attachment #8565354 - Flags: review?(gfritzsche)
Attachment #8565449 - Flags: review?(gfritzsche)
I think this is missing a newer version of part 1 that the current part 2 is based on.
Attached patch test-fix.patch (obsolete) — Splinter Review
0:10.44 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) 1424214512111	Toolkit.Telemetry	TRACE	TelemetryEnvironment::getEnvironmentData"
 0:10.44 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Error: "1424214512115	Toolkit.Telemetry	ERROR	TelemetryEnvironment::_isDefaultBrowser - Could not obtain shell service: TypeError: Cc['@mozilla.org/browser/shell-service;1'] is undefined (resource://gre/modules/TelemetryEnvironment.jsm:362:6) JS Stack trace: this.TelemetryEnvironment._isDefaultBrowser@TelemetryEnvironment.jsm:362:7 < this.TelemetryEnvironment._getSettings@TelemetryEnvironment.jsm:394:25 < this.TelemetryEnvironment._doGetEnvironmentData</sections.settings@TelemetryEnvironment.jsm:854:25 < this.TelemetryEnvironment._doGetEnvironmentData<@TelemetryEnvironment.jsm:867:25 < TaskImpl_run@Task.jsm:314:40 < TaskImpl_handleResultValue@Task.jsm:393:7 < TaskImpl_run@Task.jsm:322:13 < TaskImpl@Task.jsm:275:3 < createAsyncFunction/asyncFunction@Task.jsm:249:14 < this.TelemetryEnvironment.getEnvironmentData@TelemetryEnvironment.jsm:842:25 < assemblePing@TelemetryPing.jsm:329:14 < savePendingPings@TelemetryPing.jsm:424:12 < this.TelemetryPing<.savePendingPings@TelemetryPing.jsm:183:12 < savePendingPings@TelemetrySession.jsm:968:12 < Impl.shutdown@TelemetrySession.jsm:1123:1 < this.TelemetrySession<.shutdown@TelemetrySession.jsm:223:12 < test_savedSessionClientID@test_TelemetrySession.js:727:9 < TaskImpl_run@Task.jsm:314:40 < Handler.prototype.process@Promise-backend.js:867:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:746:7 < this.PromiseWalker.scheduleWalkerLoop/<@Promise-backend.js:688:37 < _do_main@head.js:207:5 < _execute_test@head.js:504:5 < @-e:1:1" {file: "resource://gre/modules/Log.jsm" line: 749}]"
 0:10.46 PROCESS_OUTPUT: Thread-1 (pid:59571) "1424214512151	Toolkit.Telemetry	TRACE	TelemetryEnvironment::_getGFXData - Only one display adapter detected."
Sorry, premature submit... With the above small patch i get the errors that are thrown in tests reduced to:

 0:10.71 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) 1424214683189	Toolkit.Telemetry	TRACE	TelemetryEnvironment::getEnvironmentData"
 0:10.71 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Error: "1424214683193	Toolkit.Telemetry	ERROR	TelemetryEnvironment::_isDefaultBrowser - Could not obtain shell service: TypeError: Cc['@mozilla.org/browser/shell-service;1'] is undefined (resource://gre/modules/TelemetryEnvironment.jsm:362:6) JS Stack trace: this.TelemetryEnvironment._isDefaultBrowser@TelemetryEnvironment.jsm:362:7 < this.TelemetryEnvironment._getSettings@TelemetryEnvironment.jsm:394:25 < this.TelemetryEnvironment._doGetEnvironmentData</sections.settings@TelemetryEnvironment.jsm:854:25 < this.TelemetryEnvironment._doGetEnvironmentData<@TelemetryEnvironment.jsm:867:25 < TaskImpl_run@Task.jsm:314:40 < TaskImpl_handleResultValue@Task.jsm:393:7 < TaskImpl_run@Task.jsm:322:13 < TaskImpl@Task.jsm:275:3 < createAsyncFunction/asyncFunction@Task.jsm:249:14 < this.TelemetryEnvironment.getEnvironmentData@TelemetryEnvironment.jsm:842:25 < assemblePing@TelemetryPing.jsm:329:14 < savePendingPings@TelemetryPing.jsm:424:12 < this.TelemetryPing<.savePendingPings@TelemetryPing.jsm:183:12 < savePendingPings@TelemetrySession.jsm:968:12 < Impl.shutdown@TelemetrySession.jsm:1123:1 < this.TelemetrySession<.shutdown@TelemetrySession.jsm:223:12 < test_savedSessionClientID@test_TelemetrySession.js:727:9 < TaskImpl_run@Task.jsm:314:40 < Handler.prototype.process@Promise-backend.js:867:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:746:7 < this.PromiseWalker.scheduleWalkerLoop/<@Promise-backend.js:688:37 < _do_main@head.js:207:5 < _execute_test@head.js:504:5 < @-e:1:1" {file: "resource://gre/modules/Log.jsm" line: 749}]"
This fixes test_TelemetrySession.js so that no AddonManager exception pollutes the log.
Attachment #8565449 - Attachment is obsolete: true
Attachment #8565709 - Attachment is obsolete: true
Attachment #8565449 - Flags: review?(gfritzsche)
Attachment #8565939 - Flags: review?(gfritzsche)
Attachment #8564982 - Attachment is obsolete: true
Attachment #8564982 - Flags: feedback?(vdjeric)
Attachment #8565985 - Flags: review+
Attachment #8565985 - Flags: feedback?(vdjeric)
Updated to match latest part 2.
Attachment #8565096 - Attachment is obsolete: true
Comment on attachment 8565939 [details] [diff] [review]
part 2 - Split test_telemetryPing.js and test_telemetrySession.js (v7)

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

Looks good with the below fixed.

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +475,3 @@
>      return this.assemblePing(aType, aPayload, aOptions)
> +        .then(pingData => {
> +            return (aOptions.filePath) ?

Let's not use the ternary operator here, we don't need to.
Just use a clear if () ...

Also just ...then(() => pingData.id);

Would be much nicer if TelemetryFile.savePing would simply take an optional file param... but we can skip that now.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +206,5 @@
>  });
>  
>  // Checks that a sent ping is correctly received by a dummy http server.
>  add_task(function* test_simplePing() {
> +  // Setup the webserver. This will be used also in other tests.

Could just keep that in a separate function so it stands out more.

@@ +228,5 @@
>  
> +  if (HAS_DATAREPORTINGSERVICE &&
> +      Services.prefs.getBoolPref(PREF_FHR_UPLOAD_ENABLED)) {
> +    Assert.equal(ping.clientId, gDataReportingClientID,
> +                 "The correct clientId must be reported.");

Else branch for checking !("clientId" in ping)?

@@ +242,3 @@
>  
> +  // Test a field in the environment build section.
> +  Assert.equal(ping.application.buildId, ping.environment.build.buildId);

Start out with Assert.ok("environment" in ping);

@@ +245,4 @@
>  });
>  
> +add_task(function* test_pingHasEnvironmentAndClientId() {
> +  // Send a ping with the environment data.

... and client id.

@@ +253,3 @@
>  
> +  // Test a field in the environment build section.
> +  Assert.equal(ping.application.buildId, ping.environment.build.buildId);

Assert.ok("environment" in ping) first.

@@ +256,5 @@
> +  // Test that we have the correct clientId.
> +  if (HAS_DATAREPORTINGSERVICE &&
> +      Services.prefs.getBoolPref(PREF_FHR_UPLOAD_ENABLED)) {
> +    Assert.equal(ping.clientId, gDataReportingClientID,
> +                 "The correct clientId must be reported.");

Else branch for checking !("clientId" in ping)?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +58,2 @@
>   *
> + * @param {Array} aPingTypes An array of ping type objects. Each entry must be an

Rename this. It's more aPingDescriptions or aPingInfos?

@@ +66,2 @@
>   */
> +function createSavedPings(aPingTypes) {

let createSavedPings = Task.async(... ?

@@ +68,2 @@
>    return Task.spawn(function*(){
> +    let pingsIds = [];

Nit: pingIds.

@@ +96,3 @@
>   * @returns Promise
>   */
> +function clearPings(aPingIds) {

let clearPings = Task.async(... ?

@@ +131,3 @@
>   * @returns Promise
>   */
> +function assertNotSaved(aPingIds) {

let assertNotSaved = Task.async(... ?

@@ +204,5 @@
>    run_next_test();
>  }
>  
>  /**
> + * Setup the tests by making sure pings storage directory is available, otherwise

... the ping storage directory...

@@ +214,5 @@
> +  let directory = TelemetryFile.pingDirectoryPath;
> +  let pingDirectoryCreated = yield File.exists(directory);
> +
> +  if (!pingDirectoryCreated) {
> +    yield File.makeDir(directory, { unixMode: OS.Constants.S_IRWXU });

Use ignoreExisting:true and get rid of the pingDirectoryCreated.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +186,5 @@
> +  };
> +
> +  // Check that the ping contains all the mandatory fields.
> +  for (let f of MANDATORY_PING_FIELDS) {
> +    Assert.ok(f in aPing);

Give it a message that mentions f, so we can know what field failed.

@@ +212,5 @@
> +}
> +
> +function checkPayload(payload, reason, successfulPings) {
> +  Assert.ok(payload.simpleMeasurements.uptime >= 0);
> +  Assert.ok(payload.simpleMeasurements.startupInterrupted === 1);

Assert.equal

@@ +295,5 @@
> +    sum_squares_lo: successfulPings,
> +    sum_squares_hi: 0
> +  };
> +  let tc = payload.histograms[TELEMETRY_SUCCESS];
> +  //Assert.equal(uneval(tc), uneval(expected_tc));

Why is this commented out?
Attachment #8565939 - Flags: review?(gfritzsche) → review+
Thanks for the review, Georg. I've addressed all of your suggestions.

> @@ +228,5 @@
> >  
> > +  if (HAS_DATAREPORTINGSERVICE &&
> > +      Services.prefs.getBoolPref(PREF_FHR_UPLOAD_ENABLED)) {
> > +    Assert.equal(ping.clientId, gDataReportingClientID,
> > +                 "The correct clientId must be reported.");
> 
> Else branch for checking !("clientId" in ping)?

This shouldn't be needed, as this check is already done by the following lines in |checkPingFormat|:

  // Check the clientId and environment fields, as needed.
  Assert.equal("clientId" in aPing, aHasClientId);
  Assert.equal("environment" in aPing, aHasEnvironment);

> @@ +295,5 @@
> > +    sum_squares_lo: successfulPings,
> > +    sum_squares_hi: 0
> > +  };
> > +  let tc = payload.histograms[TELEMETRY_SUCCESS];
> > +  //Assert.equal(uneval(tc), uneval(expected_tc));
> 
> Why is this commented out?

Great catch! I commented this out to perform some tests and slipped through. Uncommenting, made the test fail. This is because expected_tc only accounted for 1 ping received, and we are sending two pings to fix the STARTUP* histgrams gathering in tests. I've adjusted |expected_tc| and it works now. I've f? you as this should have worked, as far as I understood, only by increasing successfulPings to 2. This wasn't the case, as I was getting:

Gathered:
{range:[1, 2], bucket_count:3, histogram_type:2, values:{0:2, 1:1, 2:0}, sum:1, sum_squares_lo:1, sum_squares_hi:0}
Expected:
{range:[1, 2], bucket_count:3, histogram_type:2, values:{0:1, 1:2, 2:0}, sum:2, sum_squares_lo:2, sum_squares_hi:0}
Attachment #8565939 - Attachment is obsolete: true
Attachment #8567084 - Attachment is obsolete: true
Attachment #8567223 - Flags: review+
Attachment #8567223 - Flags: feedback?(gfritzsche)
(In reply to Alessio Placitelli [:Dexter] from comment #46)
> > @@ +295,5 @@
> > > +    sum_squares_lo: successfulPings,
> > > +    sum_squares_hi: 0
> > > +  };
> > > +  let tc = payload.histograms[TELEMETRY_SUCCESS];
> > > +  //Assert.equal(uneval(tc), uneval(expected_tc));
> > 
> > Why is this commented out?
> 
> Great catch! I commented this out to perform some tests and slipped through.
> Uncommenting, made the test fail. This is because expected_tc only accounted
> for 1 ping received, and we are sending two pings to fix the STARTUP*
> histgrams gathering in tests. I've adjusted |expected_tc| and it works now.
> I've f? you as this should have worked, as far as I understood, only by
> increasing successfulPings to 2. This wasn't the case, as I was getting:
> 
> Gathered:
> {range:[1, 2], bucket_count:3, histogram_type:2, values:{0:2, 1:1, 2:0},
> sum:1, sum_squares_lo:1, sum_squares_hi:0}
> Expected:
> {range:[1, 2], bucket_count:3, histogram_type:2, values:{0:1, 1:2, 2:0},
> sum:2, sum_squares_lo:2, sum_squares_hi:0}

Ok, that looks like there is something wrong, we should catch up on this.
It looks like Telemetry.finishPingRequest() is getting |success==false| instead of true (counting 2 in the false bucket).
Whiteboard: [ready]
Attachment #8567223 - Flags: feedback?(gfritzsche)
Comment on attachment 8565985 [details] [diff] [review]
part 1 - Give TelemetryPing a common API to handle pings (v5)

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +381,5 @@
> +          let p = [
> +            // Persist the ping if sending it fails.
> +            this.doPing(pingData, false)
> +                .catch(() => TelemetryFile.savePing(pingData, true)),
> +            this.sendPersistedPings(),

are we going to keep the protections against loading too many saved pings, against sending old pings, and force the sending of overdue pings?
Attachment #8565985 - Flags: feedback?(vdjeric)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #48)
> are we going to keep the protections against loading too many saved pings, against sending old pings

Yes, this is handled by TelemetryFile.jsm .

> and force the sending of overdue pings?

Yes, overdue pings are loaded by TelemetryFile into the persisted pings stack. We load them when TelemetryPing inits and send them using |sendPersistedPings|. If sending them fails, we keep trying when sending other pings (we kept the same logic that was already in place).
https://hg.mozilla.org/mozilla-central/rev/8913bfcdb26a
https://hg.mozilla.org/mozilla-central/rev/f10934f68747
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.