Telemetry in-tree docs conflict with actual pings

RESOLVED FIXED in Firefox 47

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: spenrose, Assigned: Dexter)

Tracking

unspecified
mozilla47
Points:
2

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
"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.
(Reporter)

Comment 1

3 years ago
partnerNames is an object when empty, an array when non-empty. Same for system/gfx/monitors.
(Reporter)

Comment 2

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

Comment 3

3 years ago
creationDate and resetDate often have unrealistic values; LMK if I should create a bug.
It's fine for me to collect it in one bug.
Do you expect more finds to come up soon?
Blocks: 1201022
Priority: -- → P3
Summary: docs/environment.rst type conflicts with ping → Telemetry in-tree docs conflict with actual pings
Whiteboard: [measurement:client]
Blocks: 1216191
No longer blocks: 1201022
Flags: needinfo?(spenrose)
Points: --- → 2
Priority: P3 → P1
(Assignee)

Updated

3 years ago
See Also: → bug 1136719
(Reporter)

Comment 5

3 years ago
(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.
Flags: needinfo?(spenrose)
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.
(Reporter)

Comment 7

3 years ago
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).
(Reporter)

Comment 8

3 years ago
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

3 years ago
> 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.
(Assignee)

Updated

3 years ago
Assignee: nobody → alessio.placitelli
(Reporter)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
Created attachment 8717872 [details] [diff] [review]
part 1 - Fix the docs.
Attachment #8717872 - Flags: review?(gfritzsche)
(Assignee)

Comment 12

3 years ago
Created attachment 8717873 [details] [diff] [review]
part 2 - Enforce boolean

The real question here is: should we return null/undefined is an unexpected format is encountered? (e.g. a string?)
Attachment #8717873 - Flags: feedback?(gfritzsche)
(Assignee)

Comment 13

3 years ago
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.
Status: NEW → ASSIGNED
(Reporter)

Comment 14

3 years ago
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 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.
Attachment #8717873 - Flags: feedback?(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 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.
Attachment #8717872 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 18

3 years ago
Created attachment 8719788 [details] [diff] [review]
part 1 - Fix the docs.
Attachment #8717872 - Attachment is obsolete: true
Attachment #8719788 - Flags: review+
(Assignee)

Comment 19

3 years ago
(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?
Flags: needinfo?(spenrose)
Flags: needinfo?(rvitillo)
(Reporter)

Comment 20

3 years ago
(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.
Flags: needinfo?(spenrose)
(Assignee)

Comment 21

3 years ago
(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.
I am fine with |null|.
Flags: needinfo?(rvitillo)
(Assignee)

Comment 23

3 years ago
Created attachment 8719847 [details] [diff] [review]
part 2 - Enforce boolean
Attachment #8717873 - Attachment is obsolete: true
Attachment #8719847 - Flags: review?(gfritzsche)
Attachment #8719847 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 25

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55c2f5187b1a
https://hg.mozilla.org/mozilla-central/rev/d9445561d245
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.