If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Record the currently selected search provider in FHR

VERIFIED FIXED in Firefox 31

Status

Firefox Health Report
Client: Desktop
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Benjamin Smedberg, Assigned: gps)

Tracking

unspecified
Firefox 33
Points:
5
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.

Updated

3 years ago
Points: --- → 5
Flags: firefox-backlog+
Whiteboard: [p=5][qa+] → [qa+]
(Reporter)

Comment 1

3 years ago
It is our intention to get this fixed ASAP and uplift into beta on Thursday if possible to support the search experiment.
tracking-firefox31: --- → +
tracking-firefox32: --- → +
tracking-firefox33: --- → +
(Assignee)

Comment 2

3 years ago
Benjamin told me to take this.
Assignee: nobody → gps
Status: NEW → ASSIGNED

Updated

3 years ago
Iteration: --- → 33.2
(Assignee)

Comment 3

3 years ago
Created attachment 8445554 [details] [diff] [review]
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.
Attachment #8445554 - Flags: review?(benjamin)
(Reporter)

Comment 4

3 years ago
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)
(Assignee)

Comment 5

3 years ago
Created attachment 8446083 [details] [diff] [review]
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.
Attachment #8446083 - Flags: review?(MattN+bmo)
(Assignee)

Comment 6

3 years ago
Created attachment 8446084 [details] [diff] [review]
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
Attachment #8446084 - Flags: review?(benjamin)
Attachment #8446084 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8445554 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8446084 - Flags: review?(benjamin) → review+
(Assignee)

Comment 7

3 years ago
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+
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/ab681f144bea
https://hg.mozilla.org/integration/fx-team/rev/b77106f50779
https://hg.mozilla.org/mozilla-central/rev/ab681f144bea
https://hg.mozilla.org/mozilla-central/rev/b77106f50779
https://hg.mozilla.org/mozilla-central/rev/11c29320d66a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox33: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Hi Florin, can a contact be assigned for QA verification.
Flags: needinfo?(florin.mezei)
(Reporter)

Comment 13

3 years ago
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?
status-firefox31: --- → affected
status-firefox32: --- → affected
Attachment #8446084 - Flags: approval-mozilla-beta?
Attachment #8446084 - Flags: approval-mozilla-beta+
Attachment #8446084 - Flags: approval-mozilla-aurora?
Attachment #8446084 - Flags: approval-mozilla-aurora+
(Reporter)

Updated

3 years ago
Blocks: 1029267
(Assignee)

Comment 14

3 years ago
Whoever does the uplift, please fold 11c29320d66a into b77106f50779.
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
status-firefox31: affected → fixed
status-firefox32: affected → fixed
(Assignee)

Comment 16

3 years ago
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
status-firefox31: fixed → verified
status-firefox32: fixed → verified
status-firefox33: fixed → 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)
(Assignee)

Comment 19

3 years ago
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)
(Reporter)

Comment 20

3 years ago
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)
(Assignee)

Comment 21

3 years ago
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...

Comment 23

3 years ago
(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.
(Assignee)

Comment 24

3 years ago
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)

Updated

3 years ago
QA Whiteboard: [qa!]
Whiteboard: [qa!]
Summary: Record the default search provider in FHR → Record the currently selected search provider in FHR
You need to log in before you can comment on or make changes to this bug.