1103101 - Start the telemetry module in content processes

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Right now Telemetry relies on profile-after-change/profile-before-change events to start/stop, but that doesn't work in content processes because they don't have profiles. We should switch to other events instead.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use DeferredTask in TelemetryPing (v1)

We should use DeferredTask instead of creating a timer ourselves.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Start Telemetry on content process startup (v1)

This patch takes care of Telemetry startup in the content process by using a separate event (app-startup). Clean-up and sending ping to chrome process on shutdown will come in a different patch.

Comment 3

[Vladan Djeric (:vladan)]

Comment on attachment 8529245 [details] [diff] [review]
Start Telemetry on content process startup (v1)

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +914,5 @@
>  
>    /**
> +   * Perform telemetry initialization for either chrome or content process.
> +   */
> +  enableTelemetry: function enableTelemetry(testing) {

Maybe call it "enableTelemetryRecording"?

@@ -913,5 @@
>     */
>    setup: function setup(aTesting) {
>      // Initialize some probes that are kept in their own modules
>      this._thirdPartyCookies = new ThirdPartyCookieProbe();
>      this._thirdPartyCookies.init();

I think this belongs in the content process

@@ +1030,5 @@
> +    if (!this.enableTelemetry()) {
> +      return;
> +    }
> +
> +    this.observe(null, "sessionstore-windows-restored", null);

1) Shouldn't we set "this._hasWindowRestoredObserver = true" here?

2) I think we should observe "xul-window-visible" in the content process as well.. Does this event not exist in the content process? Is there an equivalent "first-paint" event?

@@ +1139,1 @@
>        return this.setup();

consider calling it setupParentProcess() or setupParentTelemetry() instead of setup()

@@ +1158,5 @@
>        }
>        break;
>      case "sessionstore-windows-restored":
> +      if (this._hasWindowRestoredObserver) {
> +        Services.obs.removeObserver(this, "sessionstore-windows-restored");

why is this removeObserver protected by an if-check now? We wouldn't get the sessionstore-windows-restored event unless an observer was registered, right?

Comment 4

[Vladan Djeric (:vladan)]

Did you test out these patches in a build?

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1103101 - Start Telemetry on content process startup;

Addressed comments from the review, and I tested the patches to make sure they work.

(In reply to Vladan Djeric (:vladan) from comment #3)
> Comment on attachment 8529245 [details] [diff] [review]
> Start Telemetry on content process startup (v1)
> 
> Review of attachment 8529245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ -913,5 @@
> >     */
> >    setup: function setup(aTesting) {
> >      // Initialize some probes that are kept in their own modules
> >      this._thirdPartyCookies = new ThirdPartyCookieProbe();
> >      this._thirdPartyCookies.init();
> 
> I think this belongs in the content process

Isn't networking limited to the chrome process? I did rename setup to setupChromeProcess.

> @@ +1030,5 @@
> > +    if (!this.enableTelemetry()) {
> > +      return;
> > +    }
> > +
> > +    this.observe(null, "sessionstore-windows-restored", null);
> 
> 1) Shouldn't we set "this._hasWindowRestoredObserver = true" here?

We actually just simulate "sessionstore-windows-restored", and we don't actually register for it in the content process. I added a comment to explain that.

> 2) I think we should observe "xul-window-visible" in the content process as
> well.. Does this event not exist in the content process? Is there an
> equivalent "first-paint" event?

I saw that our "xul-window-visible" handler only records some startup I/O telemetry, which is not relevant to the content process, so I didn't include it. Also added a comment.

> @@ +1158,5 @@
> >        }
> >        break;
> >      case "sessionstore-windows-restored":
> > +      if (this._hasWindowRestoredObserver) {
> > +        Services.obs.removeObserver(this, "sessionstore-windows-restored");
> 
> why is this removeObserver protected by an if-check now? We wouldn't get the
> sessionstore-windows-restored event unless an observer was registered, right?

Because we're simulating "sessionstore-windows-restored" in the content process and not actually registering for it, we need an additional check here.

Comment 6

[Vladan Djeric (:vladan)]

(In reply to Jim Chen [:jchen] from comment #5)
> Isn't networking limited to the chrome process? I did rename setup to
> setupChromeProcess.

I took a second look and you're right, these cookie histograms are accumulated in the parent process.

> We actually just simulate "sessionstore-windows-restored", and we don't
> actually register for it in the content process. I added a comment to
> explain that.

Gotcha. Still, it's a bit awkward to gather data by faking an event that doesn't even exist in the content process.
Also, will the content process even be able to get the I/O counter info it gets sandboxed?

I think we should just have setupContentProcess call gatherStartupHistograms() directly instead of sending the fake event. We don't need the child process's I/O counter or debuggerAttached info.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c361538514
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb52826fcfa

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/f2c361538514
https://hg.mozilla.org/mozilla-central/rev/ffb52826fcfa