Record the currently selected search provider in FHR

VERIFIED FIXED in Firefox 31

Status

defect
VERIFIED FIXED
5 years ago
9 months ago

People

(Reporter: benjamin, Assigned: gps)

Tracking

unspecified
Firefox 33
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox31+ verified, firefox32+ verified, firefox33+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Points: --- → 5
Flags: firefox-backlog+
Whiteboard: [p=5][qa+] → [qa+]
It is our intention to get this fixed ASAP and uplift into beta on Thursday if possible to support the search experiment.
Benjamin told me to take this.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Iteration: --- → 33.2
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.
Attachment #8445554 - Flags: review?(benjamin)
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"
Attachment #8445554 - Flags: review?(benjamin)
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.
Attachment #8446083 - Flags: review?(MattN+bmo)
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
Attachment #8446084 - Flags: review?(benjamin)
Attachment #8446084 - Flags: review?(MattN+bmo)
Attachment #8445554 - Attachment is obsolete: true
Attachment #8446084 - Flags: review?(benjamin) → review+
There was a vim-induced typo in the xpcshell test causing it to fail. Re-trying.

https://tbpl.mozilla.org/?tree=Try&rev=dec1fb407752
Attachment #8446083 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8446084 - Flags: review?(MattN+bmo) → review+
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.
Hi Florin, can a contact be assigned for QA verification.
Flags: needinfo?(florin.mezei)
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
Attachment #8446084 - Flags: approval-mozilla-beta?
Attachment #8446084 - Flags: approval-mozilla-aurora?
Attachment #8446084 - Flags: approval-mozilla-beta?
Attachment #8446084 - Flags: approval-mozilla-beta+
Attachment #8446084 - Flags: approval-mozilla-aurora?
Attachment #8446084 - Flags: approval-mozilla-aurora+
Blocks: 1029267
Whoever does the uplift, please fold 11c29320d66a into b77106f50779.
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
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
QA Contact: florin.mezei
Whiteboard: [qa+] → [qa!]
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?
Flags: needinfo?(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.
Flags: needinfo?(gps)
That doesn't really define what "default" means in this context. As I understand it, "default" here is a synonym for "currently selected", right?
Flags: needinfo?(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
Flags: needinfo?(gps)
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...
(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.
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?
Flags: needinfo?(MattN+bmo)
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 .
Flags: needinfo?(MattN+bmo)
QA Whiteboard: [qa!]
Whiteboard: [qa!]
Summary: Record the default search provider in FHR → Record the currently selected search provider in FHR
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.