Closed Bug 1502435 Opened 11 months ago Closed 11 months ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 65
Tracking Status
firefox63 + verified
firefox64 + verified
firefox65 + verified

People

(Reporter: gfritzsche, Assigned: petru)

References

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(3 files)

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
Tracking for 63 - this is aimed at a Fennec dot release.
Priority: -- → P1
Bug 1502439 will take care of server-side validation and making this data available to query.
Flags: needinfo?(sdaswani)
Assignee: nobody → petru.lingurar
Flags: needinfo?(sdaswani)
Whiteboard: --do_not_change--[priority:high]
Petru Vlad Andrei - this is super high priority. If you need help figuring this out, please ping Georg or Jan-Erik.
You can also ping :snorp if you need help.
Blocks: 1501425
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
Try builds just finished, except an intermittent failure for a mochitest all is green - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a362a177bec344523e9ed3b21b7b36ae3a315574
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.
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0a9b2e68b8f
1 - Set valid clientId and save if previous was canary clientId; r=jchen
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
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
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(petru.lingurar)
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:
Flags: needinfo?(petru.lingurar)
Attachment #9021112 - Flags: approval-mozilla-beta?
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:
Attachment #9021116 - Flags: approval-mozilla-beta?
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:
Attachment #9021118 - Flags: approval-mozilla-beta?
Blocks: 1492682
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.
Probably 60 seconds, the default delay we wait for fully initializing Telemetry.
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.
Attachment #9021112 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9021116 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9021118 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Flags: in-testsuite+
Duplicate of this bug: 1492682
Duplicate of this bug: 1492653
Duplicate of this bug: 1492307
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.
Flags: qe-verify+
Petru, can you request uplift for your patch to Release now that it has been verified in Beta? Thanks
Flags: needinfo?(petru.lingurar)
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:
Flags: needinfo?(petru.lingurar)
Attachment #9021112 - Flags: approval-mozilla-release?
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:
Attachment #9021116 - Flags: approval-mozilla-release?
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:
Attachment #9021118 - Flags: approval-mozilla-release?
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.
Attachment #9021112 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #9021116 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #9021118 - Flags: approval-mozilla-release? → approval-mozilla-release+
Verified as fixed on build 63.0.2 following the same steps as in comment 25 and with the same findings.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.