Closed Bug 1121340 Opened 5 years ago Closed 5 years ago

Add telemetry to compare OSX system country data with geoip data

Categories

(Firefox :: Search, defect)

All
macOS
defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox37 + fixed
firefox38 --- fixed

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?
Flags: qe-verify-
Flags: needinfo?(gavin.sharp)
Flags: firefox-backlog+
Sounds good to me. smichaud may be able to point to documentation about what kind of country codes are returned by this API.
Flags: needinfo?(gavin.sharp) → needinfo?(smichaud)
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).
Flags: needinfo?(smichaud)
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?
Flags: needinfo?(smichaud)
> do you see any problems with the probes I'm suggesting?

No, I don't :-)
Flags: needinfo?(smichaud)
Iteration: 38.3 - 23 Feb → 38.2 - 9 Feb
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?
Attachment #8556328 - Flags: feedback?(gavin.sharp)
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.
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.
Attachment #8556328 - Flags: feedback?(gavin.sharp) → feedback+
(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?
Flags: needinfo?(gavin.sharp)
Not sure it's good to rely on that, I'd just leave it as-is.
Flags: needinfo?(gavin.sharp)
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?
Attachment #8556328 - Flags: review?(gavin.sharp)
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
Attachment #8556328 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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
https://hg.mozilla.org/mozilla-central/rev/eef81ecfb729
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Mark, let's get this uplifted to 37.
Flags: needinfo?(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
Flags: needinfo?(mhammond)
Attachment #8556328 - Flags: approval-mozilla-aurora?
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+
Attachment #8556328 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Whiteboard: [sheriff note in comment 16]
Whiteboard: [sheriff note in comment 16] → [sheriff note in comment 17]
Depends on: 1553558
You need to log in before you can comment on or make changes to this bug.