1177246 - Implement metrics for OS search query parsing

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 1

[Justin Dolske [:Dolske]]

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.

Comment 2

[Justin Dolske [:Dolske]]

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.]

Comment 3

[:shell escalante]

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?

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

(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.

Comment 5

[Justin Dolske [:Dolske]]

AIUI we're first fixing bug 1177443 (which adds a new "system" search purpose param), and can then add these metrics for that param.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a77755d622f

Comment 8

[Drew Willcoxon :adw]

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?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 10

[Georg Fritzsche [: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.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Switched over to using Telemetry and tested that the keyed histograms are showing up in about:telemetry.

Comment 12

[Georg Fritzsche [: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.

Comment 13

[Georg Fritzsche [: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?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

url:        https://hg.mozilla.org/integration/fx-team/rev/73713a40e0ffcf7d3b8987d60ab6e84760d263a3
changeset:  73713a40e0ffcf7d3b8987d60ab6e84760d263a3
user:       Jared Wein <[e-mail]>
date:       Wed Aug 26 13:28:13 2015 -0400
description:
Bug 1177246 - Implement metrics for OS search query parsing. r=gfritzsche

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

@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?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73713a40e0ff

Comment 17

[Georg Fritzsche [:gfritzsche]]

(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 18

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652900 [details] [diff] [review]
Patch v1.1

Accepting to both aurora & beta to improve the data used to take decisions.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/56bdc151e79f

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Mike Connor [:mconnor]]

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.