1281214 - Implement the boolean scalar type

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

After the core implementation lands in bug 1276195, to cover all the basic scalar use cases, we should implement the boolean scalar type [0].

The scalar boolean will store a boolean value (useful for e.g. feature checks) and will not have a default value, meaning that it won't be sent unless it's set to some value.

[0] - https://docs.google.com/document/d/13mcY17RzpE3BsB4CgR6DidytWzl4tMpAEmrvf8fOSJ8/

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1281214.patch

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1281214.patch

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8769700 [details] [diff] [review]
bug1281214.patch

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

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +87,5 @@
>    UnsignedInvalidValue,
>    UnsignedNegativeValue,
>    UnsignedTruncatedValue,
> +  // Boolean Scalar Errors
> +  BooleanInvalidType,

Do we actually need to treat this different from `StringInvalidType`?
If not lets just use a general `InvalidType` code.

@@ +434,5 @@
> +  // Check that we got the correct data type.
> +  uint16_t type;
> +  aValue->GetDataType(&type);
> +  if (type != nsIDataType::VTYPE_BOOL) {
> +    return ScalarResult::BooleanInvalidType;

This prevents from doing calls like `scalarSet("foo", 1)`.
We should support number types that convert to boolean too.

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ +216,5 @@
> +                /NS_ERROR_NOT_AVAILABLE/,
> +                "Using an unsupported operation must throw.");
> +  Assert.throws(() => Telemetry.scalarSet(BOOLEAN_SCALAR, 1),
> +                /NS_ERROR_ILLEGAL_VALUE/,
> +                "The boolean scalar must throw if we're not setting a boolean.");

I think that should be supported.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug1281214.patch

Added support for implicitly converting numeric arguments to boolean for boolean scalars.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8769726 [details] [diff] [review]
bug1281214.patch

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

LGTM.
This needs a documentation update, but we can take that to a separate patch.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1281214_docs.patch

Update the docs.

Comment 7

[Alessio Placitelli [:Dexter]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/abf809943e7c
Implement the boolean scalar type. r=gfritzsche
https://hg.mozilla.org/integration/fx-team/rev/e0d821942d29
Update the documentation. r=gfritzsche

Comment 9

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/abf809943e7c8131c2f3235fec3373e3fe852b72
Bug 1281214 - Implement the boolean scalar type. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/e0d821942d29279c0213303560505097872b6651
Bug 1281214 - Update the documentation. r=gfritzsche

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abf809943e7c
https://hg.mozilla.org/mozilla-central/rev/e0d821942d29

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to Alessio Placitelli [:Dexter] from comment #0)
> The scalar boolean will store a boolean value (useful for e.g. feature
> checks) and will not have a default value, meaning that it won't be sent
> unless it's set to some value.

Does this mean that in order to replicate the semantics of the "flag" type in Historgrams.json (the build system doesn't want me to add more of those), I have to initialize the boolean to false at startup to collect the sessions where the situation that warrants setting the flag to true doesn't occur? If so, where should I do it so that each new content process doesn't reset the flag to false?

With e10s-multi, what are the semantics for booleans collected in the content process? Is the value "true" in any content process during the lifetime of the chrome process set the boolean to true?

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Henri Sivonen (:hsivonen) from comment #11)
> (In reply to Alessio Placitelli [:Dexter] from comment #0)
> > The scalar boolean will store a boolean value (useful for e.g. feature
> > checks) and will not have a default value, meaning that it won't be sent
> > unless it's set to some value.
> 
> Does this mean that in order to replicate the semantics of the "flag" type
> in Historgrams.json (the build system doesn't want me to add more of those),
> I have to initialize the boolean to false at startup to collect the sessions
> where the situation that warrants setting the flag to true doesn't occur? If
> so, where should I do it so that each new content process doesn't reset the
> flag to false?

If you're just interested in making sure some feature is being used, you would just need to set the scalar to "true" when that happens.
The scalar, as you mentioned, won't be sent unless it has some set value (either "true" or "false").

As discussed over IRC, there's a caveat there:

> without setting the "false" value, is it still possible to see the proportion of submissions where it was set to true vs. all submissions (incl. those with neither true nor false)?

@frank, do you know the answer to the question above?

> With e10s-multi, what are the semantics for booleans collected in the
> content process?

There's some documentation here: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html#multiple-processes-caveats

> Is the value "true" in any content process during the lifetime of the chrome process set the boolean to true?

Yes, it sets the "content" scalar to "true" if any content process sets it to true. If you are recording the same scalar in both the "parent" and the "content" processes, and you set the scalar to "true" only in the content process, only the "content" version of the scalar will be populated with the value, not the "parent" version.

Here's the relevant code that updates the scalar with data coming from the other processes, if curious: http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/toolkit/components/telemetry/TelemetryScalar.cpp#2205

Bonus example code: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_ChildScalars.js

Comment 13

[Frank Bertsch [:frank]]

> > without setting the "false" value, is it still possible to see the proportion of submissions where it was set to true vs. all submissions (incl. those with neither true nor false)?

No, it is not, at least not easily. Definitely TMO will not have that information available.

Comment 14

[Benjamin Smedberg]

In particular that is almost never a good question to ask. Subsessions are of greatly variable length. Usually the right question is how many *users* over a given time period are affected, or lacking that normalizing your data by usage hour.

But if that is in fact still the question you want to ask, it's possible to count the total number of subsesssions and the total number of subsessions with a value and present that as a ratio. I believe that's already possible using telemetry.js v2.

Comment 15

[Henri Sivonen (:hsivonen)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #14)
> In particular that is almost never a good question to ask. Subsessions are
> of greatly variable length.

It's good enough to figure out if a feature affects a large proportion of sessions or if the feature is almost entirely unused. (I.e. it's not about 50% vs. 10% but about within 0% rounding error vs. something else.)

> Usually the right question is how many *users*
> over a given time period are affected, or lacking that normalizing your data
> by usage hour.

I thought we deliberately couldn't correlate submissions over time belonging to the same user. Also, doing something fancy to normalize by hour is too complicated when the expectation is that the probe will confirm that the feature sees roughly no usage.

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #14)
> > Usually the right question is how many *users*
> > over a given time period are affected, or lacking that normalizing your data
> > by usage hour.
> 
> I thought we deliberately couldn't correlate submissions over time belonging
> to the same user.

We can do that.
Can we move the discussion to a bug that's specific to the problem you are trying to solve?