1409468 - Add telemetry to detect systems with touchbar

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Stephen A Pohl [:spohl]]

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.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Test

This matches the test for isWow64 on Windows.

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8919384 [details] [diff] [review]
Patch

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

This probably requires a data review from somebody.

Comment 4

[Stephen A Pohl [:spohl]]

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.

Comment 5

[François Marier [:francois]]

Comment on attachment 8919384 [details] [diff] [review]
Patch

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

This is Category 1 data.

datareview+

Comment 6

[Stephen A Pohl [:spohl]]

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

Comment 7

[Stephen A Pohl [:spohl]]

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.

Comment 8

[Stephen A Pohl [:spohl]]

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.

Comment 9

[François Marier [: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.

Comment 10

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=241fce92c97e5fab36d2d9e236ed813f00b55f77

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Patch

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.

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Test

Comment 13

[François Marier [:francois]]

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+

Comment 14

[Stephen A Pohl [:spohl]]

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

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9b4ab3a0b00b
https://hg.mozilla.org/mozilla-central/rev/8503e0f7970e

Comment 16

[Stephen A Pohl [:spohl]]

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

Comment 17

[Stephen A Pohl [:spohl]]

Comment on attachment 8919714 [details] [diff] [review]
Test

Approval Request Comment
This is the test to go along with the patch in this bug.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8919713 [details] [diff] [review]
Patch

Low risk, taking it as this is needed for roadmap planning, Beta57+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/13789fec0a4b
https://hg.mozilla.org/releases/mozilla-beta/rev/428c84cbc00a

Comment 20

[Andrei Vaida [:avaida]]

(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.