1123384 - Move Telemetry main ping implementation out of TelemetryPing.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

To more clearly separate things out for the FHR/Telemetry unification changes, we want to:
* add a TelemetrySession.jsm
* move the data collection for the current Telemetry payload over there
* move the triggering of a "main" ping over as well

This allows us to reduce the TelemetryPing.jsm to the generic ping handling.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Vladan, is that ok with you?

Comment 2

[Vladan Djeric (:vladan)]

So if I understand correctly, you want TelemetryPing.jsm to handle uploading pings, saving pings at shutdown, deleting old saved pings from disk, etc? That seems fine.

What do you mean by "triggering of a main ping" and "main ping"?

Comment 3

[Vladan Djeric (:vladan)]

Oic, "main ping" is terminology from the FHR/Telemetry re-design document

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) from comment #2)
> What do you mean by "triggering of a main ping" and "main ping"?

Right, i'm not sure if that's the best terminology yet, but "main ping" or "session" etc. but be the ping type that contains the current Telemetry payload etc.

We create those on certain events or triggers: on shutdown, environment changes, 24h interval.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Split TelemetryPing and TelemetrySession (WIP)

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Split TelemetryPing and TelemetrySession (WIP v2)

Changes:

- TelemetrySession is not sending overdue pings anymore. This is handled by TelemetryPing.

Notes: test_telemetryPing test is timing out.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Split TelemetryPing and TelemetrySession (WIP v3)

This patch fixes the failing tests. We need to deal with the "submissionPath" used by TelemetryPing to submit the pings to the main server.

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

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Split TelemetryPing and TelemetrySession (WIP v4)

I've updated the patch to build against latest m-c.

Comment 9

[Alessio Placitelli [:Dexter]]

I aborted the previous try-run, here's the new one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b2a667a1e0

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Split TelemetryPing and TelemetrySession (WIP v5)

This fixes the test problems from the previous try run. Since I don't feel confident about this, I've only scheduled try-runs for linux and windows:

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

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8553293 [details] [diff] [review]
part 1 - Split TelemetryPing and TelemetrySession (WIP v5)

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

Side-note: Things like this are easiest reviewed with the help of original old file vs. new file (i.e. diff of old TelemetryPing.jsm vs new TelemetrySession.jsm).
Providing such a diff on the bug helps reviewers :)

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +262,4 @@
>    /**
>     * Initializes telemetry within a timer. If there is no PREF_SERVER set, don't turn on telemetry.
>     */
>    setupChromeProcess: function setupChromeProcess(testing) {

We shouldn't need to setup the TelemetryPing in content processes, so we can just rename this function.

@@ +277,5 @@
>      // lead to some stale client ids.
>      this._clientID = Preferences.get(PREF_CACHED_CLIENTID, null);
>  
>      AsyncShutdown.sendTelemetry.addBlocker(
>        "Telemetry: shutting down",

This only triggers uninstall(), which is empty right now.
Lets strip this and see what we need in later patches when we get there.

@@ +297,5 @@
>          // It doesn't really matter what we pass to this.send as a reason,
>          // since it's never sent to the server. All that this.send does with
>          // the reason is check to make sure it's not a test-ping.
> +        // TODO: we need to change the way submission URLs are handled.
> +        let submissionURL = "/submit/telemetry/";

Lets try to keep this working as is for now. See comment in TelemetrySession on submission path handling.

@@ +327,3 @@
>     * Remove observers to avoid leaks
>     */
>    uninstall: function uninstall() {

This empty now? Do we still need to unregister anything here?
If not, we don't need to keep an empty function?

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +9,5 @@
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Log.jsm");

This is probably meant to go in the logging patch?

@@ +31,5 @@
> +// This is the HG changeset of the Histogram.json file, used to associate
> +// submitted ping data with its histogram definition (bug 832007)
> +#expand const HISTOGRAMS_FILE_VERSION = "__HISTOGRAMS_FILE_VERSION__";
> +
> +const LOGGER_NAME = "toolkit.Telemetry";

Meant for the logging patch?

@@ +158,5 @@
> +  /**
> +   * Send a ping to a test server. Used only for testing.
> +   */
> +  testPing: function() {
> +    return Impl.testPing();

Side-note: I'm kind of wondering what we need the Impl separation for.
Apparently bug 913070 introduced this, i'm not sure yet why, so lets just move this over for now.

@@ +167,5 @@
> +   */
> +  getPayload: function() {
> +    return Impl.getPayload();
> +  },
> +  Constants: Object.freeze({

If you are reordering the members anyway, lets move this to the top of this object?

@@ +832,5 @@
> +  send: function send(reason) {
> +    // populate histograms one last time
> +    this.gatherMemory();
> +    // TODO: there should be a submission path per ping.
> +    let submissionPath = this.submissionPath();

Ok, if we do that then we can't land this ahead of the other changes because this breaks existing behavior.
How about we keep the submission path construction as is in TelemetryPing for now and remove that payload.info dependency in bug 1122061?

::: toolkit/components/telemetry/tests/unit/test_TelemetryLog.js
@@ +2,5 @@
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/TelemetryLog.jsm", this);
>  Cu.import("resource://gre/modules/TelemetryPing.jsm", this);

Unused.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +594,5 @@
>    // Get requests received by dummy server.
>    let request1 = yield gRequestIterator.next();
>    let request2 = yield gRequestIterator.next();
>  
> +  // We decode the first request to check for the reason.

I see us decoding both requests.

@@ +623,5 @@
>  });
>  
>  add_task(function* test_savedSessionClientID() {
>    // Assure that we store the ping properly when saving sessions on shutdown.
>    // We make the TelemetryPings shutdown to trigger a session save.

Update comment.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +71,1 @@
>      // clean up.

Hm, i don't see any service being created.
Old comment? Lets drop this.

@@ +173,5 @@
>    return deferred.promise;
>  }
>  
>  /**
> + * Teardown a TelemetrySession instance and clear out any pending

Something like "Reset telemetry state"?

@@ +190,5 @@
>   * Creates and returns a TelemetryPing instance in "testing"
>   * mode.
>   */
>  function startTelemetry() {
>    return TelemetryPing.setup();

Lets just do TelemetrySession.setup() in here too (either by chaining promises or taskifying) so we don't need have two startTelemetry*() lines in multiple places below.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +2395,5 @@
>        Services.appinfo.annotateCrashReport("Add-ons", data);
>      }
>      catch (e) { }
>  
> +    Cu.import("resource://gre/modules/TelemetrySession.jsm", {}).TelemetrySession.setAddOns(data);

This line is getting a bit long, wrap or use a temp variable?

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> @@ +158,5 @@
> > +  /**
> > +   * Send a ping to a test server. Used only for testing.
> > +   */
> > +  testPing: function() {
> > +    return Impl.testPing();
>
> Side-note: I'm kind of wondering what we need the Impl separation for.
> Apparently bug 913070 introduced this, i'm not sure yet why, so lets just
> move this over for now.

Yoric, do you remember the motivation here? I didn't see this being mentioned or discussed on bug 913070.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Split TelemetryPing and TelemetrySession v6

This patch implements the changes from the previous review. A try push can be found here:

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

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: New TelemetryPing.jsm - Old TelemetryPing.jsm diff

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: New TelemetrySession.jsm - Old TelemetryPing.jsm diff

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: Split TelemetryPing and TelemetrySession v7

This patch fixes TelemetrySession usage in tests without previous initialisation.

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: Split TelemetryPing and TelemetrySession v8

|setupChromeProcess|/|setupTelemetry| now return a resolved Promise when Telemetry is disabled (to be consistent with other values returned from the same function).

Fixed the toolkit\mozapps\extensions\test\browser\browser_bug557956.js test.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8554506 [details] [diff] [review]
Split TelemetryPing and TelemetrySession v8

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

Looks good with the below comments addressed.

::: browser/metro/base/tests/mochitest/browser_ui_telemetry.js
@@ +17,5 @@
>  
>  gTests.push({
>    desc: "Test browser-ui telemetry",
>    run: function testBrowserUITelemetry() {
> +    yield TelemetrySession.setup();

One setup() and one shutdown is presumably enough?
You could probably just add a |gTests.push({desc:"Setup"},...)| and a |gTests.push({desc:"Shutdown"},...)| ?

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +292,5 @@
>        if (TelemetryFile.pingsOverdue > 0) {
>          // It doesn't really matter what we pass to this.send as a reason,
>          // since it's never sent to the server. All that this.send does with
>          // the reason is check to make sure it's not a test-ping.
> +        yield this.send("overdue-flush", null);

Did this need to change for the setTestServer() approach?

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +221,5 @@
>    // Take off ["","submit","telemetry"].
>    let pathComponents = request.path.split("/").slice(3);
>  
>    checkPayloadInfo(payload, reason);
> +  Assert.ok(UUID_REGEX.test(pathComponents));

I'm surprised this passes. Shouldn't it be pathComponents[1]?

::: toolkit/modules/tests/xpcshell/test_TelemetryTimestamps.js
@@ +9,3 @@
>  
>  // The @mozilla/xre/app-info;1 XPCOM object provided by the xpcshell test harness doesn't
> +// implement the nsIAppInfo interface, which is needed by Services.jsm and TelemetrySession.jsm.

Nit: Line is getting wrapped, lets add a line-break.

@@ +31,5 @@
>    });
>  }
>  
>  function actualTest() {
> +  yield TelemetrySession.setup();

It looks like we're getting lucky with this passing.
This is not a task; we should just
* change it to add_task()
* adjust the above to |asyncFetchTelemetryData(run_next_test)|
* move the do_test_finished() somewhere appropriate.

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: Split TelemetryPing and TelemetrySession v9

- Fixes test_TelemetryTimestamps.js, test_TelemetryPing.js tests;
- Changed browser_ui_telemetry.js

Comment 20

[Alessio Placitelli [:Dexter]]

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

Comment 21

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/133d1d99d3e8

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/133d1d99d3e8

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Sorry, for some reason, this needinfo didn't show up in my dashboard until today. Do you still need the answer?

Comment 24

[Georg Fritzsche [:gfritzsche]]

We moved forward with this, but the question in comment 12 still stands.
From other bugs i assume this is due to worries about addons etc. becoming the dependent on the interface?

Comment 25

[David Teller [:Yoric] - still alive but not very active]

(In reply to Georg Fritzsche [:gfritzsche] [away Feb 27 - March 15] from comment #24)
> We moved forward with this, but the question in comment 12 still stands.
> From other bugs i assume this is due to worries about addons etc. becoming
> the dependent on the interface?

It's just a design pattern that's used by some of the code in toolkit/ to clearly hide the private variables. When porting the code to a jsm, I picked that design pattern, although other patterns would have worked, too.