1502435 - Tag clients affected by Fennec client id issue in core ping data

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

Following up to the incident in bug 1501329, we need fixes for the Fennec code.

In the outgoing Telemetry data, we need to be able to tell which profiles were affected. For this purpose we need to add a new property to outgoing Telemetry pings in 1) the Gecko Telemetry (handled in bug 1501329) and 2) the Fennec Telemetry data (handled here).

- bug 1501329, comment 20 has the gecko telemetry patches explained
- bug 1501329, comment 21 has the data explained
- for saved-session pings we send environment.profile.wasCanary (boolean, only available on Android)
- we'll need to add a field to the core ping on fennec here
- for affected clients, the field should be “bug_1501329_affected”: true
- for unaffected clients, the field should be “bug_1501329_affected”: false
- the information is tracked in gecko telemetry
- the core ping is implemented in fennec code
- so the information needs to be passed up to / loaded from Fennec code
- Fennec loads state.json, where we're adding this information (bug 1501329) as `wasCanary` (optional boolean property)
- Fennec may also load the client id at runtime from Gecko Telemetry (i'm not sure)

One spot of client id handling for core ping:
https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingDelegate.java#159,170

Comment 1

[Georg Fritzsche [:gfritzsche]]

Tracking for 63 - this is aimed at a Fennec dot release.

Comment 2

[Frank Bertsch [:frank]]

Bug 1502439 will take care of server-side validation and making this data available to query.

Comment 3

[:sdaswani]

Petru Vlad Andrei - this is super high priority. If you need help figuring this out, please ping Georg or Jan-Erik.

Comment 4

[:sdaswani]

You can also ping :snorp if you need help.

Comment 5

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1502435 - 1 - Set valid clientId and save if previous was canary clientId; r?sdaswani

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1502435 - 2 - Include new `bug_1501329_affected` property in the core ping; r?sdaswani

Depends on D10202

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1502435 - 3 - Update tests to include checks for canary clientId; r?sdaswani

Depends on D10205

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Because the Fennec side telemetry starts before Gecko and before the core telemetry [1], Fennec will also check the clientId and if it is the known canary clientId will reset it and persist about this in datareporting/state.json.

Tested with :janerik and confirmed that the core ping contains the new “bug_1501329_affected” property which is only set to true if the previous clientId was the canary one.

[1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoProfile.java#462

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Try builds just finished, except an intermittent failure for a mochitest all is green - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a362a177bec344523e9ed3b21b7b36ae3a315574

Comment 10

[Jan-Erik Rediger [:janerik]]

Here's a try run with these changes based on top of mine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c292cc10df20f3b94152bf1a272afbd502360067

Looks good to me as well.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0a9b2e68b8f
1 - Set valid clientId and save if previous was canary clientId; r=jchen

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cb4e28e1eb8
2 - Include new `bug_1501329_affected` property in the core ping; r=jchen
https://hg.mozilla.org/integration/autoland/rev/3992a5ddbac3
3 - Update tests to include checks for canary clientId; r=jchen

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/e0a9b2e68b8f
https://hg.mozilla.org/mozilla-central/rev/5cb4e28e1eb8
https://hg.mozilla.org/mozilla-central/rev/3992a5ddbac3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9021112 [details]
Bug 1502435 - 1 - Set valid clientId and save if previous was canary clientId; r?sdaswani

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1489520

User impact if declined: Telemetry is sent with a  canary clientId

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501329

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small change, patch has been verified and confirmed as working.

String changes made/needed:

Comment 16

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9021116 [details]
Bug 1502435 - 2 - Include new `bug_1501329_affected` property in the core ping; r?sdaswani

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1489520

User impact if declined: Telemetry is sent with a  canary clientId

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501329

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small change, patch has been verified and confirmed as working.

String changes made/needed:

Comment 17

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9021118 [details]
Bug 1502435 - 3 - Update tests to include checks for canary clientId; r?sdaswani

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: --

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Added a few unit tests for the canary clientId.
No issues found when ran locally or on treeherder - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a362a177bec344523e9ed3b21b7b36ae3a315574

String changes made/needed:

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

Verified on Firefox 65 nightly. Downloaded an affected nightly (2018-10-01 API 16) verified that the client id was the canary value. Used the in app updater to download a build with the fix. (2018-10-31 API 16) 

Worth noting that the change is not done on startup so you may see the canary value when the build with the fix is run and immediately check about:telemetry or the about:config value. After a few minutes (seconds?) a new random value was generated.

Comment 19

[Jan-Erik Rediger [:janerik]]

Probably 60 seconds, the default delay we wait for fully initializing Telemetry.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9021112 [details]
Bug 1502435 - 1 - Set valid clientId and save if previous was canary clientId; r?sdaswani

[Triage Comment]
Fix for broken Fennec Telemetry reporting needed for the 63.0.2 dot release. Approved for 64.0b6.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/02cbbb3db3b5
https://hg.mozilla.org/releases/mozilla-beta/rev/81b5e0d8a726
https://hg.mozilla.org/releases/mozilla-beta/rev/cb358118e54d

Comment 25

[Sorina Florean [:sflorean]]

Tested on build 64.0b6 and with the help and steps provided by Petru, these are my findings:

Used https://github.com/mozilla/gzipServer to be able to intercept the pings the app sends and found the following
1) When first started the app it started with a valid clientId and the value for "bug_1501329_affected" was "false"
2) Changed the clientId from WebIDE with ClientID.setClientID(TelemetryUtils.knownClientID) and restarted the app
The new ping now showed a different clientId but the value for "bug_1501329_affected" was now "true"
3) Any other following pings showed the value for "bug_1501329_affected" being correctly persisted, it remained "true

Marking status 64 as VERIFIED.

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

Petru, can you request uplift for your patch to Release now that it has been verified in Beta? Thanks

Comment 27

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9021112 [details]
Bug 1502435 - 1 - Set valid clientId and save if previous was canary clientId; r?sdaswani

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1489520

User impact if declined: Telemetry is sent with a  canary clientId

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501329

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small change, patch verified in Nightly and Beta.

String changes made/needed:

Comment 28

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9021116 [details]
Bug 1502435 - 2 - Include new `bug_1501329_affected` property in the core ping; r?sdaswani

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1489520

User impact if declined: Telemetry is sent with a  canary clientId

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501329

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small change, patch verified in Nightly and Beta

String changes made/needed:

Comment 29

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9021118 [details]
Bug 1502435 - 3 - Update tests to include checks for canary clientId; r?sdaswani

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Added a few unit tests for the canary clientId.
No issues found when ran locally or on treeherder - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a362a177bec344523e9ed3b21b7b36ae3a315574

String changes made/needed:

Comment 30

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9021112 [details]
Bug 1502435 - 1 - Set valid clientId and save if previous was canary clientId; r?sdaswani

Fix for broken Fennec Telemetry reporting needed for the 63.0.2 dot release.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/2ff0200c61b3
https://hg.mozilla.org/releases/mozilla-release/rev/6166ae37d035
https://hg.mozilla.org/releases/mozilla-release/rev/3acb6478c0ab

Comment 32

[Sorina Florean [:sflorean]]

Verified as fixed on build 63.0.2 following the same steps as in comment 25 and with the same findings.