1474365 - Sync telemetry still sent for self hosting users that configure server using services.sync.tokenServerURI pref

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

In our telemetry code [0] we check if the "identity.sync.tokenserver.uri" pref is set to determine if the user is a self hoster, however we support configuring this via the legacy "services.sync.tokenServerURI" pref as well [1]. If a user configured it this way, we'd still send sync pings from them, when we shouldn't.

This should just adding `|| Services.prefs.prefHasUserValue("services.sync.tokenServerURI")` after the if at the start of submit at [0].

[0]: https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/services/sync/modules/telemetry.js#516
[1]: https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/services/sync/modules/browserid_identity.js#427

Comment 1

[Mihir Karbelkar(:fauxwizard_)]

Hi! I am new to open source and I would like to work on this bug. So I just have to change the if condition? What about the second link? thanks.

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

The second link is for context or future reference and you don't have to worry about it.

Comment 3

[Mihir Karbelkar(:fauxwizard_)]

Can you guide me on how I do send my code for review? Thanks

Comment 4

[Mihir Karbelkar(:fauxwizard_)]

Attachment: Sync telemetry patch

I have made changes as mentioned in the bug. How do I get it reviewed?

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

It's probably easiest for you to attach a patch here, the way you did. If the commit message ends in r?tcsc I'll be notified and review it.

That said, I don't think that patch is what you intended to attach, since it's a patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1471573.

Comment 6

[Mihir Karbelkar(:fauxwizard_)]

Attachment: Sync telemetry sent for self hosting users

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8991114 [details] [diff] [review]
Sync telemetry sent for self hosting users

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

This is more or less exactly what I expected. The only issue is the commit message and the trailing space.

When you resubmit the commit message should be more descriptive, like "Prevent submitting sync telemetry for self hosting users" (the current message looks to me like we're starting to submit this, when it's the opposite).

Upload a patch with these fixed, and we'll get it landed!

::: services/sync/modules/telemetry.js
@@ +513,4 @@
>    }
>  
>    submit(record) {
> +    if (Services.prefs.prefHasUserValue("identity.sync.tokenserver.uri") || 

Please delete the trailing space from this line.

Comment 8

[Mihir Karbelkar(:fauxwizard_)]

I have made another commit removing the trailing space and editing the commit message. Is it necessary to attach a patch here even after committing or either one works?

Comment 9

[Mihir Karbelkar(:fauxwizard_)]

Attachment: Patch with all the required changes. (No trailing space, proper commit message)

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8991125 [details] [diff] [review]
Patch with all the required changes. (No trailing space, proper commit message)

LGTM

Comment 11

[Cosmin Sabou [:CosminS]]

This patch failed to apply, please take a look:
applying change.patch
patching file services/sync/modules/telemetry.js
Hunk #1 FAILED at 512
1 out of 1 hunks FAILED -- saving rejects to file services/sync/modules/telemetry.js.rej
patch failed, unable to continue (try -v)

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Hi Mihor,
  It looks like the most recent patch you attached was an update to the first one you updated - ie, it only shows the trailing whitespace being removed. You probably need to look into rebasing your mercurial patch (https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial might help) and uploading a single patch which includes all the changes you made.

Comment 13

[Mihir Karbelkar(:fauxwizard_)]

(In reply to Mark Hammond [:markh] from comment #12)
> Hi Mihor,
>   It looks like the most recent patch you attached was an update to the
> first one you updated - ie, it only shows the trailing whitespace being
> removed. You probably need to look into rebasing your mercurial patch
> (https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial might
> help) and uploading a single patch which includes all the changes you made.

I am unable to rebase. I have added the extension hg rebase upon trying to rebase to my changeset I get a message saying :
abort: branch 'default' has 13 heads - please rebase to an explicit rev
(run 'hg heads .' to see heads)

Comment 14

[Mihir Karbelkar(:fauxwizard_)]

(In reply to Cosmin Sabou [:CosminS] from comment #11)
> This patch failed to apply, please take a look:
> applying change.patch
> patching file services/sync/modules/telemetry.js
> Hunk #1 FAILED at 512
> 1 out of 1 hunks FAILED -- saving rejects to file
> services/sync/modules/telemetry.js.rej
> patch failed, unable to continue (try -v)

Comment 15

[Mihir Karbelkar(:fauxwizard_)]

Attachment: Patch with all the required changes and previous code. (No trailing space, proper commit message)

I have uploaded the patch with all the changes I have made till now. Thanks for the help. But I probably need to look into rebasing,  how it's done. The previous patch was review by Thom Chiovoloni, so I am flagging this to be reviewed by Mark Hammond as he requested me to make changes and I would like to know if all the changes were done as required. Thanks, people at Mozzila have been really helpful as this was my first bug.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8991691 [details] [diff] [review]
Patch with all the required changes and previous code. (No trailing space, proper commit message)

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

Thanks for persevering! Unfortunately some other changes are necessary.

Note that you still need to upload a hg changeset, much like you did last time (although you'll use r?markh if you are asking me for review) - the patch you uploaded here doesn't have a description etc, which is necessary for it to be checked in.

I suspect you are having trouble rebasing either because you didn't actually commit your changes (ie, so |hg diff| continued to show them), or because you committed them to the "tip", which would mean it doesn't know how to rebase and you would have trouble updating. If you follow the process at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/bookmarks.html you should be on the right path, although you may still need to reset things before that actually works for you.

Let me know if you have followed those instructions and still have problems - getting your head around hg can be difficult!

::: services/sync/modules/telemetry.js
@@ +513,4 @@
>    }
>  
>    submit(record) {
> +    if (Services.prefs.prefHasUserValue("identity.sync.tokenserver.uri")||

While this is nitpicking, our automated code checks will get upset - you must add a space between the ")" and the "||"

@@ +513,5 @@
>    }
>  
>    submit(record) {
> +    if (Services.prefs.prefHasUserValue("identity.sync.tokenserver.uri")||
> +      Services.prefs.prefHasUserValue("services.sync.tokenServerURI")) {

and you should indent this line so the "Services" work is directly under the same word of the line above (ie, you need 2 more spaces at the start of this line)

Comment 17

[Mihir Karbelkar(:fauxwizard_)]

Attachment: Sync telemetry patch with all the final changes

I have made the changes. If further changes are required please do let me know I'll be happy to make the necessary changes, thanks.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8991795 [details] [diff] [review]
Sync telemetry patch with all the final changes

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

Looks great, thanks!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21285bab4f03
Prevent submitting sync telemetry for self hosting users. r=markh

Comment 20

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=21285bab4f03a13424c8ff0fec4d9c8d4576e20e&tochange=121c74871ff7a9c86f1a79a21fd0be3f52cc1dba&selectedJob=187981494&filter-searchStr=Linting%20opt%20source-test-mozlint-eslint%20(ES)

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187981494&repo=mozilla-inbound&lineNumber=66

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/121c74871ff7a9c86f1a79a21fd0be3f52cc1dba

[task 2018-07-13T06:50:29.858Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-07-13T06:50:29.858Z] 
[task 2018-07-13T06:50:29.858Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-07-13T06:55:41.624Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/services/sync/modules/telemetry.js:517:6 | Unexpected tab character. (no-tabs)
[taskcluster 2018-07-13 06:55:41.921Z] === Task Finished ===

Comment 21

[Mihir Karbelkar(:fauxwizard_)]

Attachment: The patch with no tabs and all things.

I hope this should do it. Checked manually and by program all the tabs in the file if any.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

I pushed this to try to avoid having it backed out again - if this looks green we can flag it as needing checkin again.

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

Comment 23

[Andreea Pavel [:apavel]]

:markh please add checkin-needed only after the patch has received a review+. Thank you.

Comment 24

[Mark Hammond [:markh] [:mhammond]]

(In reply to Andreea Pavel [:apavel] from comment #23)
> :markh please add checkin-needed only after the patch has received a
> review+. Thank you.

Oops, sorry, I thought I already did that!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d1139d20af
Prevent submitting sync telemetry for self hosting users. r=markh

Comment 26

[Andreea Pavel [:apavel]]

No problem, patch has been landed.

Comment 27

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b2d1139d20af