Closed Bug 1553558 Opened 7 months ago Closed 4 months ago

Search service should do its own countryCode fetching as-needed instead of having system-info init get it eagerly

Categories

(Firefox :: Search, task, P3)

68 Branch
task

Tracking

()

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: Gijs, Assigned: mandycheang135)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p2] [fxperfsize:S])

Attachments

(1 file)

Talking to the registry for data that only the search service needs and only for some telemetry is not really the best for performance, considering when the system info service is initialized.

We should check that the telemetry is still needed. If not, we can just remove all the associated code. If it is needed, we can move the OS-based lookups elsewhere, e.g. to a property on xpcom/system/nsIXULRuntime.idl that does the work the first time someone asks for it, and then do the requesting only after startup, off an idle task.

Drew, do you know if this telemetry (added in bug 1121340) is still important?

Flags: needinfo?(adw)
Type: defect → task

I don't know. Mike, do you know?

Flags: needinfo?(adw) → needinfo?(mconnor)
Assignee: nobody → mcheang

The telemetry is still important, we need to keep it around a while longer.

Flags: needinfo?(mconnor)

Perhaps we can make the country code something that Telemetry is responsible for getting (lazily, and maybe off of the main thread).

Hey, Georg - is there precedent for Telemetry gathering and hosting information, not to send as part of a ping, but for other parts of Firefox to make decisions on? Specifically, would it be outlandish to have Telemetry be responsible for retrieving and storing the platform OS's country code value for the Search Service to use here?: https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/toolkit/components/search/SearchService.jsm#186-212

rather than storing the country code in nsSystemInfo (which isn't currently designed to get any of its values lazily / off-main-thread).

Flags: needinfo?(gfritzsche)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #3)

Hey, Georg - is there precedent for Telemetry gathering and hosting information, not to send as part of a ping, but for other parts of Firefox to make decisions on? Specifically, would it be outlandish to have Telemetry be responsible for retrieving and storing the platform OS's country code value for the Search Service to use here?: https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/toolkit/components/search/SearchService.jsm#186-212

There is no precedent for that AFAIK. I think currently the Telemetry interface are not ideal for exposing information to code inside Firefox.
Is there value from exposing this through Telemetry vs. another interface?

ni?chutten for his perspective.

Flags: needinfo?(gfritzsche) → needinfo?(chutten)

Telemetry at present is write-only except for test APIs, so it doesn't seem like it'd be a good fit.

Flags: needinfo?(chutten)

(In reply to Mike de Boer [:mikedeboer] from comment #2)

The telemetry is still important, we need to keep it around a while longer.

Can you elaborate a bit? We've had this telemetry for 3+ years. Do we still not know whether our geoip determination is more/less accurate than the OS one? Does that change a lot? Have we ever switched away from our geoip one to the OS one?

FWIW, analogous to the patch in bug 1553540 we can probably remove this from the existing Init() in the system info service, and instead add a promise attribute to the nsISystemInfo idl that fetches the mac/windows bits OMT, and have the search service code call that when it actually wants this stuff.

Flags: needinfo?(mdeboer)

(In reply to :Gijs (he/him) from comment #6)

(In reply to Mike de Boer [:mikedeboer] from comment #2)

The telemetry is still important, we need to keep it around a while longer.

Can you elaborate a bit? We've had this telemetry for 3+ years. Do we still not know whether our geoip determination is more/less accurate than the OS one? Does that change a lot? Have we ever switched away from our geoip one to the OS one?

Well, it's been a while indeed and we've only now solidified the plans to implement changes here in Q3 of this year with the goal to improve the accuracy. This probe will tell us if we succeeded, eventually.

Flags: needinfo?(mdeboer)
Priority: -- → P3

ni?ing tcampbell for a quick second opinion on https://phabricator.services.mozilla.com/D39343#inline-240889.

Flags: needinfo?(tcampbell)

(Replied on phabricator)

Flags: needinfo?(tcampbell)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6368c1261d84
collect countryCode in system info off the main thread. r=mconley,Standard8
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
You need to log in before you can comment on or make changes to this bug.