1121340 - Add telemetry to compare OSX system country data with geoip data

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

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

Bug 1102295 gave us exposure to a Mac API for fetching country data.  We'd like to compare the data that gives us with the country-code we fetch via geo-ip so we can ascertain if this would make a reasonable fallback (or possibly even default) in the case the geoip request fails.

In bug 1102295, it seems unclear exactly what the country codes returned are (ie, whether they are the ISO codes or Apple specific).  We do know the code for the US, and currently that's the only requirement for search, so I suggest we add 2 probes:

* Do the geoip and OSX calls agree about "is it US"?
* Do the geoip and OSX calls agree on the actual value?

Gavin, does that sound right to you?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Sounds good to me. smichaud may be able to point to documentation about what kind of country codes are returned by this API.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

As I said in bug 1102295 comment #13 and bug 1102295 comment #15, Apple has no documentation on the API used to fix that bug.  (Though it's also clear that Apple uses it themselves, so it's safe to use it.)

I suggest (as I already did at bug 1102295 comment #10) that someone come up with a list of country codes that we're "interested in".  Then we can check Apple's "CountryCode" for each of them (by deliberately choosing the wrong "closest city" in the Date & Time preference panel).

Comment 3

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

Thanks for the comment, and particularly the test program in bug 1102295. I did previously see your comments there about the lack of documentation on the country-codes, which is why I proposed the probes in comment 0.

(In reply to Steven Michaud from comment #2)
> As I said in bug 1102295 comment #13 and bug 1102295 comment #15, Apple has
> no documentation on the API used to fix that bug.  (Though it's also clear
> that Apple uses it themselves, so it's safe to use it.)

This is why one of the probes I want to add is specifically a check for "US" - this is the only one we know we care about at the moment.

> I suggest (as I already did at bug 1102295 comment #10) that someone come up
> with a list of country codes that we're "interested in".  Then we can check
> Apple's "CountryCode" for each of them (by deliberately choosing the wrong
> "closest city" in the Date & Time preference panel).

I suspect these are ISO 3166 codes.  Of the 20 or so random countries I tested, all correspond to the ISO code, including ones that aren't obvious from the abbreviation (eg, Ukraine is UA, United Arab Emirates is AE, Democratic Republic of the Congo is CD, "Myanmar/Burma" is MM, etc)

So while this obviously isn't conclusive, I think it's reasonable to suggest that Apple aren't likely to change existing codes (for the same reason we can consider the API safe to use) and it sure *looks* like it's using ISO codes - which is the reason I'm suggesting we add a second probe to see if we find a strong correlation between the ISO code our geoip service reports and the country-code the OSX technique offers.

Steven, other than us making assumptions about an undocumented API, do you see any problems with the probes I'm suggesting?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> do you see any problems with the probes I'm suggesting?

No, I don't :-)

Comment 5

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

Attachment: 0001-Bug-1121340-add-telemetry-to-determine-how-consisten.patch

This patch adds 2 new probes, both bools, and mutually exclusive:

* SEARCH_SERVICE_US_COUNTRY_MISMATCHED_PLATFORM_OSX is set if *either* geoip or OSX says we are in the US.  It is set to false if they both are (ie, no mismatch) and true if only 1 is.  There's no way of knowing *which* said US and which didn't (but I guess there's also no way of knowing which one is accurate in that case)

* SEARCH_SERVICE_NONUS_COUNTRY_MISMATCHED_PLATFORM_OSX is set when neither say US, and similarly, set to false if they agree and true is they don't.  There's no way to know what countries these might be.

Includes a test.  Is this what you had in mind?

Comment 6

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

Also, when using the telemetry dashboard, I couldn't find a way to combine probes or otherwise sort by platform.  If there is a way the "_OSX" suffix could be dropped.

Comment 7

[Matthew N. [:MattN]]

Not exactly sure what you mean by "combine probes or otherwise sort by platform" but you can filter by platform once you select reason=saved_session, appName=Firefox.

Comment 8

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

(In reply to Matthew N. [:MattN] from comment #7)
> Not exactly sure what you mean by "combine probes or otherwise sort by
> platform" but you can filter by platform once you select
> reason=saved_session, appName=Firefox.

Thanks Matt, that's exactly what I was after.  Gavin, that would make it possible to use a platform-agnostic probe name here with only a relatively minor inconvenience to filter by platform - do you think we should do that?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Not sure it's good to rely on that, I'd just leave it as-is.

Comment 10

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

Comment on attachment 8556328 [details] [diff] [review]
0001-Bug-1121340-add-telemetry-to-determine-how-consisten.patch

Gavin, can you propose a delegate for review?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8556328 [details] [diff] [review]
0001-Bug-1121340-add-telemetry-to-determine-how-consisten.patch

>diff --git a/toolkit/components/search/tests/xpcshell/head_search.js b/toolkit/components/search/tests/xpcshell/head_search.js

>+// Stash the real OS value so tests that need it don't find themselves with "XPCShell"
>+var OS = XULRuntime.OS;

Can you just mirror this to XULAppInfo instead, as with processType? Or does that break something?

>diff --git a/toolkit/components/search/tests/xpcshell/test_location.js b/toolkit/components/search/tests/xpcshell/test_location.js

>+    print("This OS is", OS)

remove

Comment 12

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

(In reply to :Gavin Sharp [email: [e-mail]] from comment #11)
> Can you just mirror this to XULAppInfo instead, as with processType? Or does
> that break something?

That works fine.

> >+    print("This OS is", OS)
> 
> remove

Done.

https://hg.mozilla.org/integration/fx-team/rev/eef81ecfb729

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Mark, let's get this uplifted to 37.

Comment 15

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

Comment on attachment 8556328 [details] [diff] [review]
0001-Bug-1121340-add-telemetry-to-determine-how-consisten.patch

Approval Request Comment
[Feature/regressing bug #]: Locale-based search defaults
[User impact if declined]: None, but we lose the ability to quickly measure how feasible it is to avoid geoip requests completely for OSX users.
[Describe test coverage new/current, TreeHerder]: Has tests
[Risks and why]: Low risk - only records telemetry.
[String/UUID change made/needed]: None

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8556328 [details] [diff] [review]
0001-Bug-1121340-add-telemetry-to-determine-how-consisten.patch

This will assist with information about how we set search defaults. Aurora+

Comment 17

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

Note to sheriffs - to avoid Aurora recording bad telemetry data, please uplift bug 1130132 at the same time (but note that this bug depends on that)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c5e12814f601