Add telemetry to detect systems with touchbar

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: spohl, Assigned: spohl)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [tpi:+])

Attachments

(2 attachments, 2 obsolete attachments)

To help prioritize macOS platform work, it would be great to know what the distribution of users is with physical touchbar vs no touchbar. Apple intentionally does not provide an API to query this information directly, so we have to check and compare the MacBook model names instead.
Posted patch Patch (obsolete) — Splinter Review
Attachment #8919384 - Flags: review?(mstange)
Posted patch Test (obsolete) — Splinter Review
This matches the test for isWow64 on Windows.
Attachment #8919386 - Flags: review?(mstange)
Comment on attachment 8919384 [details] [diff] [review]
Patch

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

This probably requires a data review from somebody.
Attachment #8919384 - Flags: review?(mstange) → review+
Attachment #8919386 - Flags: review?(mstange) → review+
Comment on attachment 8919384 [details] [diff] [review]
Patch

This intentionally does not collect the user's actual model name, as we're only interested in whether or not a user has a system with touchbar.
Attachment #8919384 - Flags: review?(francois)
Comment on attachment 8919384 [details] [diff] [review]
Patch

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

This is Category 1 data.

datareview+
Attachment #8919384 - Flags: review?(francois) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6b39895ac2393514d2fba5cb60d0e2b9c47c72
Bug 1409468: Add a property to telemetry's environment data to detect when a device has an Apple touchbar. r=mstange,francois

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb727e675319191e4e86a7b3c1348cabd30af3b
Bug 1409468: Test for telemetry hasTouchbar environment property. r=mstange
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae1c0108e987e22160b5de706eae128f18ef75a. Thinking about the best way to analyze this data brought up some questions that I'd like to resolve first.
Francois, thinking about how to best analyze this data made me wonder if it would be acceptable to collect the actual model names of Mac users. This would future-proof our code because Apple might release new MacBooks with touchbars. Otherwise, new MacBooks with touchbars would incorrectly set the `hasTouchbar` property to false until we fix our telemetry code and uplift it.

By collecting the actual model names, we could differentiate between systems that have touchbars vs ones that don't during analysis.

If this is acceptable from a data collection perspective, I will update the patches and go through a new round of reviews.
Flags: needinfo?(francois)
(In reply to Stephen A Pohl [:spohl] from comment #8)
> If this is acceptable from a data collection perspective, I will update the
> patches and go through a new round of reviews.

The model name would still be Category 1 data.
Flags: needinfo?(francois)
Posted patch PatchSplinter Review
Sorry about this additional round of reviews, but I believe it will make our life a lot easier in the future. This patch accommodates future MacBook iterations with touchbars by reporting the MacBook model ID directly. Instead of detecting MacBooks with touchbars on the client side, we will now be able to select the correct model IDs during analysis. Once new MacBooks with touchbars are released, we can simply change the analysis script instead of having to write a telemetry patch and uplift it to all the branches that we're interested in.

Regarding MacBook model IDs: For the latest MacBooks (mid-2017) for example, there are three model IDs: MacBookPro14,1, MacBookPro14,2 and MacBookPro14,3. Each ID is associated with two if not three different actual MacBook models. And to be clear: the serial number is never included in hw.model, i.e. the model ID. This means that the data that we collect is still very high level.
Attachment #8919384 - Attachment is obsolete: true
Attachment #8919713 - Flags: review?(mstange)
Attachment #8919713 - Flags: review?(francois)
Posted patch TestSplinter Review
Attachment #8919386 - Attachment is obsolete: true
Attachment #8919714 - Flags: review?(mstange)
Comment on attachment 8919713 [details] [diff] [review]
Patch

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

(In reply to Stephen A Pohl [:spohl] from comment #11)
> Regarding MacBook model IDs: For the latest MacBooks (mid-2017) for example,
> there are three model IDs: MacBookPro14,1, MacBookPro14,2 and
> MacBookPro14,3. Each ID is associated with two if not three different actual
> MacBook models. And to be clear: the serial number is never included in
> hw.model, i.e. the model ID. This means that the data that we collect is
> still very high level.

Based on this, the extra info we are collecting is Category 1 data.

datareview+
Attachment #8919713 - Flags: review?(francois) → review+
Attachment #8919713 - Flags: review?(mstange) → review+
Attachment #8919714 - Flags: review?(mstange) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4ab3a0b00b188f5cd45c04fb14895f49883c5d
Bug 1409468: Add a property appleModelId to telemetry's environment data to collect model IDs for Apple desktop devices. r=mstange,francois

https://hg.mozilla.org/integration/mozilla-inbound/rev/8503e0f7970e6eeb0c419bcb2e3aa0e9b645dd5c
Bug 1409468: Test for appleModelId telemetry environment property. r=mstange
https://hg.mozilla.org/mozilla-central/rev/9b4ab3a0b00b
https://hg.mozilla.org/mozilla-central/rev/8503e0f7970e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8919713 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: We will be unable to collect model ID telemetry data from our user population on Apple devices, making it harder to prioritize platform work.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is simply adding a field to our telemetry environment data on macOS.
[String changes made/needed]: none
Attachment #8919713 - Flags: approval-mozilla-beta?
Comment on attachment 8919714 [details] [diff] [review]
Test

Approval Request Comment
This is the test to go along with the patch in this bug.
Attachment #8919714 - Flags: approval-mozilla-beta?
Comment on attachment 8919713 [details] [diff] [review]
Patch

Low risk, taking it as this is needed for roadmap planning, Beta57+
Attachment #8919713 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8919714 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Stephen A Pohl [:spohl] from comment #16)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Stephen's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.