1029031 - Record the currently selected search provider in FHR

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Benjamin Smedberg]

Record in the FHR payload which search provider is the default.

This should be recorded in the FHR payload; for now, this should only record when telemetry is enabled: after we have proved the data and done more work on user-facing tips we plan to turn this on by default as part of FHR.

Comment 1

[Benjamin Smedberg]

It is our intention to get this fixed ASAP and uplift into beta on Thursday if possible to support the search experiment.

Comment 2

[Gregory Szorc [:gps]]

Benjamin told me to take this.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Record default search provider in FHR

My only open question is whether we want to differentiate between
internal/bundled vs external search providers. In the search counts
measurement, if nsISearchEngine.identifier is not set, we prefix with
"other-". It's easy enough to change if that's what you want.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8445554 [details] [diff] [review]
Record default search provider in FHR

We should use the same identifiers that we use for the searches.

So e.g.

        "org.mozilla.searches.counts": {
          "_v": 3,
          "bing.searchbar": 1,
          "other-DuckDuckGo.searchbar": 3
        },

So when reporting the selected engine, it would be:

"bing" or "other-DuckDuckGo"

Comment 5

[Gregory Szorc [:gps]]

Attachment: Rewrite test to use tasks

An upcoming patch will add more code to
browser_urlbar_search_healthreport.js. We now have support for running
tasks in bc tests, so let's use it to make my life easier.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Record default search provider in FHR

Now with a BC test.

I recycled the urlbar test. I feel a little dirty for that. But, I
didn't want to create a whole new test for this.

https://tbpl.mozilla.org/?tree=Try&rev=57d312c4f48e

Comment 7

[Gregory Szorc [:gps]]

There was a vim-induced typo in the xpcshell test causing it to fail. Re-trying.

https://tbpl.mozilla.org/?tree=Try&rev=dec1fb407752

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8446084 [details] [diff] [review]
Record default search provider in FHR

Review of attachment 8446084 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/tests/xpcshell/test_provider_searches.js
@@ +177,5 @@
> +  Services.search.defaultEngine = engine1;
> +
> +  yield provider.collectDailyData();
> +  data = yield g.Values();
> +  Assert.equal(data.days.getDay(now).get("default"), "other-testdefault");

It would be nice to also have a test for an engine which has an identifier (e.g. a shipped one) and not other-* but it seems unlikely to break.

Comment 9

[Gregory Szorc [:gps]]

Boo test failure.

The failing test is testing whether the telemetry pref is honored.

My guess is it is failing because something else during the bc run is triggering a collection and populating the data. We have that coverage in the xpcshell test. So I'm just going to remove that test and land it.

Comment 10

[Gregory Szorc [:gps]]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab681f144bea
https://hg.mozilla.org/mozilla-central/rev/b77106f50779
https://hg.mozilla.org/mozilla-central/rev/11c29320d66a

Comment 12

[Marco Mucci [:MarcoM]]

Hi Florin, can a contact be assigned for QA verification.

Comment 13

[Benjamin Smedberg]

Comment on attachment 8446084 [details] [diff] [review]
Record default search provider in FHR

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Needed for search experiment
Testing completed (on m-c, etc.): landed, manual testing
Risk to taking this patch (and alternatives if risky): relatively low risk, and the risk is entirely contained to the FHR data collection path
String or IDL/UUID changes made by this patch: none

Comment 14

[Gregory Szorc [:gps]]

Whoever does the uplift, please fold 11c29320d66a into b77106f50779.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/28c71f97aaf1
https://hg.mozilla.org/releases/mozilla-aurora/rev/98a0b7def57a

https://hg.mozilla.org/releases/mozilla-beta/rev/eea86e68648b
https://hg.mozilla.org/releases/mozilla-beta/rev/45c11dc1d609

Comment 16

[Gregory Szorc [:gps]]

beta/test only bustage follow-up (it fixed things on my local machine). Something must have changed in search/fhr land to cause the race to go away in new versions. Most weird. Or, I inadvertently introduced a new intermittent failure with these patches. We should know in a few days :)

https://hg.mozilla.org/releases/mozilla-beta/rev/aef4862304e6

Comment 17

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified the implementation on Windows 7 x64, Windows 8 x64, Mac OS 10.9.3 and Ubuntu 13.04 x64, on Firefox 31 Beta 5 (BuildID: 	20140626181429), latest Firefox 32 Aurora (BuildID: 20140627004000), and latest Firefox 33 Nightly (BuildID: 20140627030212). 

In about:healthreport, I see entries like:
"org.mozilla.searches.engines": {
          "_v": 1,
          "default": "wikipedia" 
}
"org.mozilla.searches.engines": {
          "_v": 1,
          "default": "other-DuckDuckGo"
}

If I disable Telemetry (about:telemetry) then the entry no longer changes when changing the search engine (and restarting Firefox). When Telemetry is re-enabled, the entry changes each time the search engine is changed (and Firefox restarted). If Telemetry is disabled from the start, then there is no "org.mozilla.searches.engines" entry. 

All search engines (including custom ones) are correctly recorded.

Comment 18

[Dave Zeber [:dzeber]]

For clarification: what definition of "default provider" are we using here - is it the one that's currently showing in the search box? 

Say I change the search box provider once or twice during the session, and my searches.counts shows searches from several providers on a given day. What does my default provider register as?

Comment 19

[Gregory Szorc [:gps]]

The documentation answers the "default provider" question: https://hg.mozilla.org/mozilla-central/file/36a5f54bd12b/services/healthreport/docs/dataformat.rst#l1429

The reported value is the default provider at the time FHR collects data. It's possible for there to be a 1 reported day lag between provider changes and the FHR payload. This window could presumably be closed if it's important.

Comment 20

[Benjamin Smedberg]

That doesn't really define what "default" means in this context. As I understand it, "default" here is a synonym for "currently selected", right?

Comment 21

[Gregory Szorc [:gps]]

I can't add any additional context beyond what nsIBrowserSearchService.defaultEngine's docs say: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIBrowserSearchService.idl#388

Comment 22

[Mike Connor [:mconnor]]

Why are we using defaultEngine instead of currentEngine ? defaultEngine won't tell us if a different engine is in use.

If we're recording _both_, however, we'll know approximately how many users switch back from hijack attempts...

Comment 23

[Robert Kaiser]

(In reply to Mike Connor [:mconnor] from comment #22)
> Why are we using defaultEngine instead of currentEngine ? defaultEngine
> won't tell us if a different engine is in use.

FWIW, I thought that some time ago, we made both be synchronized anyhow.

Comment 24

[Gregory Szorc [:gps]]

I have no clue what the difference between defaultEngine and currentEngine is. The docs in the IDL are somewhat ambiguous. They kinda sound like the same thing to me. "The default engine is the engine that gets used by default, right?" I am under the impression that switching search engines updates .defaultEngine.

In my defense and in the defense of the reviewer, the word "default" is used a lot in the summary and comments. It's easy to see how that got translated into choosing .defaultEngine over .currentEngine.

In either case, this has been on Aurora and Beta for 2 weeks. We should have data to prove or refute any claim.

MattN: Can you enlighten us on the differences between .defaultEngine and .currentEngine?

Comment 25

[Mike Connor [:mconnor]]

The difference is somewhat moot, because we have code that syncs these when selectedEngine changes, and cleans up differences.  What I'm really wishing for is what is now implemented internally as _originalCurrentEngine, i.e. finding what the base value is, rather than the currently selected value.

What we have is fine for the original purpose, but if we want to get better at diagnostics we may need to go deeper.  We can figure that out in bug 1029063 .