1180790 - Stop resetting the client id on FHR opt-out

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

With the system changes for unified Telemetry, we won't reset the client id anymore.
This change is reasonable privacy-wise as we already persist historical ping data with the client id on disk.

Now, if/while we hold back unified Telemetry from release, we should avoid corner-case and data inconsistencies by stopping FHR/DRS to reset the saved client id.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1180790.patch

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c512b904238

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8631524 [details] [diff] [review]
bug1180790.patch

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

I see some more - now unused - code related to this:
https://dxr.mozilla.org/mozilla-central/search?q=resetClientID

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +757,5 @@
>      do_check_null(reporter.lastSubmitID);
>      do_check_false(reporter.haveRemoteData());
>      do_check_false(server.hasDocument(reporter.serverNamespace, id));
>  
> +    // Client ID should be updated.

Nit: Just "Client ID should stay the same" and drop the other comment?

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1180790.patch - v2

Ouch, thanks.

Comment 4

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=526fc4185bb6

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/24ea36d26155

Comment 6

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=24ea36d26155

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24ea36d26155

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8631535 [details] [diff] [review]
bug1180790.patch - v2

Approval Request Comment
[Feature/regressing bug #]:
This is part of the uplift3 batch for the unified Telemetry project: http://bit.ly/1TCl4r8
This is targetting 41 now and these are the last minor "feature" patches blocking shipping - any further patches will be fixes for data quality etc. with very limited scope & impact.
[User impact if declined]:
Unified Telemetry can't ship.
This is a change needed for data consistency.
[Describe test coverage new/current, TreeHerder]:
This has good automated test-coverage.
[Risks and why]:
Low-risk, well understood change that only removes one action.
[String/UUID change made/needed]:
No string changes.

Comment 9

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

Tracked. Also, requesting QE team to verify this fix along with all other Unified Telemetry related fixes. Thanks!

Comment 10

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

Comment on attachment 8631535 [details] [diff] [review]
bug1180790.patch - v2

Should be safe to uplift as the patch has been in m-c for a few days and the patch is mainly removing the code where clientID was being reset.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a047db3341e4

Comment 12

[Ada [:adalucinet]]

Verified fixed on 41.0b8, across platforms [1] - no clientID reset encountered during our testing.

[1] Mac OS X 10.10.4, Ubuntu 14.04 32-bit and Windows 7 64-bit