Closed Bug 1103101 Opened 10 years ago Closed 10 years ago

Start the telemetry module in content processes

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
We should use DeferredTask instead of creating a timer ourselves.
Attachment #8529243 - Flags: review?(vdjeric)
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 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+
Attachment #8529243 - Flags: review?(vdjeric) → review+
Did you test out these patches in a build?
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)
(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.
Attachment #8534653 - Flags: review?(vdjeric) → review+
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.

Attachment

General

Created:
Updated:
Size: