Closed
Bug 1103101
Opened 10 years ago
Closed 10 years ago
Start the telemetry module in content processes
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(2 files, 1 obsolete file)
4.34 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
13.45 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
We should use DeferredTask instead of creating a timer ourselves.
Attachment #8529243 -
Flags: review?(vdjeric)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #8529245 -
Flags: review?(vdjeric)
Comment 3•10 years ago
|
||
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?
Attachment #8529245 -
Flags: review?(vdjeric) → review+
Updated•10 years ago
|
Attachment #8529243 -
Flags: review?(vdjeric) → review+
Comment 4•10 years ago
|
||
Did you test out these patches in a build?
Assignee | ||
Comment 5•10 years ago
|
||
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.
Attachment #8529245 -
Attachment is obsolete: true
Attachment #8534653 -
Flags: review?(vdjeric)
Comment 6•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8534653 -
Flags: review?(vdjeric) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f2c361538514
https://hg.mozilla.org/mozilla-central/rev/ffb52826fcfa
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•