Add keyword specific telemetry

RESOLVED FIXED in Firefox 64

Status

()

defect
P1
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: mkaply, Assigned: adw)

Tracking

(Blocks 1 bug)

Trunk
Firefox 65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox64 fixed, firefox65 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

7 months ago
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.
Reporter

Updated

7 months ago
Priority: -- → P1
Assignee

Updated

7 months ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee

Comment 1

7 months ago
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.
Assignee

Comment 2

7 months ago
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 `"engineName.@engine.source"` key under the `SEARCH_COUNTS` histogram -- in addition to incrementing the usual `"engineName.source"` key under that same histogram.

Comment 4

7 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9412ce1e2e93
Add new SEARCH_COUNTS telemetry for internal search engine alias usage. r=mkaply

Comment 5

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9412ce1e2e93
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee

Updated

7 months ago
Flags: qe-verify-
Flags: in-testsuite+
Assignee

Comment 6

7 months ago
Posted patch Beta/64 patch (obsolete) — Splinter Review
[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
Attachment #9022664 - Flags: approval-mozilla-beta?
Since this seems to be extending the collected key space, would you mind filing the data review form for this change?
Flags: needinfo?(adw)
Assignee

Comment 8

6 months ago
Requesting data review
Flags: needinfo?(adw)
Attachment #9023496 - Flags: review?(chutten)

Comment 9

6 months ago
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 ))
Attachment #9023496 - Flags: review?(chutten)
Assignee

Comment 11

6 months ago
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/
Attachment #9023496 - Attachment is obsolete: true
Attachment #9023781 - Flags: review?(chutten)

Comment 12

6 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1ff54300cc9
Follow-up: Update metadata for the SEARCH_COUNTS keyed histogram. r=mkaply
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
Flags: needinfo?(adw)
Assignee

Updated

6 months ago
Attachment #9022664 - Attachment is obsolete: true
Flags: needinfo?(adw)
Attachment #9022664 - Flags: approval-mozilla-beta?

Comment 14

6 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/343511c3bccd
Follow-up: Update metadata for the SEARCH_COUNTS keyed histogram. r=mkaply
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+
Attachment #9023781 - Flags: review?(chutten) → review+
Assignee

Comment 17

6 months ago
[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
Attachment #9024081 - Flags: approval-mozilla-beta?
Comment on attachment 9024081 [details] [diff] [review]
Combinded Beta/64 patch

telemetry for search shortcuts, data-review=chutten, approved for 64.0b9
Attachment #9024081 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Comment 21

6 months ago
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.
Flags: needinfo?(adw) → needinfo?(rmaries)
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 `bing.@bing.urlbar` 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
Flags: needinfo?(adw)
Assignee

Comment 24

6 months ago
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., bing.@bing.urlbar 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.
Flags: needinfo?(adw) → needinfo?(rharter)
Assignee

Updated

6 months ago
Flags: needinfo?(rmaries)
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?
Flags: needinfo?(rharter) → needinfo?(adw)
Assignee

Comment 26

6 months ago
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.
Flags: needinfo?(adw) → needinfo?(mozilla)
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.
Flags: needinfo?(mozilla)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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".
Flags: needinfo?(mconnor)
Assignee

Comment 29

6 months ago
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.
Assignee

Comment 30

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

Comment 31

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

Comment 33

6 months ago
No problem, we can fix it when we land the new patch with the histogram key change.  My fault for needing a backout anyway.
Yes, bing.alias is fine.
Assignee

Updated

6 months ago
Flags: needinfo?(mconnor)

Comment 35

6 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b006cee2f7bd
Follow-up 2: Update SEARCH_COUNTS key r=mkaply
Assignee

Comment 36

6 months ago
Posted patch Combined Beta/64 patch v2 (obsolete) — Splinter Review
[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
Attachment #9024081 - Attachment is obsolete: true
Attachment #9025819 - Flags: approval-mozilla-beta?

Comment 37

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b006cee2f7bd
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago6 months ago
Resolution: --- → FIXED
Comment on attachment 9025819 [details] [diff] [review]
Combined Beta/64 patch v2

afaict beta just needs the followup
Attachment #9025819 - Flags: approval-mozilla-beta?
Comment on attachment 9025408 [details]
Bug 1499193 - Follow-up 2: Update SEARCH_COUNTS key

followup fix for search telemetry, approved for 64.0b11
Attachment #9025408 - Flags: approval-mozilla-beta+
Assignee

Updated

6 months ago
Attachment #9024081 - Attachment is obsolete: false
Assignee

Updated

6 months ago
Attachment #9025819 - Attachment is obsolete: true
Depends on: 1514021
You need to log in before you can comment on or make changes to this bug.