Closed
Bug 1248399
Opened 8 years ago
Closed 8 years ago
Update browser.snippets.geoURL and handling code to use MLS instead of geodude
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox47 fixed, firefox48 fixed, fennec47+)
RESOLVED
FIXED
Firefox 48
People
(Reporter: giorgos, Assigned: Margaret)
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
giorgos
:
review+
Grisha
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
Geodude is deprecated and will be decommissioned in the future. Please use the following URL to fetch user's country https://location.services.mozilla.com/v1/country?key=fff72d56-b040-4205-9a11-82feda9d83a3 Also add a comment to prevent others from using this URL and instead point them to the MLS team for their own API key. Something between the lines """ Please do not directly use this code or Snippets key. Contact MLS team for your own credentials. https://location.services.mozilla.com/contact """ MLS implements a different API so changes to the code must be made to handle the new response. Thanks!
Comment 1•8 years ago
|
||
cc'ing ckolos from ops. This is the Fennec side of snippets.
As part of the decom for this, we need to be careful about existing Firefox installations - there is code compiled into the client that performs the GET https://geo.mozilla.org/country.js and we need to ensure that our decom approach treats that code appropriately (and test what happens).
Comment 3•8 years ago
|
||
Margaret, Barbara: the geo endpoint is going to continue running until it… doesn't. It hasn't seen a human hand in years, and we shouldn't be using it when it goes away. We're the last users, AIUI. Can we get an assignee to switch out its usage in mobile/android/components/Snippets.js so ops can decom this service?
tracking-fennec: --- → ?
Flags: needinfo?(margaret.leibovic)
(In reply to Richard Newman [:rnewman] from comment #3) > Margaret, Barbara: the geo endpoint is going to continue running until it… > doesn't. It hasn't seen a human hand in years, and we shouldn't be using it > when it goes away. We're the last users, AIUI. We've been working since shortly before Orlando to track down and migrate each user of the older cluster. There are data and SLA issues with the old cluster that cannot be resolved without commissioning a new cluster, and thus duplicating the work the MLS team has put into their high-SLA cluster. The parent bug here lists the users of this service. The other two endpoints are server-side, that can be updated without client code updates. As I understand it, one has switched, another just committed the pull request, and the third is Fennec (this bug). Of those three, Fennec is the most likely to consume significant time (up to 24 weeks, if Fennec release cycles have any resemblance to Desktop), so this bug is now my top priority for the GeoIP to MLS migration. I'm available here, by email/IRC, or on Vidyo, to help answer any questions that can help make this happen.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #3) > Margaret, Barbara: the geo endpoint is going to continue running until it… > doesn't. It hasn't seen a human hand in years, and we shouldn't be using it > when it goes away. We're the last users, AIUI. > > Can we get an assignee to switch out its usage in > mobile/android/components/Snippets.js so ops can decom this service? I'm the owner, so I can do it.
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
Updated•8 years ago
|
tracking-fennec: ? → 47+
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41795/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41795/
Attachment #8733515 -
Flags: review?(gkruglov)
Attachment #8733515 -
Flags: review?(giorgos)
Assignee | ||
Comment 7•8 years ago
|
||
I tested this and verified the snippets logic still worked. The response JSON from both endpoints includes the country_code property, and that's all we use. Grisha, I picked you to help review this to give you a taste of snippets code. I'm basically the only person who works on snippets, and it would be good to have another person familiar with this code :)
Reporter | ||
Comment 8•8 years ago
|
||
Aha you were using the json endpoint with geodude, so this is an easy change. Code looks good, thanks Margaret!
Reporter | ||
Updated•8 years ago
|
Attachment #8733515 -
Flags: review?(giorgos) → review+
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8733515 [details] MozReview Request: Bug 1248399 - Update browser.snippets.geoURL and handling code to use MLS instead of geodude. r=giorgos,grisha https://reviewboard.mozilla.org/r/41795/#review38407 Looks good!
Comment 10•8 years ago
|
||
feedback+ from the MLS service side. We intentionally kept the JSON API very similar to the one from geodude, so this payed off :)
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/189d3e93d615c6d79cb12fb5a05bdd5139a14e52 Bug 1248399 - Update browser.snippets.geoURL and handling code to use MLS instead of geodude. r=giorgos,grisha
Assignee | ||
Comment 12•8 years ago
|
||
Oops, I misread the comments and thought I saw an r+ from grisha... I'll leave this as landed, but wait for a review before requesting uplift.
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/189d3e93d615
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 14•8 years ago
|
||
Comment on attachment 8733515 [details] MozReview Request: Bug 1248399 - Update browser.snippets.geoURL and handling code to use MLS instead of geodude. r=giorgos,grisha https://reviewboard.mozilla.org/r/41795/#review39289 ::: mobile/android/app/mobile.js:868 (Diff revision 1) > > // How frequently we check for new snippets, in seconds (1 day) > pref("browser.snippets.updateInterval", 86400); > > -// URL used to check for user's country code > -pref("browser.snippets.geoUrl", "https://geo.mozilla.org/country.json"); > +// URL used to check for user's country code. Please do not directly use this code or Snippets key. > +// Contact MLS team for your own credentials. https://location.services.mozilla.com/contact Is it possible to not hard-code the key directly here, but rather expose it as a variable somewhere else? It seems a bit lost here, and likely easy to miss if we don't want people re-using our credentials. I suppose it depends on where downstream repackagers are expecting to find this stuff.
Attachment #8733515 -
Flags: review?(gkruglov)
Comment 15•8 years ago
|
||
(In reply to :Grisha Kruglov from comment #14) > Is it possible to not hard-code the key directly here, but rather expose it > as a variable somewhere else? It seems a bit lost here, and likely easy to > miss if we don't want people re-using our credentials. > I suppose it depends on where downstream repackagers are expecting to find > this stuff. From the perspective of the MLS service, putting the key here is totally fine. For the country API the API key is only used for metrics purposes, to allow us to distinguish between different components of Firefox, like the search service, snippets, heartbeat, or other Mozilla websites calling the service. So think of it more like we'd put "https://.../v1/country?clientid=snippets" here. We just choose the spell that as "?key=<some uuid>", as it was easier to handle it this way on the service end. We do have semi-secret variables in place for actual precise geolocation usage, with GOOGLE_API_KEY and the MLS-specific and badly named MOZILLA_API_KEY. But those need to be put into releng's secret repositories, deployed to all build machines and configured during build time. All of that still doesn't cover test and developer machines, so some fallback needs to be in place for them. That's a whole lot of hassle, that isn't required for this API.
Comment 16•8 years ago
|
||
Thanks for clarifying, Hanno!
Comment 17•8 years ago
|
||
Comment on attachment 8733515 [details] MozReview Request: Bug 1248399 - Update browser.snippets.geoURL and handling code to use MLS instead of geodude. r=giorgos,grisha https://reviewboard.mozilla.org/r/41795/#review39363
Attachment #8733515 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8733515 [details] MozReview Request: Bug 1248399 - Update browser.snippets.geoURL and handling code to use MLS instead of geodude. r=giorgos,grisha Approval Request Comment [Feature/regressing bug #]: none [User impact if declined]: traffic continues to visit geo.mozilla.org [Describe test coverage new/current, TreeHerder]: tested locally, baked on Nightly [Risks and why]: low-risk, pref change for different geolocation server for snippets (change only affects snippets) [String/UUID change made/needed]: none
Attachment #8733515 -
Flags: approval-mozilla-beta?
Attachment #8733515 -
Flags: approval-mozilla-aurora?
Comment on attachment 8733515 [details] MozReview Request: Bug 1248399 - Update browser.snippets.geoURL and handling code to use MLS instead of geodude. r=giorgos,grisha Baked on Nightly, Aurora48+, Beta47+
Attachment #8733515 -
Flags: approval-mozilla-beta?
Attachment #8733515 -
Flags: approval-mozilla-beta+
Attachment #8733515 -
Flags: approval-mozilla-aurora?
Attachment #8733515 -
Flags: approval-mozilla-aurora+
status-firefox47:
--- → affected
This appears to already be on aurora. Was there something more to uplift beyond https://hg.mozilla.org/mozilla-central/rev/189d3e93d615
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/60fa90929489
Assignee | ||
Comment 22•8 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #20) > This appears to already be on aurora. Was there something more to uplift > beyond https://hg.mozilla.org/mozilla-central/rev/189d3e93d615 Sorry, my bad, this already landed in 48. Carry on!
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•