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

RESOLVED FIXED in Firefox 63

Status

()

RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: tcsc, Assigned: fauxwizard_, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 63
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

9 months ago
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
(Assignee)

Comment 1

8 months ago
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.
(Reporter)

Comment 2

8 months ago
The second link is for context or future reference and you don't have to worry about it.
(Assignee)

Comment 3

8 months ago
Can you guide me on how I do send my code for review? Thanks
(Assignee)

Comment 4

8 months ago
Posted patch Sync telemetry patch (obsolete) — Splinter Review
I have made changes as mentioned in the bug. How do I get it reviewed?
(Reporter)

Comment 5

8 months ago
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.
(Reporter)

Updated

8 months ago
Attachment #8991077 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8991114 - Flags: review?(tchiovoloni)
(Reporter)

Comment 7

8 months ago
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.
Attachment #8991114 - Flags: review?(tchiovoloni)
(Assignee)

Comment 8

8 months ago
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?
(Assignee)

Comment 9

8 months ago
Attachment #8991114 - Attachment is obsolete: true
Attachment #8991125 - Flags: review?(tchiovoloni)
(Reporter)

Comment 10

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

LGTM
Attachment #8991125 - Flags: review?(tchiovoloni) → review+
(Reporter)

Updated

8 months ago
Keywords: checkin-needed
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)
Flags: needinfo?(karbelkar.mihir)
Keywords: checkin-needed
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.
Assignee: nobody → karbelkar.mihir
(Assignee)

Comment 13

8 months ago
(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)
(Assignee)

Comment 14

8 months ago
(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)
Flags: needinfo?(karbelkar.mihir) → needinfo?(tchiovoloni)
(Assignee)

Comment 15

8 months ago
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.
Attachment #8991125 - Attachment is obsolete: true
Attachment #8991691 - Flags: review?(markh)
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)
Attachment #8991691 - Flags: review?(markh) → review-
(Assignee)

Comment 17

8 months ago
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.
Attachment #8991691 - Attachment is obsolete: true
Attachment #8991795 - Flags: review?(markh)
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!
Attachment #8991795 - Flags: review?(markh) → review+
Keywords: checkin-needed

Comment 19

8 months ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21285bab4f03
Prevent submitting sync telemetry for self hosting users. r=markh
Keywords: checkin-needed
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 ===
Flags: needinfo?(karbelkar.mihir)
(Assignee)

Comment 21

8 months ago
I hope this should do it. Checked manually and by program all the tabs in the file if any.
Attachment #8991795 - Attachment is obsolete: true
Flags: needinfo?(karbelkar.mihir)
Attachment #8991948 - Flags: review?(markh)
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
Keywords: checkin-needed
:markh please add checkin-needed only after the patch has received a review+. Thank you.
Keywords: checkin-needed
Attachment #8991948 - Flags: review?(markh) → review+
(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!
Keywords: checkin-needed

Comment 25

8 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d1139d20af
Prevent submitting sync telemetry for self hosting users. r=markh
Keywords: checkin-needed
No problem, patch has been landed.

Comment 27

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2d1139d20af
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Reporter)

Updated

8 months ago
Flags: needinfo?(tchiovoloni)
You need to log in before you can comment on or make changes to this bug.