Closed Bug 1123384 Opened 7 years ago Closed 7 years ago

Move Telemetry main ping implementation out of TelemetryPing.jsm

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

Attachments

(1 file, 10 obsolete files)

121.41 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
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.
Vladan, is that ok with you?
Flags: needinfo?(vdjeric)
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"?
Flags: needinfo?(vdjeric)
Oic, "main ping" is terminology from the FHR/Telemetry re-design document
(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.
Assignee: nobody → alessio.placitelli
Changes:

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

Notes: test_telemetryPing test is timing out.
Attachment #8552592 - Attachment is obsolete: true
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
Attachment #8553072 - Attachment is obsolete: true
Attachment #8553101 - Flags: review?(gfritzsche)
I've updated the patch to build against latest m-c.
Attachment #8553101 - Attachment is obsolete: true
Attachment #8553101 - Flags: review?(gfritzsche)
Attachment #8553204 - Flags: review?(gfritzsche)
I aborted the previous try-run, here's the new one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b2a667a1e0
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
Attachment #8553204 - Attachment is obsolete: true
Attachment #8553204 - Flags: review?(gfritzsche)
Attachment #8553293 - Flags: review?(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?
Attachment #8553293 - Flags: review?(gfritzsche) → feedback+
(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.
Flags: needinfo?(dteller)
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
Attachment #8553293 - Attachment is obsolete: true
Attachment #8554171 - Flags: review?(gfritzsche)
This patch fixes TelemetrySession usage in tests without previous initialisation.
Attachment #8554171 - Attachment is obsolete: true
Attachment #8554172 - Attachment is obsolete: true
Attachment #8554173 - Attachment is obsolete: true
Attachment #8554171 - Flags: review?(gfritzsche)
Attachment #8554475 - Flags: review?(gfritzsche)
|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.
Attachment #8554475 - Attachment is obsolete: true
Attachment #8554475 - Flags: review?(gfritzsche)
Attachment #8554506 - Flags: review?(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.
Attachment #8554506 - Flags: review?(gfritzsche) → review+
- Fixes test_TelemetryTimestamps.js, test_TelemetryPing.js tests;
- Changed browser_ui_telemetry.js
Attachment #8554506 - Attachment is obsolete: true
Attachment #8555776 - Flags: review?(gfritzsche)
Attachment #8555776 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/133d1d99d3e8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Sorry, for some reason, this needinfo didn't show up in my dashboard until today. Do you still need the answer?
Flags: needinfo?(dteller) → needinfo?(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?
Flags: needinfo?(gfritzsche)
(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.
Depends on: 1161764
You need to log in before you can comment on or make changes to this bug.