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)

defect
Not set
normal

Tracking

(firefox47 fixed, firefox48 fixed, fennec47+)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
fennec 47+ ---

People

(Reporter: giorgos, Assigned: Margaret)

Details

Attachments

(1 file)

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!
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).
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.
(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)
tracking-fennec: ? → 47+
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 :)
Aha you were using the json endpoint with geodude, so this is an easy change. Code looks good, thanks Margaret!
Attachment #8733515 - Flags: review?(giorgos) → review+
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!
feedback+ from the MLS service side. We intentionally kept the JSON API very similar to the one from geodude, so this payed off :)
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
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.
https://hg.mozilla.org/mozilla-central/rev/189d3e93d615
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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)
(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.
Thanks for clarifying, Hanno!
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+
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+
This appears to already be on aurora. Was there something more to uplift beyond https://hg.mozilla.org/mozilla-central/rev/189d3e93d615
(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!
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.