1276196 - Add scalars to the main ping payload

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

In TelemetrySession, we want to add the |scalars| and |keyedScalars| fields with the serialized scalar data.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1276196.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8764567 [details] [diff] [review]
bug1276196.patch

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

Look good with the below fixed.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +968,5 @@
> +    this._log.trace("getScalars - subsession: " + subsession + ", clearSubsession: " + clearSubsession);
> +
> +    if (!subsession) {
> +      // We only support scalars for subsessions.
> +      this._log.trace("getScalars - We only support scalars in subsessions.");

That probably means that we don't show scalars by default in about:telemetry.
Let's open a follow-up for showing subsession data by default in about:telemetry, otherwise it might be confusing for users in the future.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +379,5 @@
>    };
>    Assert.deepEqual(expected_keyed_count, keyedHistograms[TELEMETRY_TEST_KEYED_COUNT]);
> +
> +  // Check that the scalars section is available in the ping payload.
> +  Assert.ok("scalars" in payload, "The scalars section must be available in the payload.");

I think you can do more.
Check that:
* it's an object
* all its keys are of type string
* all its values are string or number type
* maybe check for max. string length values too

@@ +585,5 @@
> +  const TEST_SCALARS = [ UINT_SCALAR, STRING_SCALAR ];
> +  for (let name of TEST_SCALARS) {
> +    // No scalar must be reported in classic pings (e.g. saved-session).
> +    Assert.ok(!(name in classic.scalars),
> +              name + " must not be reported in a classic ping.");

If nothing should be reported there, test that, i.e.: 
Object.keys(classic.scalars).length == 0

@@ +608,5 @@
> +  }
> +
> +  // Check if setting the scalars again works as expected.
> +  Telemetry.scalarSet(UINT_SCALAR, expectedUint);
> +  Telemetry.scalarSet(STRING_SCALAR, expectedString);

You should test setting them to a different value in the new subsession.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1276196.patch

Comment 4

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2047df1eff88

Comment 5

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6b68efdb522

Comment 6

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/f5a2832bb5f9a9f5d31df70ca6557f3be6f9468e
Bug 1276196 - Add scalars to the main ping payload. r=gfritzsche

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f5a2832bb5f9