Closed Bug 1156857 Opened 5 years ago Closed 4 years ago

Histograms seem to be gone in the child payload.

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
e10s m6+ ---
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: jaoo, Assigned: jimm)

References

Details

Attachments

(1 file, 6 obsolete files)

While working on bug 1115820 I've seen the histograms are gone in the child payload (no childPayloads[N].histograms).
See Also: → 1115820
Blocks: 1115820
Assignee: nobody → gfritzsche
Apparently after bug 1100501 we are not initializing the StatisticsRecorder in child processes anymore.
Blocks: 1100501
I can take this if need be. Are you planning on working on this soon Georg?
It's fine and an easy fix now that i know what caused it.
I'm just unsure where best to add the initializer. Is ScopedXREEmbed ok?
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Attachment #8598679 - Flags: review?(jmathies) → review+
Comment on attachment 8598679 [details] [diff] [review]
Fix StatisticsRecorder not being initialized in content processes

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

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +146,5 @@
>  		   nsIFile *aAppDirectory,
>  		   nsIDirectoryServiceProvider *aAppDirProvider)
>  {
> +  // This is needed by Telemetry to initialize histogram collection.
> +  gStatisticsRecorder = MakeUnique<base::StatisticsRecorder>();

I think we want to move this below the sInitCounter check which returns early.
Simple patch - this makes the xpchell head.js IPC helpers return promises. do_await_remote_message() message is trivial to change - only test_geolocation_reset_accuracy.js uses it and is fine with the change. run_test_in_child() has more tests already depending on the do_test_finished() call and the callback arg, so i'm leaving that for now.
Attachment #8599256 - Flags: review?(ted)
Roberto, can you review this?
Attachment #8599262 - Flags: review?(rvitillo)
Comment on attachment 8599256 [details] [diff] [review]
Make xpcshells run_test_in_child() and do_await_remote_message() return promises

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

In general I'm pro-Promise, but I think we should be using Promise directly and not any jsm wrappers.

::: testing/xpcshell/head.js
@@ +1246,2 @@
>  {
> +  var deferred = _PromiseUtils.defer();

PromiseUtils just seems to use ES6 Promises, so I think you should just use them directly:
https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/PromiseUtils.jsm#27

That would look something like:
return new Promise((resolve, reject) => {
  do_load_child_test_harness();
  // ...
  sendCommand(//...,
              () => {
    resolve();
    //...
  }
}
});

You could instead make this function call itself recursively, like
if (typeof optionalCallback == 'undefined') {
  return new Promise((resolve, reject) => {
    run_test_in_child(testFile, () => { resolve(); do_test_finished(); });
  });
}
// Existing rest of function.

...and not return a Promise when a callback is passed.
Attachment #8599256 - Flags: review?(ted)
Ok - i usually much prefer the readability with defer(), but in this case it's definitely simple enough.
Attachment #8599270 - Flags: review?(ted)
Attachment #8599256 - Attachment is obsolete: true
Attachment #8599270 - Flags: review?(ted) → review+
Status: NEW → ASSIGNED
Keywords: leave-open
Comment on attachment 8599262 [details] [diff] [review]
Add test coverage for e10s child histogram collection

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

::: toolkit/components/telemetry/tests/unit/head.js
@@ +189,5 @@
> +  Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> +  // Telemetry archiving should be on.
> +  Services.prefs.setBoolPref("toolkit.telemetry.archive.enabled", true);
> +}
> +

As discussed on IRC, we should make sure that changing the logging level is reflected both in the parent and the children, no matter when it happens.

::: toolkit/components/telemetry/tests/unit/test_ChildHistograms.js
@@ +81,5 @@
> +  Assert.ok("keyedHistograms" in payload.childPayloads[0], "Child payload should have keyed histograms.");
> +  check_histogram_values(payload.childPayloads[0]);
> +
> +  do_test_finished();
> +});

Could you also test registerAddonHistogram and getAddonHistogram in the child?
Attachment #8599262 - Flags: review?(rvitillo) → review-
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #14)
> Comment on attachment 8599262 [details] [diff] [review]
> Add test coverage for e10s child histogram collection
> 
> Review of attachment 8599262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/tests/unit/head.js
> @@ +189,5 @@
> > +  Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> > +  // Telemetry archiving should be on.
> > +  Services.prefs.setBoolPref("toolkit.telemetry.archive.enabled", true);
> > +}
> > +
> 
> As discussed on IRC, we should make sure that changing the logging level is
> reflected both in the parent and the children, no matter when it happens.

Doing a quick check here: we do the setup in TelemetryController configureLogging().
This is called in both content and chrome process and adds an observer for the logging prefs, so this is fine.
(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> (In reply to Roberto Agostino Vitillo (:rvitillo) from comment #14)
> > Comment on attachment 8599262 [details] [diff] [review]
> > Add test coverage for e10s child histogram collection
> > 
> > Review of attachment 8599262 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/telemetry/tests/unit/head.js
> > @@ +189,5 @@
> > > +  Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> > > +  // Telemetry archiving should be on.
> > > +  Services.prefs.setBoolPref("toolkit.telemetry.archive.enabled", true);
> > > +}
> > > +
> > 
> > As discussed on IRC, we should make sure that changing the logging level is
> > reflected both in the parent and the children, no matter when it happens.
> 
> Doing a quick check here: we do the setup in TelemetryController
> configureLogging().
> This is called in both content and chrome process and adds an observer for
> the logging prefs, so this is fine.

OK, so for this test it's fine but what about the more general case? Do we have a listener somewhere in case the preference is changed afterwards?
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #16)
> > > As discussed on IRC, we should make sure that changing the logging level is
> > > reflected both in the parent and the children, no matter when it happens.
> > 
> > Doing a quick check here: we do the setup in TelemetryController
> > configureLogging().
> > This is called in both content and chrome process and adds an observer for
> > the logging prefs, so this is fine.
> 
> OK, so for this test it's fine but what about the more general case? Do we
> have a listener somewhere in case the preference is changed afterwards?

Yes, that's not specific to this test. This is part of the standard Telemetry module setup - TelemetryController being the central module and setting up the logging & observer on "profile-after-change" (chrome) and "app-startup" (content)
Flags: needinfo?(jmathies)
Assignee: gfritzsche → jmathies
Flags: needinfo?(jmathies)
Attached patch wip (obsolete) — Splinter Review
I think all we need to care about here is XRE_InitChildProcess.
Attachment #8598679 - Attachment is obsolete: true
Attachment #8600873 - Flags: review?(gfritzsche)
(In reply to Jim Mathies [:jimm] from comment #21)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=60427fb7c8e1

I think we're ok here, by adding this to XRE_InitChildProcess it gets initialized earlier and shuts regardless of which process object we create.
Comment on attachment 8600873 [details] [diff] [review]
wip

This avoid late freeing of this statistics memory which our memory alloocator doesn't like. We've already patched the main process and xpcshell in bug 1100501. This patch covers all content and plugin processes.
Attachment #8600873 - Flags: review?(gfritzsche) → review?(benjamin)
review ping.. bsmedberg, any issues here?
This bug is blocking bug 1115820 to land. I don't want to put pressure on anyone but I'd like to know if there is any ETA here. Thanks guys!
Comment on attachment 8600873 [details] [diff] [review]
wip

Why is this a global? It doesn't look like we need to use this as a global and could just make it a scoped variable.
Attachment #8600873 - Flags: review?(benjamin) → review-
Any updates here? This is pretty bad.
Per comment 25 and comment 27 setting ni to Jim, your feedback here would be much appreciated. Thanks.
Flags: needinfo?(jmathies)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #26)
> Comment on attachment 8600873 [details] [diff] [review]
> wip
> 
> Why is this a global? It doesn't look like we need to use this as a global
> and could just make it a scoped variable.

I don't think it makes any difference. We can try using as local. Seems fine either way though, not sure you're r- just for that.
Flags: needinfo?(jmathies)
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #8600873 - Attachment is obsolete: true
Comment on attachment 8611301 [details] [diff] [review]
patch v.2

updated to use a local variable.
Attachment #8611301 - Flags: review?(benjamin)
Attachment #8611301 - Flags: review?(benjamin) → review+
Attached patch patch v.2Splinter Review
Attachment #8599262 - Attachment is obsolete: true
Attachment #8599270 - Attachment is obsolete: true
Attachment #8611301 - Attachment is obsolete: true
Attachment #8611328 - Flags: review+
Re the patch obsoletion - i still intend to pick up the tests here now that it's fixed (currently we have no coverage of child payloads at all), but i can move that to a separate bug.
Depends on: 1169159
Blocks: 1169159
No longer depends on: 1169159
https://hg.mozilla.org/mozilla-central/rev/158cc43ff74c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.