The GeckoView API should pack histograms

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments)

On desktop, we pack histograms [1] before adding them to the main ping. The pipeline seems to expect them to be in this format [2]. Even if we're not using the aggregator for the "mobile-ping", it seems a sane thing to have histograms packed on GeckoView as well, for consistency.

[1] - https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/components/telemetry/TelemetrySession.jsm#814-862
[2] - https://github.com/mozilla/python_mozaggregator/blob/8aa3582112b5ab006f0b5527c1c02a29639353bf/mozaggregator/aggregator.py#L128-L149
Assignee

Updated

Last year
Blocks: 1452550
Priority: -- → P1
Hey, what do you think of comment 0? I would lean toward packing histograms in the GeckoView API, so that we have a consistent outgoing histogram format across products.
Flags: needinfo?(jrediger)
Flags: needinfo?(fbertsch)
A consistent format across different products seems sensible. We should just properly document this format.
Where in the GeckoView code would we do the packing?
Flags: needinfo?(jrediger)
(In reply to Jan-Erik Rediger [:janerik] from comment #2)
> A consistent format across different products seems sensible. We should just
> properly document this format.
> Where in the GeckoView code would we do the packing?

I think this should live in the GeckoViewTelemetryController.js, here: https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm#96

We should share the code from TelemetrySession.jsm, move it to TelemetryUtils.jsm, and use it in both places (maybe add test coverage as well :D).
(In reply to Alessio Placitelli [:Dexter] from comment #0)
> On desktop, we pack histograms [1] before adding them to the main ping. The
> pipeline seems to expect them to be in this format [2]. Even if we're not
> using the aggregator for the "mobile-ping", it seems a sane thing to have
> histograms packed on GeckoView as well, for consistency.

Sparse Histograms: definitely, because we have the histogram definitions available
min/max/bucket_count/histogram_type: Let's remove this, we don't use them anywhere
sum: Definitely keep

Thoughts on this?
Flags: needinfo?(fbertsch)
Assignee

Updated

Last year
Assignee: nobody → alessio.placitelli
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

Last year
mozreview-review
Comment on attachment 8984542 [details]
Bug 1467705 - Pack GeckoView histograms using TelemetryUtils.

https://reviewboard.mozilla.org/r/250418/#review256678

::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:107
(Diff revision 1)
>      const snapshots = {
> -      histograms: Services.telemetry.snapshotHistograms(
> -                      dataset, /* subsession */ false, /* clear */ false),
> +      histograms: TelemetryUtils.packHistograms(rawHistograms),
> +      keyedHistograms: TelemetryUtils.packKeyedHistograms(rawKeyedHistograms),
> -      keyedHistograms: Services.telemetry.snapshotKeyedHistograms(
> -                           dataset, /* subsession */ false, /* clear */ false),
>        scalars: Services.telemetry.snapshotScalars(

Can you please move the snapshot retrieval for scalars out of this definition for consistency?

const scalars = Services...;
const keyedScalars = Services...;
const snapshot = { ..., scalars, keyedScalars };
s/snapshot/snapshots

Comment 9

Last year
mozreview-review
Comment on attachment 8984541 [details]
Bug 1467705 - Refactor the histogram packing routines in TelemetryUtils.

https://reviewboard.mozilla.org/r/250416/#review256698

Two nits, otherwise good to go.

::: toolkit/components/telemetry/TelemetryUtils.jsm:286
(Diff revision 1)
> +  /**
> +   * Converts histograms from the raw to the packed format.
> +   * This additionally filters TELEMETRY_TEST_ histograms.
> +   *
> +   * @param {Object} snapshot - The histogram snapshot.
> +   * @param {Boolean} [testingMode=false] - Wheter or not testing histograms

*Whether

::: toolkit/components/telemetry/TelemetryUtils.jsm:325
(Diff revision 1)
> +   * Converts keyed histograms from the raw to the packed format.
> +   * This additionally filters TELEMETRY_TEST_ histograms and skips
> +   * empty keyed histograms.
> +   *
> +   * @param {Object} snapshot - The keyed histogram snapshot.
> +   * @param {Boolean} [testingMode=false] - Wheter or not testing histograms should

*Whether
Attachment #8984541 - Flags: review?(chutten) → review+

Comment 10

Last year
mozreview-review
Comment on attachment 8984542 [details]
Bug 1467705 - Pack GeckoView histograms using TelemetryUtils.

https://reviewboard.mozilla.org/r/250418/#review257192

r+ with nits.
Attachment #8984542 - Flags: review?(esawin) → review+
See Also: → 1468761
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

Last year
mozreview-review
Comment on attachment 8985443 [details]
Bug 1467705 - Add test coverage for histogram packing routines.

https://reviewboard.mozilla.org/r/251050/#review257338

::: toolkit/components/telemetry/tests/unit/test_TelemetryUtils.js:5
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +ChromeUtils.import('resource://gre/modules/ObjectUtils.jsm', this);
> +ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm", this);

Which are we using in this file, " or '?

::: toolkit/components/telemetry/tests/unit/test_TelemetryUtils.js:83
(Diff revision 1)
> +
> +add_task(async function testKeyedHistogramPacking() {
> +  const KEYED_HISTOGRAM_SNAPSHOT = {
> +    sample_process: {
> +      HISTOGRAM_1_DATA: {
> +        someKey: {

...would you mind adding a second key in either or both of these? We've been burned by forgetting to test the multi-key case in the recent past.
Attachment #8985443 - Flags: review?(chutten)
Comment hidden (mozreview-request)

Comment 16

Last year
mozreview-review
Comment on attachment 8985443 [details]
Bug 1467705 - Add test coverage for histogram packing routines.

https://reviewboard.mozilla.org/r/251050/#review257356
Attachment #8985443 - Flags: review?(chutten) → review+

Comment 17

Last year
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc7a9042ec08
Refactor the histogram packing routines in TelemetryUtils. r=chutten
https://hg.mozilla.org/integration/autoland/rev/bde911a9320a
Pack GeckoView histograms using TelemetryUtils. r=esawin
https://hg.mozilla.org/integration/autoland/rev/915699f04f03
Add test coverage for histogram packing routines. r=chutten
You need to log in before you can comment on or make changes to this bug.