Closed Bug 1276196 Opened 4 years ago Closed 3 years ago

Add scalars to the main ping payload

Categories

(Toolkit :: Telemetry, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

In TelemetrySession, we want to add the |scalars| and |keyedScalars| fields with the serialized scalar data.
Blocks: 1275517
No longer blocks: 1276195
Assignee: nobody → alessio.placitelli
Priority: P3 → P1
Attached patch bug1276196.patch (obsolete) — Splinter Review
Status: NEW → ASSIGNED
Attachment #8764567 - Flags: review?(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.
Attachment #8764567 - Flags: review?(gfritzsche) → review+
Attached patch bug1276196.patchSplinter Review
Attachment #8764567 - Attachment is obsolete: true
Attachment #8764951 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f5a2832bb5f9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.