1499193 - Add keyword specific telemetry

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Mike Kaply [:mkaply]]

Currently when we use our new search keywords (@google for instance), we track it as a normal URL bar search in telemetry:

google-2018.urlbar
1 samples, average = 1, sum = 1

0 |#########################  1  100%
1 |  0  0%

google.in-content:sap:firefox-b-1-ab
1 samples, average = 1, sum = 1

0 |#########################  1  100%
1 |  0  0%

We should track the search keyword as a separate thing from a urlbar search.

Comment 1

[Drew Willcoxon :adw]

We currently have keyed scalar telemetry that records the total number of times you use search aliases in the urlbar.  (It expires never.)  It looks like this (copying and pasting from about:telemetry):

browser.engagement.navigation.urlbar
search_alias: 3

The Scalars.yaml definition is here: https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/components/telemetry/Scalars.yaml#180

It doesn't distinguish between the new @ aliases and non-@ aliases.  But if the @ aliases are getting traction with users, it should be reflected in this existing telemetry.

We also have telemetry events for search, which also records alias usage, but that expires in 65: https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/components/telemetry/Events.yaml#148

But it looks like what you're asking for, Mike, is using new keys for @ aliases for the SEARCH_COUNTS keyed histogram?

Firefox telemetry is so confusing.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1499193 - Add new SEARCH_COUNTS telemetry for internal search engine alias usage.

Modify `BrowserUsageTelemetry.recordSearch` to take an alias instead of the bool `isAlias`. If an alias is given and it's an internal alias of the given engine, then increment a new `"[e-mail]"` key under the `SEARCH_COUNTS` histogram -- in addition to incrementing the usual `"engineName.source"` key under that same histogram.

Comment 3

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8028acd87355f8a2c04c20c82666b4e3dc4702dd

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9412ce1e2e93
Add new SEARCH_COUNTS telemetry for internal search engine alias usage. r=mkaply

Comment 5

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/9412ce1e2e93

Comment 6

[Drew Willcoxon :adw]

Attachment: Beta/64 patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Not a regression but improvement to search shortcuts in the awesomebar: see bug 1496772

User impact if declined: We've landed a group of improvements to search shortcuts in 64 (see bug 1496772) and want this bug to make it too

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Only adds telemetry, has automated tests, and it's relatively early in the cycle

String changes made/needed: None

Comment 7

[Julien Cristau [:jcristau]]

Since this seems to be extending the collected key space, would you mind filing the data review form for this change?

Comment 8

[Drew Willcoxon :adw]

Attachment: Request for Data Collection Review.txt

Requesting data review

Comment 9

[Chris H-C :chutten]

Comment on attachment 9023496 [details]
Request for Data Collection Review.txt

Preliminary notes:

We need an individual's name in the answer to question 4 for permanent collection. That should ideally be the person who owns the email address in the alert_emails field of the measurement in question (which is currently missing). You should also add this bug's number to the bug_numbers field of the measurement (which is currently missing).

(( The review request is otherwise fine. Fix these three things and re-r? me and we'll be good to go ))

Comment 10

[Drew Willcoxon :adw]

Attachment: Bug 1499193 - Follow-up: Update metadata for the SEARCH_COUNTS keyed histogram.

Comment 11

[Drew Willcoxon :adw]

Attachment: Request for Data Collection Review.txt v2

Thanks, updated.  I just queued on autoland a follow-up patch that adds the missing bug_numbers and alert_emails: https://lando.services.mozilla.com/D11382/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1ff54300cc9
Follow-up: Update metadata for the SEARCH_COUNTS keyed histogram. r=mkaply

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out for bustages on TelemetryHistogram

backout: https://hg.mozilla.org/integration/autoland/rev/643b9ec9aa1fb4b5a6f628bf1f8c1b3a1aa02628

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d1ff54300cc94d518a25bfde18105df706962d75

failure log: 23:10:23     INFO -  error parsing histograms in z:/build/build/src/toolkit/components/telemetry/Histograms.json: Expecting , delimiter: line 8002 column 5 (char 317711)
23:10:23     INFO -  backend.mk:51: recipe for target '.deps/TelemetryHistogramData.inc.stub' failed
23:10:23     INFO -  mozmake.EXE[4]: *** [.deps/TelemetryHistogramData.inc.stub] Error 1
23:10:23     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/telemetry'
23:10:23     INFO -  mozmake.EXE[4]: *** Waiting for unfinished jobs....
23:10:23     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/telemetry'
23:10:23     INFO -  toolkit/components/telemetry/TelemetryHistogramEnums.h.stub
23:10:23     INFO -  z:/build/build/src/obj-firefox/_virtualenvs/init/Scripts/python.exe -m mozbuild.action.file_generate z:/build/build/src/toolkit/components/telemetry/build_scripts/gen_histogram_enum.py main TelemetryHistogramEnums.h .deps/TelemetryHistogramEnums.h.pp .deps/TelemetryHistogramEnums.h.stub z:/build/build/src/toolkit/components/telemetry/Histograms.json z:/build/build/src/dom/base/UseCounters.conf z:/build/build/src/dom/base/nsDeprecatedOperationList.h
23:10:23     INFO -  error parsing histograms in z:/build/build/src/toolkit/components/telemetry/Histograms.json: Expecting , delimiter: line 8002 column 5 (char 317711)
23:10:23     INFO -  backend.mk:61: recipe for target '.deps/TelemetryHistogramEnums.h.stub' failed
23:10:23     INFO -  mozmake.EXE[4]: *** [.deps/TelemetryHistogramEnums.h.stub] Error 1
23:10:23     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/telemetry'
23:10:23     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/telemetry'
23:10:23     INFO -  toolkit/components/telemetry/TelemetryProcessData.h.stub
23:10:23     INFO -  z:/build/build/src/obj-firefox/_virtualenvs/init/Scripts/python.exe -m mozbuild.action.file_generate z:/build/build/src/toolkit/components/telemetry/build_scripts/gen_process_data.py main TelemetryProcessData.h .deps/TelemetryProcessData.h.pp .deps/TelemetryProcessData.h.stub z:/build/build/src/toolkit/components/telemetry/Processes.yaml
23:10:23     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/components/telemetry'
23:10:23     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/toolkit/components/telemetry'
23:10:23     INFO -  toolkit/components/telemetry/TelemetryHistogramNameMap.h.stub
23:10:23     INFO -  z:/build/build/src/obj-firefox/_virtualenvs/init/Scripts/python.exe -m mozbuild.action.file_generate z:/build/build/src/toolkit/components/telemetry/build_scripts/gen_histogram_phf.py main TelemetryHistogramNameMap.h .deps/TelemetryHistogramNameMap.h.pp .deps/TelemetryHistogramNameMap.h.stub z:/build/build/src/toolkit/components/telemetry/Histograms.json z:/build/build/src/dom/base/UseCounters.conf z:/build/build/src/dom/base/nsDeprecatedOperationList.h
23:10:23     INFO -  error parsing histograms in z:/build/build/src/toolkit/components/telemetry/Histograms.json: Expecting , delimiter: line 8002 column 5 (char 317711)
23:10:23     INFO -  backend.mk:71: recipe for target '.deps/TelemetryHistogramNameMap.h.stub' failed
23:10:23     INFO -  mozmake.EXE[4]: *** [.deps/TelemetryHistogramNameMap.h.stub] Error 1

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/343511c3bccd
Follow-up: Update metadata for the SEARCH_COUNTS keyed histogram. r=mkaply

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/343511c3bccd

Comment 16

[Chris H-C :chutten]

Comment on attachment 9023781 [details]
Request for Data Collection Review.txt v2

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. Standard Telemetry mechanisms apply.

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. Standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, :adw will monitor it over time.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction. (As the specificity of the search providers gets higher, this edges closer to Category 3.)

    Is the data collection request for default-on or default-off?

Default on, all channels.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

Yes. It uses an identifier of the form searchProvider.searchShortcut.searchAccessPoint. Since all three of those are controlled by us, this appears to not be too worrisome.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No, this is permanent collection.

---
Result: datareview+

Comment 17

[Drew Willcoxon :adw]

Attachment: Combinded Beta/64 patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Not a regression but improvement to search shortcuts in the awesomebar: see bug 1496772

User impact if declined: We've landed a group of improvements to search shortcuts in 64 (see bug 1496772) and want this bug to make it too

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Only adds telemetry, has automated tests, and it's relatively early in the cycle

String changes made/needed: None

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9024081 [details] [diff] [review]
Combinded Beta/64 patch

telemetry for search shortcuts, data-review=chutten, approved for 64.0b9

Comment 19

[Razvan Maries]

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

Comment 20

[Razvan Maries]

Backed out from beta for failing bc at browser/modules/test/browser/browser_UsageTelemetry_urlbar.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=211229706&revision=a4175191b29b274d59ed117567ec03df9d6b8cf5

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211229706&repo=mozilla-beta&lineNumber=7810

Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/6226ce7f4038d566fe750b1d13f107146d137463

Comment 21

[Drew Willcoxon :adw]

The failure is in test code that's part of the other bug that landed with this one, bug 1504370.  Could you please land this one again?  I'll work on fixing bug 1504370 over there, separately.

Comment 22

[Cristian Brindusan [:cbrindusan]]

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

Comment 23

[Ryan Harter [:harter]]

Hey all, I think this is going to cause some issues on the data analysis side. IIUC, it looks like this is going to create new keys of the form `[e-mail]` instead of `bing.urlbar `. This will be interpreted as {engine: bing, source: "@bing.urlbar"} by our ETL [0]. We use the `source` field to differentiate between SAP and non-SAP search, so these keys are likely going to be assigned to the "unknown" category.

I'd like to avoid further complicating the search ETL pipeline. Can we change this collection to create keys like (e.g.): `bing.keyword` or `bing.keyword.urlbar` with preference towards the former?

[0] https://github.com/mozilla/telemetry-batch-view/blob/master/src/main/scala/com/mozilla/telemetry/utils/MainPing.scala#L163

Comment 24

[Drew Willcoxon :adw]

Ryan, a couple of questions:

Do you need us to drop the "@" from the keyword?  (These keywords include @, but of course we can drop it from the telemetry keys.)

bing.keyword.urlbar is what we're currently using -- e.g., [e-mail] like you mention, where the keyword is @bing.  Did you really mean to say that bing.keyword.urlbar is acceptable, or is this related to my first question?

Also, it's possible that we'll end up supporting keywords for other sources, not only the urlbar.  To be clear, I haven't heard anyone talk about that, but it's technically possible.  I only mention it in case it's important on your side, but maybe we just cross that bridge when/if we come to it.

Comment 25

[Ryan Harter [:harter]]

The `@` doesn't matter to me. My suggestion is that the `@bing` be replaced with `keyword`. This way we have one `source` for all keyword searches instead of a separate `source` for each engine. 

To make sure I understand, this change affects the SEARCH_COUNTS histogram, correct?

Comment 26

[Drew Willcoxon :adw]

Oh, the literal "keyword".  That should probably be OK.  Some engines have multiple keywords, but I'm not sure it's important to us to distinguish between them.  Mike, what do you think?

Yes, this affects SEARCH_COUNTS.

Comment 27

[Mike Connor [:mconnor]]

I'm not at all worried about which keyword was used, just that it was the method.

Only change I'd ask for is let's call it alias, mostly because "keyword" would create ambiguity.  It's already used for the "purpose" for the urlbar, and the underlying code calls these aliases.  Otherwise this meets anticipated product needs.

I'm the original requester for this data, so I'm going to call this answered and clear the NI.

Comment 28

[Ryan Harter [:harter]]

Sounds good. So the resulting key will look like: "bing.alias.urlbar"? Is the ".urlbar" suffix useful? As I mentioned in #23, it would be slightly better on the ETL side if the new keys were like: "bing.alias".

Comment 29

[Drew Willcoxon :adw]

I can't answer for Mike but I would think that bing.alias would be fine.  I'll work on a patch.  If we end up supporting aliases in other places besides the urlbar, we can use bing.alias-newtab etc. for example.

Comment 30

[Drew Willcoxon :adw]

Attachment: Bug 1499193 - Follow-up 2: Update SEARCH_COUNTS key

Change the `<engine>.<alias>.urlbar` `SEARCH_COUNTS` keys to `<engine>.alias` as described in bug 1499193 comment 23 and later.

Comment 31

[Drew Willcoxon :adw]

And it looks like the wrong patch ended up landing on beta after the original was backed out.

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry about this.

The commit from comment 22 got backed out: https://hg.mozilla.org/releases/mozilla-beta/rev/f4a229e163c6ab284c471afda36f8e381c79270c
And the one from comment 19 relanded: https://hg.mozilla.org/releases/mozilla-beta/rev/d3724480adee4f9b6a1a0d1bcbe3c4608fa57dbe

Comment 33

[Drew Willcoxon :adw]

No problem, we can fix it when we land the new patch with the histogram key change.  My fault for needing a backout anyway.

Comment 34

[Mike Connor [:mconnor]]

Yes, bing.alias is fine.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b006cee2f7bd
Follow-up 2: Update SEARCH_COUNTS key r=mkaply

Comment 36

[Drew Willcoxon :adw]

Attachment: Combined Beta/64 patch v2

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Not a regression but improvement to search shortcuts in the awesomebar: see bug 1496772

User impact if declined: We've landed a group of improvements to search shortcuts in 64 (see bug 1496772) and want this bug to make it too

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Only adds telemetry, has automated tests

String changes made/needed: None

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b006cee2f7bd

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 9025819 [details] [diff] [review]
Combined Beta/64 patch v2

afaict beta just needs the followup

Comment 39

[Julien Cristau [:jcristau]]

Comment on attachment 9025408 [details]
Bug 1499193 - Follow-up 2: Update SEARCH_COUNTS key

followup fix for search telemetry, approved for 64.0b11

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32fc87f1af7c