Implement metrics for OS search query parsing

RESOLVED FIXED in Firefox 41

Status

()

Firefox
Search
P1
normal
Rank:
12
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: clarkbw, Assigned: jaws)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed)

Details

(Whiteboard: [win10][fxsearch])

User Story

As a product manager I want to have a new metric probe for the OS search query parsing such that I can understand how often our users make use of the function and get a sense of how much Cortana is used by Firefox Windows users.

Attachments

(1 attachment, 1 obsolete attachment)

We should have some metrics to understand how useful this is in the wild.

We'll want these metrics from production.  I'll loop in the metrics team so we can get some help defining the right metrics.  I believe we just need a count of usage.
Priority: -- → P1
(Reporter)

Updated

3 years ago
Blocks: 1175223
No longer blocks: 1175592
AIUI we track some of these metrics in FHR, but I'm unsure of the details. Possibly this is just extending the existing FHR probes to cover this additional case?

Sounds like FHR is strongly preferred to get real data from release-channel users.
Ah, yes, we record something like this already with FHR:

http://hg.mozilla.org/mozilla-central/file/9340658848d1/services/healthreport/providers.jsm#l1502

Doesn't look like any of the existing recordSearch() places will get triggered by how Windows 10 searches are done, so we'd likely need to both extend FHR with an additional place, and explicitly add a measurement in the Win10 search codepath.

[This stuff seems like it could benefit from some cleanup, but with Unified Telemetry around the corner, probably not worth it. Not sure if there are any lurking complications to extending this, with it being such an important measurement.]

Updated

3 years ago
Flags: firefox-backlog+
Whiteboard: [fxsearch]

Updated

3 years ago
Rank: 3
Whiteboard: [fxsearch] → [fxsearch][win10]

Updated

3 years ago
Rank: 3 → 10

Updated

3 years ago
Rank: 10 → 12
Whiteboard: [fxsearch][win10] → [win10][fxsearch]

Comment 3

3 years ago
Hi Brian, do you want us to wait on the metrics you are asking about?  are we blocked on this waiting your comment in the User Story?
Flags: needinfo?(clarkbw)
(Reporter)

Comment 4

3 years ago
(In reply to :shell escalante from comment #3)
> Hi Brian, do you want us to wait on the metrics you are asking about?  are
> we blocked on this waiting your comment in the User Story?

I would really like these metrics to land with the search work.  That said if it wasn't possible to land this in 40 I think we'll only miss a small set of the population who have adopted Windows 10 very early on.
User Story: (updated)
Flags: needinfo?(clarkbw)

Updated

3 years ago
Blocks: 1191384
AIUI we're first fixing bug 1177443 (which adds a new "system" search purpose param), and can then add these metrics for that param.
Depends on: 1177443
Created attachment 8650070 [details] [diff] [review]
Patch
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8650070 - Flags: review?(adw)

Comment 8

3 years ago
Comment on attachment 8650070 [details] [diff] [review]
Patch

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

I'm pretty sure you need to add a new SearchCountMeasurement2 in order to add a new field.  At least that's what I had to do in a similar case in bug 1009765, and bug 1135543 did it, too.  And you also need to update the org.mozilla.searches.counts section in http://mxr.mozilla.org/mozilla-central/source/services/healthreport/docs/dataformat.rst I think.

Other than that this looks fine to be, but to be honest I'm not super comfortable reviewing changes to FHR data because it's fairly complex and important and I may be missing something myself.  gps or rnewman would probably be better?
Attachment #8650070 - Flags: review?(adw) → feedback+
Georg, I've been looking at providers.jsm and I have some questions:

- Do you know why SearchCountMeasurement1 doesn't derive from SearchCountMeasurementBase?
- SearchCountMeasurementBase defines a _fieldSpecs property that is returned from the `fields` getter, but is not populated anywhere else in this file. A DXR search for `_fieldSpecs` doesn't turn up anything either.
- How does SearchCountMeasurementBase._fieldSpecs relate to SearchCountMeasurement1.fields?
- If I want to define a new SearchCountMeasurement4 class, it looks like I should be extending SearchCountMeasurement3, but at the same time I want to append to the `fields` that SearchCountMeasurement1 has defined as well as the `SOURCES` that SearchCountMeasurementBase has defined. What is the preferred way to do this?
Flags: needinfo?(gfritzsche)
Comment on attachment 8650070 [details] [diff] [review]
Patch

Per IRC, as we are about to phase FHR out on desktop and we don't want to double-implement this, this will be submitted via Telemetry instead.
Flags: needinfo?(gfritzsche)
Attachment #8650070 - Flags: feedback+
Created attachment 8652900 [details] [diff] [review]
Patch v1.1

Switched over to using Telemetry and tested that the keyed histograms are showing up in about:telemetry.
Attachment #8650070 - Attachment is obsolete: true
Attachment #8652900 - Flags: review?(gfritzsche)
Comment on attachment 8652900 [details] [diff] [review]
Patch v1.1

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

::: browser/components/nsBrowserContentHandler.js
@@ +269,5 @@
>  
> +function logSystemBasedSearch(engine) {
> +  var countId = (engine.identifier || ("other-" + engine.name)) + ".system";
> +  var count = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS");
> +  count.add(countId);

Can we just route this through BrowserSearch.recordSearchInHealthReport() ?
https://dxr.mozilla.org/mozilla-central/rev/f61c3cc0eb8b7533818e7379ccc063b611015d9d/browser/base/content/browser.js#

That way we don't distribute the string-building logic more across the tree and get it in FHR too for free.
Attachment #8652900 - Flags: review?(gfritzsche)
Comment on attachment 8652900 [details] [diff] [review]
Patch v1.1

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

Per IRC BrowserSearch is not easily accessible at this point.

We should consolidate the search telemetry logic in one place in toolkit telemetry so we don't need a window for that, please file a follow-up.

I also see that we don't have proper documentation for the search count Telemetry yet. We should document that in the in-tree Telemetry docs. Can you please file a follow-up on this too?
Attachment #8652900 - Flags: review+
Depends on: 1198847
Depends on: 1198848
url:        https://hg.mozilla.org/integration/fx-team/rev/73713a40e0ffcf7d3b8987d60ab6e84760d263a3
changeset:  73713a40e0ffcf7d3b8987d60ab6e84760d263a3
user:       Jared Wein <jwein@mozilla.com>
date:       Wed Aug 26 13:28:13 2015 -0400
description:
Bug 1177246 - Implement metrics for OS search query parsing. r=gfritzsche
@mconnor, this bug updated the telemetry histogram that tracks searches combined with their origin within the product. However, it did not update the similar metric within FHR. Is this going to cause a problem with reports that are created? If so, is there a timeline for transitioning from the FHR data to the Telemetry data?
Flags: needinfo?(mconnor)
https://hg.mozilla.org/mozilla-central/rev/73713a40e0ff
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> @mconnor, this bug updated the telemetry histogram that tracks searches
> combined with their origin within the product. However, it did not update
> the similar metric within FHR. Is this going to cause a problem with reports
> that are created? If so, is there a timeline for transitioning from the FHR
> data to the Telemetry data?

There would be concern about FHR vs. Unified Telemetry comparison (i'll flag that) and we might see a slight bump on switch-over on the executive dashboards etc. (depending on how far you uplift).

I personally don't think it matters too much though whether we see that bump now or when we switch over to powering reports/dashboards from Unified Telemetry.
We will see some bumps around that time anyway.
Comment on attachment 8652900 [details] [diff] [review]
Patch v1.1

Approval Request Comment
[Feature/regressing bug #]: telemetry tracking for new search capability introduced in firefox 40, bug 1177237
[User impact if declined]: delayed data on search usage
[Describe test coverage new/current, TreeHerder]: manually tested
[Risks and why]: none, updates existing telemetry probe
[String/UUID change made/needed]: none
Attachment #8652900 - Flags: approval-mozilla-beta?
Attachment #8652900 - Flags: approval-mozilla-aurora?
status-firefox41: --- → affected
status-firefox42: --- → affected
Comment on attachment 8652900 [details] [diff] [review]
Patch v1.1

Accepting to both aurora & beta to improve the data used to take decisions.
Attachment #8652900 - Flags: approval-mozilla-beta?
Attachment #8652900 - Flags: approval-mozilla-beta+
Attachment #8652900 - Flags: approval-mozilla-aurora?
Attachment #8652900 - Flags: approval-mozilla-aurora+
I think this was mostly addressed in commment 18.  I know there's a lot of work to nail down inconsistencies, so I will leave concerns to those folks.  The FHR issue is known, but complicated, there is/was a bug to resolve that once the dust settles on Win10.
Flags: needinfo?(mconnor)
You need to log in before you can comment on or make changes to this bug.