1376600 - Review filtering of snapshots in TelemetrySession based on registered histograms

Assignee

Description

•

7 years ago

As we assemble the payload in TelemetrySession we ensure that the (keyed)histogram snapshots returned from C++ correspond to "registered" (keyed)histograms (which means they're specified in Histograms.json (I think that's the only source of truth for gHistogramInfos these days) and aren't expired)

We might not need to do this anymore. We may just need to double-check that no expired histograms are snapshotted and to conditionally filter out TELEMETRY_TEST histograms when we're not testing.

Georg Fritzsche [:gfritzsche]

Updated

•

7 years ago

Blocks: 1201022

Priority: -- → P3

Georg Fritzsche [:gfritzsche]

Comment 1

•

7 years ago

We should be able to remove nsITelemetry.registerHistograms() and .registeredKeyedHistograms() here.

Chris H-C :chutten

Assignee

Updated

•

7 years ago

Blocks: 1384175

Chris H-C :chutten

Assignee

Updated

•

7 years ago

Priority: P3 → P1

Chris H-C :chutten

Assignee

Updated

•

7 years ago

Assignee: nobody → chutten

Comment hidden (mozreview-request)

With the removal of the old addonHistograms, all histograms are now registered.
So removing registered(Keyed)Histograms should be straightforward?

Unfortunately not, as this was how we filtered data based on dataset
(opt-in/opt-out), so a little more fiddling was needed to get C++ to only
serialize dataset-appropriate data (instead of post-facto filtering it in JS).

Review commit: https://reviewboard.mozilla.org/r/171718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171718/

Georg Fritzsche [:gfritzsche]

Updated

•

7 years ago

Attachment #8900364 - Flags: review?(gfritzsche) → review?(alessio.placitelli)

Comment hidden (mozreview-request)

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171718/diff/1-2/

Chris H-C :chutten

Assignee

Comment 4

•

7 years ago

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

Clearing review as it is supposed to be redirected to :Dexter _and_ there are some test failures I missed. My regex-fu failed to find them the first time around :(

Attachment #8900364 - Flags: review?(gfritzsche)

Comment hidden (mozreview-request)

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171718/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171718/diff/3-4/

Alessio Placitelli [:Dexter]

Comment 7

•

7 years ago

mozreview-review

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

https://reviewboard.mozilla.org/r/171718/#review177260

Hey Chris, the patch looks ok. Don't get scared about the number of comments: most of them are related to changing the test code to use the telemetry defined constant for OPT_IN instead of the magic number 1.
The only reason why this is r- and not r+ is the open question on the Android support in Telemetry.cpp. Let's clear this out and fix the tests and we should be ready to land this.

::: dom/ipc/tests/browser_remote_navigation_delay_telemetry.js:16
(Diff revision 4)
>    await SpecialPowers.pushPrefEnv({set: [["toolkit.telemetry.enabled", true]]});
>    let canRecordExtended = Services.telemetry.canRecordExtended;
>    Services.telemetry.canRecordExtended = true;
>    registerCleanupFunction(() => Services.telemetry.canRecordExtended = canRecordExtended);
>  
> -  Services.telemetry.snapshotSubsessionKeyedHistograms(true /*clear*/);
> +  Services.telemetry.snapshotKeyedHistograms(1 /*opt-in*/, true /*subsession*/, true /*clear*/);

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: dom/ipc/tests/browser_remote_navigation_delay_telemetry.js:32
(Diff revision 4)
>    ok(tab2.linkedBrowser.isRemoteBrowser, "|tab2| should have a remote browser by now.");
>  
>    // There is no good way to make sure that the parent received the histogram entries from the child processes.
>    // Let's stick to the ugly, spinning the event loop until we have a good approach (Bug 1357509).
>    await BrowserTestUtils.waitForCondition(() => {
> -    let s = Services.telemetry.snapshotSubsessionKeyedHistograms().content["FX_TAB_REMOTE_NAVIGATION_DELAY_MS"];
> +    let s = Services.telemetry.snapshotKeyedHistograms(1, true, false).content["FX_TAB_REMOTE_NAVIGATION_DELAY_MS"];

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: dom/ipc/tests/browser_remote_navigation_delay_telemetry.js:36
(Diff revision 4)
>    await BrowserTestUtils.waitForCondition(() => {
> -    let s = Services.telemetry.snapshotSubsessionKeyedHistograms().content["FX_TAB_REMOTE_NAVIGATION_DELAY_MS"];
> +    let s = Services.telemetry.snapshotKeyedHistograms(1, true, false).content["FX_TAB_REMOTE_NAVIGATION_DELAY_MS"];
>      return s && "WebNavigation:LoadURI" in s && "SessionStore:restoreTabContent" in s;
>    });
>  
> -  let s = Services.telemetry.snapshotSubsessionKeyedHistograms().content["FX_TAB_REMOTE_NAVIGATION_DELAY_MS"];
> +  let s = Services.telemetry.snapshotKeyedHistograms(1, true, false).content["FX_TAB_REMOTE_NAVIGATION_DELAY_MS"];

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: dom/ipc/tests/browser_remote_navigation_delay_telemetry.js:45
(Diff revision 4)
>  
>    let loadURISnapshot = s["WebNavigation:LoadURI"];
>    ok(loadURISnapshot.sum > 0, "Zero delay for the LoadURI case is unlikely.");
>    ok(loadURISnapshot.sum < 10000, "More than 10 seconds delay for the LoadURI case is unlikely.");
>  
> -  Services.telemetry.snapshotSubsessionKeyedHistograms(true /*clear*/);
> +  Services.telemetry.snapshotKeyedHistograms(1, true, true /*clear*/);

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/extensions/test/xpcshell/head_telemetry.js:17
(Diff revision 4)
>  function arraySum(arr) {
>    return arr.reduce((a, b) => a + b, 0);
>  }
>  
>  function clearHistograms() {
> -  Services.telemetry.snapshotSubsessionHistograms(true);
> +  Services.telemetry.snapshotHistograms(1 /* opt-in */, true /* subsession */, true);

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/extensions/test/xpcshell/head_telemetry.js:21
(Diff revision 4)
>  function clearHistograms() {
> -  Services.telemetry.snapshotSubsessionHistograms(true);
> +  Services.telemetry.snapshotHistograms(1 /* opt-in */, true /* subsession */, true);
>  }
>  
>  function getSnapshots(process) {
> -  return Services.telemetry.snapshotSubsessionHistograms()[process];
> +  return Services.telemetry.snapshotHistograms(1, true, false)[process];

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/extensions/test/xpcshell/head_telemetry.js:30
(Diff revision 4)
>  // entries from the extension and content processes.
>  // Let's stick to the ugly, spinning the event loop until we have a good
>  // approach (Bug 1357509).
>  function promiseTelemetryRecorded(id, process, expectedCount) {
>    let condition = () => {
> -    let snapshot = Services.telemetry.snapshotSubsessionHistograms()[process][id];
> +    let snapshot = Services.telemetry.snapshotHistograms(1, true, false)[process][id];

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/osfile/tests/xpcshell/test_telemetry.js:30
(Diff revision 4)
>  // histograms
>  add_task(async function test_startup() {
>    let LAUNCH = "OSFILE_WORKER_LAUNCH_MS";
>    let READY = "OSFILE_WORKER_READY_MS";
>  
> -  let before = Services.telemetry.histogramSnapshots.parent;
> +  let before = Services.telemetry.snapshotHistograms(1, false, false).parent;

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/osfile/tests/xpcshell/test_telemetry.js:35
(Diff revision 4)
> -  let before = Services.telemetry.histogramSnapshots.parent;
> +  let before = Services.telemetry.snapshotHistograms(1, false, false).parent;
>  
>    // Launch the OS.File worker
>    await File.getCurrentDirectory();
>  
> -  let after = Services.telemetry.histogramSnapshots.parent;
> +  let after = Services.telemetry.snapshotHistograms(1, false, false).parent;

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/osfile/tests/xpcshell/test_telemetry.js:50
(Diff revision 4)
>  
>  // Ensure that calling writeAtomic adds data to the relevant histograms
>  add_task(async function test_writeAtomic() {
>    let LABEL = "OSFILE_WRITEATOMIC_JANK_MS";
>  
> -  let before = Services.telemetry.histogramSnapshots.parent;
> +  let before = Services.telemetry.snapshotHistograms(1, false, false).parent;

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/osfile/tests/xpcshell/test_telemetry.js:56
(Diff revision 4)
>  
>    // Perform a write.
>    let path = Path.join(Constants.Path.profileDir, "test_osfile_telemetry.tmp");
>    await File.writeAtomic(path, LABEL, { tmpPath: path + ".tmp" } );
>  
> -  let after = Services.telemetry.histogramSnapshots.parent;
> +  let after = Services.telemetry.snapshotHistograms(1, false, false).parent;

nit: let's use Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN instead of 1 here

::: toolkit/components/telemetry/Telemetry.cpp:581
(Diff revision 4)
> -NS_IMETHODIMP
> -TelemetryImpl::SnapshotSubsessionKeyedHistograms(bool clearSubsession,
> -                                                 JSContext *cx,
> -                                                 JS::MutableHandle<JS::Value> ret)
>  {
> -#if !defined(MOZ_WIDGET_ANDROID)
> +  return TelemetryHistogram::GetKeyedHistogramSnapshots(aCx, aResult, aDataset,

We're no longer considering the `MOZ_WIDGET_ANDROID` here. Since we don't support subsessions on Android,  do we need to keep that guard and just make it something like:

```
#ifdef MOZ_WIDGET_ANDROID
  if (aSubsession) {
    return NS_OK;
  }
#endif

  return TelemetryHistogram::GetKeyed...

```

::: toolkit/components/telemetry/TelemetryHistogram.h:63
(Diff revision 4)
>  
>  const char*
>  GetHistogramName(mozilla::Telemetry::HistogramID id);
>  
>  nsresult
> -CreateHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret,
> +CreateHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret, unsigned int aDataset,

nit: since we're changing the parameters to use our naming scheme, why not change `cx` and `ret` to `aCx` and `aRet` too?

::: toolkit/components/telemetry/TelemetryHistogram.h:67
(Diff revision 4)
>  nsresult
> -CreateHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret,
> -                         bool subsession, bool clearSubsession);
> +CreateHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret, unsigned int aDataset,
> +                         bool aSubsession, bool aClearSubsession);
>  
>  nsresult
> -RegisteredHistograms(uint32_t aDataset, uint32_t *aCount,
> +GetKeyedHistogramSnapshots(JSContext *cx, JS::MutableHandle<JS::Value> ret, unsigned int aDataset,

nit: same here

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1919
(Diff revision 4)
>    const HistogramInfo& h = gHistogramInfos[id];
>    return h.name();
>  }
>  
>  nsresult
>  TelemetryHistogram::CreateHistogramSnapshots(JSContext *cx,

nit: since we're changing the names of the other parameters, let's do the same for cx/ret as well

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2005
(Diff revision 4)
> -  return internal_GetRegisteredHistogramIds(true,
> -                                            aDataset, aCount, aHistograms);
> -}
> -
> -nsresult
>  TelemetryHistogram::GetKeyedHistogramSnapshots(JSContext *cx,

nit: let's make cx/ret names consistent with the other parameter names

::: toolkit/components/telemetry/nsITelemetry.idl:60
(Diff revision 4)
> -   * An object containing a snapshot from all of the currently registered histograms from all processes.
> -   * { process1: {name1: {data1}, name2:{data2}...} }
> -   * where data is consists of the following properties:
> -   *   min - Minimal bucket size
> +   * Serializes the histograms from the given dataset to a JSON-style object.
> +   * The returned structure looks like:
> +   *   { process1: {name1: {histogramData1}, name2:{histogramData2}...}}
> +   *
> +   * Where histogramDataN has the following properties:
> +   *   min - Minimum bucket size

nit: let's start Minimum with a lowercase letter for consistency with the other entries in this docs

::: toolkit/components/telemetry/nsITelemetry.idl:61
(Diff revision 4)
> -   * { process1: {name1: {data1}, name2:{data2}...} }
> -   * where data is consists of the following properties:
> -   *   min - Minimal bucket size
> +   * The returned structure looks like:
> +   *   { process1: {name1: {histogramData1}, name2:{histogramData2}...}}
> +   *
> +   * Where histogramDataN has the following properties:
> +   *   min - Minimum bucket size
>     *   max - Maximum bucket size

nit: let's change this too to start with a lowercase M

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:75
(Diff revision 4)
>  
>    // there should be exactly one element per bucket
>    for (let i of s.counts) {
>      do_check_eq(i, 1);
>    }
> -  var hgrams = Telemetry.histogramSnapshots.parent;
> +  var hgrams = Telemetry.snapshotHistograms(1, false, false).parent;

nit: can we replace `1` with the relevant constant definition?

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:117
(Diff revision 4)
>    // Instantiate the subsession histogram through |add| and make sure they match.
>    // This MUST be the first use of "TELEMETRY_TEST_COUNT" in this file, otherwise
>    // |add| will not instantiate the histogram.
>    h.add(1);
>    let snapshot = h.snapshot();
> -  let subsession = Telemetry.snapshotSubsessionHistograms().parent;
> +  let subsession = Telemetry.snapshotHistograms(1 /* opt-in */, true /* subsession */, false /* clear */).parent;

nit: ditto here

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:175
(Diff revision 4)
>  
>  add_task(async function test_noSerialization() {
>    // Instantiate the storage for this histogram and make sure it doesn't
>    // get reflected into JS, as it has no interesting data in it.
>    Telemetry.getHistogramById("NEWTAB_PAGE_PINNED_SITES_COUNT");
> -  do_check_false("NEWTAB_PAGE_PINNED_SITES_COUNT" in Telemetry.histogramSnapshots.parent);
> +  do_check_false("NEWTAB_PAGE_PINNED_SITES_COUNT" in Telemetry.snapshotHistograms(1, false, false).parent);

nit: let's change the `1` here as well

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:441
(Diff revision 4)
> -  Assert.ok(!!rh);
>  
>    dummy.add(1);
>  
>    for (let process of ["main", "content", "gpu", "extension"]) {
> -    if (!(process in Telemetry.histogramSnapshots)) {
> +    if (!(process in Telemetry.snapshotHistograms(1, false, false))) {

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:445
(Diff revision 4)
>    for (let process of ["main", "content", "gpu", "extension"]) {
> -    if (!(process in Telemetry.histogramSnapshots)) {
> +    if (!(process in Telemetry.snapshotHistograms(1, false, false))) {
>        do_print("Nothing present for process " + process);
>        continue;
>      }
> -    do_check_eq(Telemetry.histogramSnapshots[process].__expired__, undefined);
> +    do_check_eq(Telemetry.snapshotHistograms(1, false, false)[process].__expired__, undefined);

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:447
(Diff revision 4)
>        do_print("Nothing present for process " + process);
>        continue;
>      }
> -    do_check_eq(Telemetry.histogramSnapshots[process].__expired__, undefined);
> +    do_check_eq(Telemetry.snapshotHistograms(1, false, false)[process].__expired__, undefined);
>    }
> -  do_check_eq(Telemetry.histogramSnapshots.parent[test_expired_id], undefined);
> +  do_check_eq(Telemetry.snapshotHistograms(1, false, false).parent[test_expired_id], undefined);

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:503
(Diff revision 4)
>    testSnapShot[key].sum = 0;
>    testSnapShot[key].counts = [1, 0, 0];
>    Assert.deepEqual(h.keys().sort(), testKeys);
>    Assert.deepEqual(h.snapshot(), testSnapShot);
>  
> -  let allSnapshots = Telemetry.keyedHistogramSnapshots.parent;
> +  let allSnapshots = Telemetry.snapshotKeyedHistograms(1, false, false).parent;

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:559
(Diff revision 4)
>    testSnapShot[key] = testHistograms[4];
>  
>    Assert.deepEqual(h.keys().sort(), testKeys);
>    Assert.deepEqual(h.snapshot(), testSnapShot);
>  
> -  let allSnapshots = Telemetry.keyedHistogramSnapshots.parent;
> +  let allSnapshots = Telemetry.snapshotKeyedHistograms(1, false, false).parent;

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:632
(Diff revision 4)
>    };
>  
>    Assert.deepEqual(h.keys().sort(), [KEY]);
>    Assert.deepEqual(h.snapshot(), testSnapshot);
>  
> -  let allSnapshots = Telemetry.keyedHistogramSnapshots.parent;
> +  let allSnapshots = Telemetry.snapshotKeyedHistograms(1, false, false).parent;

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:782
(Diff revision 4)
>  add_task(async function test_histogramSnapshots() {
>    let keyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT");
>    keyed.add("a", 1);
>  
>    // Check that keyed histograms are not returned
> -  Assert.ok(!("TELEMETRY_TEST_KEYED_COUNT" in Telemetry.histogramSnapshots.parent));
> +  Assert.ok(!("TELEMETRY_TEST_KEYED_COUNT" in Telemetry.snapshotHistograms(1, false, false).parent));

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:825
(Diff revision 4)
>    let h = Telemetry.getHistogramById(COUNT);
>    let flag = Telemetry.getHistogramById(FLAG);
>  
>    // Both original and duplicate should start out the same.
>    h.clear();
> -  let snapshot = Telemetry.histogramSnapshots.parent;
> +  let snapshot = Telemetry.snapshotHistograms(1, false, false).parent;

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:826
(Diff revision 4)
>    let flag = Telemetry.getHistogramById(FLAG);
>  
>    // Both original and duplicate should start out the same.
>    h.clear();
> -  let snapshot = Telemetry.histogramSnapshots.parent;
> -  let subsession = Telemetry.snapshotSubsessionHistograms().parent;
> +  let snapshot = Telemetry.snapshotHistograms(1, false, false).parent;
> +  let subsession = Telemetry.snapshotHistograms(1, true, false).parent;

nit: change the `1`

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:832
(Diff revision 4)
>    Assert.ok(!(COUNT in snapshot));
>    Assert.ok(!(COUNT in subsession));
>  
>    // They should instantiate and pick-up the count.
>    h.add(1);
> -  snapshot = Telemetry.histogramSnapshots.parent;
> +  snapshot = Telemetry.snapshotHistograms(1, false, false).parent;

nit: here as well

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:833
(Diff revision 4)
>    Assert.ok(!(COUNT in subsession));
>  
>    // They should instantiate and pick-up the count.
>    h.add(1);
> -  snapshot = Telemetry.histogramSnapshots.parent;
> -  subsession = Telemetry.snapshotSubsessionHistograms().parent;
> +  snapshot = Telemetry.snapshotHistograms(1, false, false).parent;
> +  subsession = Telemetry.snapshotHistograms(1, true, false).parent;

nit: here as well

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:841
(Diff revision 4)
>    Assert.equal(snapshot[COUNT].sum, 1);
>    Assert.equal(subsession[COUNT].sum, 1);
>  
>    // They should still reset properly.
>    h.clear();
> -  snapshot = Telemetry.histogramSnapshots.parent;
> +  snapshot = Telemetry.snapshotHistograms(1, false, false).parent;

nit: here as well

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:842
(Diff revision 4)
>    Assert.equal(subsession[COUNT].sum, 1);
>  
>    // They should still reset properly.
>    h.clear();
> -  snapshot = Telemetry.histogramSnapshots.parent;
> -  subsession = Telemetry.snapshotSubsessionHistograms().parent;
> +  snapshot = Telemetry.snapshotHistograms(1, false, false).parent;
> +  subsession = Telemetry.snapshotHistograms(1, true, false).parent;

nit: here as well

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:848
(Diff revision 4)
>    Assert.ok(!(COUNT in snapshot));
>    Assert.ok(!(COUNT in subsession));
>  
>    // Both should instantiate and pick-up the count.
>    h.add(1);
> -  snapshot = Telemetry.histogramSnapshots.parent;
> +  snapshot = Telemetry.snapshotHistograms(1, false, false).parent;

nit: well, you know the drill, let's just replace all the 1s with the relevant constant here and below ;)

::: toolkit/components/terminator/tests/xpcshell/test_terminator_reload.js:36
(Diff revision 4)
>  });
>  
>  add_task(async function test_reload() {
>    do_print("Forging data");
>    let data = {};
> -  let telemetrySnapshots = Services.telemetry.histogramSnapshots.parent;
> +  let telemetrySnapshots = Services.telemetry.snapshotHistograms(1 /* opt-in */, false /* subsession */, false /* clear */).parent;

nit: s/1/constant name

::: toolkit/components/terminator/tests/xpcshell/test_terminator_reload.js:67
(Diff revision 4)
>  
>    do_print("Waiting until telemetry is updated");
>    // Now wait until Telemetry is updated
>    await wait;
>  
> -  telemetrySnapshots = Services.telemetry.histogramSnapshots.parent;
> +  telemetrySnapshots = Services.telemetry.snapshotHistograms(1 /* opt-in */, false /* subsession */, false /* clear */).parent;

nit: here as well

Attachment #8900364 - Flags: review?(alessio.placitelli) → review-

Chris H-C :chutten

Assignee

Comment 8

•

7 years ago

mozreview-review-reply

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

https://reviewboard.mozilla.org/r/171718/#review177260

> We're no longer considering the `MOZ_WIDGET_ANDROID` here. Since we don't support subsessions on Android,  do we need to keep that guard and just make it something like:
> 
> ```
> #ifdef MOZ_WIDGET_ANDROID
>   if (aSubsession) {
>     return NS_OK;
>   }
> #endif
> 
>   return TelemetryHistogram::GetKeyed...
> 
> ```

We have a confusing handling of MOZ_WIDGET_ANDROID for subsession snapshots. TelemetryHistogram seems to think it's okay to compel any "subsession" request to a session request (see TelemetryHistogram::CreateHistogramSnapshots) rather than early return.

So which do we want? Early empty return with NS_OK, or compel to session request? I'm okay with either, but we should be consistent.

Comment hidden (mozreview-request)

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171718/diff/4-5/

Alessio Placitelli [:Dexter]

Comment 10

•

7 years ago

mozreview-review-reply

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

https://reviewboard.mozilla.org/r/171718/#review177260

> We have a confusing handling of MOZ_WIDGET_ANDROID for subsession snapshots. TelemetryHistogram seems to think it's okay to compel any "subsession" request to a session request (see TelemetryHistogram::CreateHistogramSnapshots) rather than early return.
> 
> So which do we want? Early empty return with NS_OK, or compel to session request? I'm okay with either, but we should be consistent.

Good point. In the current form, without your patch, we have [two calls](http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/components/telemetry/Telemetry.cpp#569,578). The first one always sets the subsession to false, the second is protected by MOZ_WIDGET_ANDROID. Looks like the MOZ_WIDGET_ANDROID calls [here](http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/components/telemetry/TelemetryHistogram.cpp#1965,2016) are not that useful. I'd say early empty return with NS_OK is the way to go, but I have no strong opinion with either.

Alessio Placitelli [:Dexter]

Comment 11

•

7 years ago

mozreview-review

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

https://reviewboard.mozilla.org/r/171718/#review177464

::: toolkit/components/telemetry/Telemetry.cpp:572
(Diff revision 5)
> +                                                    aSubsession,
> +                                                    aClearHistograms);

nit: I think this and the line above should be indented a bit to the right

Attachment #8900364 - Flags: review?(alessio.placitelli) → review-

Comment hidden (mozreview-request)

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171718/diff/5-6/

Alessio Placitelli [:Dexter]

Comment 13

•

7 years ago

mozreview-review

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

https://reviewboard.mozilla.org/r/171718/#review177510

This looks good with the below addressed, rwc+!

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1926
(Diff revisions 5 - 6)
>                                               unsigned int aDataset,
>                                               bool aSubsession,
>                                               bool aClearSubsession)
>  {
> +#if defined(MOZ_WIDGET_ANDROID)
> +  if (aSubsession {

This is missing the closing ")"

Attachment #8900364 - Flags: review?(alessio.placitelli) → review+

Comment hidden (mozreview-request)

Comment on attachment 8900364 [details]
bug 1376600 - Remove registered(Keyed)Histograms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171718/diff/6-7/

Georg Fritzsche [:gfritzsche]

Updated

•

7 years ago

Whiteboard: [qf]

Pulsebot

Comment 15

•

7 years ago

Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/557d2d6e5c80
Remove registered(Keyed)Histograms r=Dexter

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 16

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/557d2d6e5c80

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox57: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla57

Dave Hunt [:davehunt] [he/him] ⌚BST

Updated

•

2 years ago

Performance Impact: --- → ?

Whiteboard: [qf]