1116383 - Better telemetry for geo ip lookup failures

Status: RESOLVED WONTFIX

(Firefox :: Search, defect, P3)

Description

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

The geoip lookups added in bug 1109120 should report telemetry for cases (other than timeouts) where we fail to get a country code - currently we only see that the request was not successful, but have no insight into why.

There are a couple of hurdles to this:

* We need to get the error reason from the XHR request.  This *seems* to be as simple as querying:

  request.channel.QueryInterface(Ci.nsIRequest).status

to get the NS_ERROR_* value in the onerror handler.

* It's not clear how "package" these codes for telemetry.  A reasonable first attempt might be to have a "keyed" histogram and to use the error code's module as the key (so, eg, we'd know if the problem is "network" or "security", but not be able to break it down further).  Or we might be able to use a key that captures both the specific codes we expect (presumably ones reflecting "no network" and "cert error due to captive portal") *and* simple module names for other errors.

* It's not clear how we would test this.  xpcshell tests do *not* get the list of hosts in server-locations.txt.  I can't find *any* xpcshell tests that use https:// (other than one spdy test that uses a node server, has special support in runxpcshelltests.py, and isn't enabled unless the user installs node and sets an env var to point at it)

And related to the above, I just noticed the test test_location_error.js does *not* do what I thought it did - it says:

  // from server-locations.txt, we choose a URL without a cert.
  let url = "https://nocert.example.com:443";

but in reality this test is just causing an NS_ERROR_UNKNOWN_HOST error rather than the a SEC_ERROR_*_CERTIFICATE error I was expecting.  However, I did manually check that a certificate error does cause onerror to be called (and the nsIRequest.status hack above to report the correct error) so this is a test-only issue - at a minimum the comments in the test should change, but ideally we'd work out how to get an actual certificate error.

Comment 1

[Mark Banner (:standard8)]

This is referring to telemetry that has now been removed/changed. However in discussing with Dale, the region services handles failures fairly well, so there's not a big need for adding additional telemetry here.