1467705 - The GeckoView API should pack histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

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

Comment 1

[Alessio Placitelli [:Dexter]]

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.

Comment 2

[Jan-Erik Rediger [:janerik]]

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?

Comment 3

[Alessio Placitelli [:Dexter]]

(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).

Comment 4

[Frank Bertsch [:frank]]

(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?

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1467705 - Refactor the histogram packing routines in TelemetryUtils.

Firefox Desktop is not sending raw histograms out, in pings. Instead, it
processes the raw histograms and packs them in a sparser representation.
GeckoView will need to do the same, so share the code rather than copy
paste it.

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

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1467705 - Pack GeckoView histograms using TelemetryUtils.

Firefox Desktop uses a sparse representation for histograms and the
pipeline expects them to be this way. This patch makes sure the
GeckoView API returns packed histograms and keyed histograms.

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

Comment 7

[Eugen Sawin [:esawin]]

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 };

Comment 8

[Eugen Sawin [:esawin]]

s/snapshot/snapshots

Comment 9

[Chris H-C :chutten]

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

Comment 10

[Eugen Sawin [:esawin]]

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

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

r+ with nits.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1467705 - Add test coverage for histogram packing routines.

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

Comment 12

[Alessio Placitelli [:Dexter]]

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

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

Comment 13

[Alessio Placitelli [:Dexter]]

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

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

Comment 14

[Chris H-C :chutten]

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.

Comment 15

[Alessio Placitelli [:Dexter]]

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

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

Comment 16

[Chris H-C :chutten]

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

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/fc7a9042ec08
https://hg.mozilla.org/mozilla-central/rev/bde911a9320a
https://hg.mozilla.org/mozilla-central/rev/915699f04f03