Closed Bug 1474365 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: tcsc, Assigned: fauxwizard_, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 5 obsolete files)

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
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.
The second link is for context or future reference and you don't have to worry about it.
Can you guide me on how I do send my code for review? Thanks
Attached patch Sync telemetry patch (obsolete) — Splinter Review
I have made changes as mentioned in the bug. How do I get it reviewed?
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.
Attachment #8991077 - Attachment is obsolete: true
Attachment #8991114 - Flags: review?(tchiovoloni)
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)
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?
Attachment #8991114 - Attachment is obsolete: true
Attachment #8991125 - Flags: review?(tchiovoloni)
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+
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
(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)
(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)
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-
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+
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)
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
: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
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.
https://hg.mozilla.org/mozilla-central/rev/b2d1139d20af
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: needinfo?(tchiovoloni)
You need to log in before you can comment on or make changes to this bug.