1243893 - Telemetry in-tree docs conflict with actual pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Sam Penrose]

"family", "stepping", and "model" are listed as strings:

  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/environment.rst#84

but in about:healthreport they are integers. Running 46 Nightly.

Comment 1

[Sam Penrose]

partnerNames is an object when empty, an array when non-empty. Same for system/gfx/monitors.

Comment 2

[Sam Penrose]

payload/UIMeasurements can be an empty object (and maybe an empty array). Also payload/threadHangStats

Comment 3

[Sam Penrose]

creationDate and resetDate often have unrealistic values; LMK if I should create a bug.

Comment 4

[Georg Fritzsche [:gfritzsche]]

It's fine for me to collect it in one bug.
Do you expect more finds to come up soon?

Comment 5

[Sam Penrose]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> It's fine for me to collect it in one bug.
> Do you expect more finds to come up soon?

I expect to find a few more issues as we cover more fields. There is certainly no rush for a fix, though if you think I am mistaken on any points please let me know! I assume that if we use a schema to control the ping contents on the client, it will lead to a clean documentation solution.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Ok, we can just fix the issues mentioned here when we get there.
If you find new ones and this was already fixed, we can always file a new bug for other issues.

Comment 7

[Sam Penrose]

environment/addons/activeGMPlugins: applyBackgroundUpdates is documented as boolean but can come in as an integer (i.e. 0 or 1).
environment/addons/activeAddons: foreignInstall is documented as boolean but can come in as an integer (i.e. 0 or 1).

Comment 8

[Sam Penrose]

environment/build/buildId and application/buildId can be 10 or 14 digits long.
environment/addons/activeAddons/<key>/version can be numbers or strings.
environment/addons/activeAddons/<key>/userDisabled is documented as boolean but can come in as an integer (i.e. 0 or 1).

environment/addons/activePlugins can be an empty object instead of an array.
environment/addons/activePlugins/[i]/mimeTypes can be an empty object instead of an array.
Fixing the last two would be great; there are other constraints that depend on its being an array.

Comment 9

[Benjamin Smedberg]

> environment/build/buildId and application/buildId can be 10 or 14 digits
> long.

This is expected/normal: mozilla.org Firefox builds use a 14-digit buildid, but Ubuntu distro builds and other similar cases use the 10-digit form.

Comment 10

[Sam Penrose]

(In reply to Sam Penrose from comment #1)
> partnerNames is an object when empty, an array when non-empty. Same for
> system/gfx/monitors.

Regarding all of these "array when full, object when empty" dimensions, we should do forensic analysis of the pings in question (i.e. which FF are they coming from). Same with other issues, starting with bad dates.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Fix the docs.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Enforce boolean

The real question here is: should we return null/undefined is an unexpected format is encountered? (e.g. a string?)

Comment 13

[Alessio Placitelli [:Dexter]]

Misc notes:

* "family", "stepping", and "model" 
- Fixed the docs

* environment/addons/activeGMPlugins: applyBackgroundUpdates is documented as boolean but can come in as an integer (i.e. 0 or 1)
- This should definitely be an integer, with the correct values being 0, 1 or 2 (see https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/toolkit/mozapps/extensions/AddonManager.jsm#3058). Fixed the docs.
Also see https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/Addon

* environment/build/buildId and application/buildId can be 10 or 14 digits long.
- This is normal/expected as Benjamin mentioned. Added to the docs.

* environment/addons/activeAddons/<key>/version can be numbers or strings.
- This should be fixed now, see bug 1240682.

* environment/addons/activeAddons: foreignInstall is documented as boolean but can come in as an integer (i.e. 0 or 1).
* environment/addons/activeAddons/<key>/userDisabled is documented as boolean but can come in as an integer (i.e. 0 or 1).
- This should be fixed by part 2.

* partnerNames is an object when empty, an array when non-empty
- Doesn't seem to behave like that with the latest nightly and Release. partnerNames is an empty array on Windows and Ubuntu.

* system/gfx/monitors is an object when empty, an array when non-empty
* payload/UIMeasurements can be an empty object (and maybe an empty array)
* payload/threadHangStats can be an empty object (and maybe an empty array)
* creationDate and resetDate often have unrealistic values
* environment/addons/activePlugins can be an empty object instead of an array.
* environment/addons/activePlugins/[i]/mimeTypes can be an empty object instead of an array

I could not really reproduce those locally. Having access to the pings would be useful here.

Comment 14

[Sam Penrose]

childPayloads seems to be described as an object:

  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/docs/main-ping.rst#49

but is an array of objects.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8717873 [details] [diff] [review]
part 2 - Enforce boolean

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +190,5 @@
> + *         value. Otherwise, just return aValue.
> + */
> +function enforceBoolean(aValue) {
> +  if (typeof aValue != "number" && typeof aValue != "boolean") {
> +    return aValue;

We have a limited set of options here:
* return null
* return the original, broken value
* leave the property undefined
All are not nice for analysis, but i think we need some signal to see broken client-side data.

I'd say lets return |null| here - but we need to talk with Sam & Roberto to see if that fits with their use-cases.

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #13)
> ...
> * partnerNames is an object when empty, an array when non-empty
...
> * system/gfx/monitors is an object when empty, an array when non-empty
> * payload/UIMeasurements can be an empty object (and maybe an empty array)
> * payload/threadHangStats can be an empty object (and maybe an empty array)
> ...
> * environment/addons/activePlugins can be an empty object instead of an
> array.
> * environment/addons/activePlugins/[i]/mimeTypes can be an empty object
> instead of an array

This seems to be a common pattern - we should nail down whether this:
a) is a current problem
b) happening on the client or an artifact of the analysis

Lets move this to a follow-up bug.

> * creationDate and resetDate often have unrealistic values

I guess there is only so much we can do about that. We can take a look on whether this has crazy values, but if the filesystem "modified" timestamps are borked we don't have good options.
It might make more sense to submit broken values so we see them instead of sanitizing?

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8717872 [details] [diff] [review]
part 1 - Fix the docs.

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

Lets fix the childPayloads per comment 14 too.

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Fix the docs.

Comment 19

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> Comment on attachment 8717873 [details] [diff] [review]
> part 2 - Enforce boolean
> 
> Review of attachment 8717873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> @@ +190,5 @@
> > + *         value. Otherwise, just return aValue.
> > + */
> > +function enforceBoolean(aValue) {
> > +  if (typeof aValue != "number" && typeof aValue != "boolean") {
> > +    return aValue;
> 
> We have a limited set of options here:
> * return null
> * return the original, broken value
> * leave the property undefined
> All are not nice for analysis, but i think we need some signal to see broken
> client-side data.
> 
> I'd say lets return |null| here - but we need to talk with Sam & Roberto to
> see if that fits with their use-cases.

Sam, Roberto, would reporting |null| instead of a broken value break any of your use-cases here?

Comment 20

[Sam Penrose]

(In reply to Alessio Placitelli [:Dexter] from comment #19)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> > Comment on attachment 8717873 [details] [diff] [review]
> > part 2 - Enforce boolean
> > 
> > Review of attachment 8717873 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> > @@ +190,5 @@
> > > + *         value. Otherwise, just return aValue.
> > > + */
> > > +function enforceBoolean(aValue) {
> > > +  if (typeof aValue != "number" && typeof aValue != "boolean") {
> > > +    return aValue;
> > 
> > We have a limited set of options here:
> > * return null
> > * return the original, broken value
> > * leave the property undefined
> > All are not nice for analysis, but i think we need some signal to see broken
> > client-side data.
> > 
> > I'd say lets return |null| here - but we need to talk with Sam & Roberto to
> > see if that fits with their use-cases.
> 
> Sam, Roberto, would reporting |null| instead of a broken value break any of
> your use-cases here?

I am fine with |null|. 0 would also work.

Comment 21

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> (In reply to Alessio Placitelli [:Dexter] from comment #13)
> > ...
> > * partnerNames is an object when empty, an array when non-empty
> ...
> > * system/gfx/monitors is an object when empty, an array when non-empty
> > * payload/UIMeasurements can be an empty object (and maybe an empty array)
> > * payload/threadHangStats can be an empty object (and maybe an empty array)
> > ...
> > * environment/addons/activePlugins can be an empty object instead of an
> > array.
> > * environment/addons/activePlugins/[i]/mimeTypes can be an empty object
> > instead of an array
> 
> This seems to be a common pattern - we should nail down whether this:
> a) is a current problem
> b) happening on the client or an artifact of the analysis
> 
> Lets move this to a follow-up bug.

Filed bug 1248622.

> > * creationDate and resetDate often have unrealistic values
> 
> I guess there is only so much we can do about that. We can take a look on
> whether this has crazy values, but if the filesystem "modified" timestamps
> are borked we don't have good options.
> It might make more sense to submit broken values so we see them instead of
> sanitizing?

Yes, I think so.

Comment 22

[Roberto Agostino Vitillo (:rvitillo)]

I am fine with |null|.

Comment 23

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Enforce boolean

Comment 24

[Alessio Placitelli [:Dexter]]

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

Comment 25

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/55c2f5187b1a3729769cf5d9602c8ab632aaa17f
Bug 1243893 - Telemetry in-tree docs conflict with actual pings. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/d9445561d245533d4165c9f32d971ab0361cfcb9
Bug 1243893 - Force values to true/false if the expected type is boolean in the environment. r=gfritzsche

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55c2f5187b1a
https://hg.mozilla.org/mozilla-central/rev/d9445561d245