Closed Bug 1499193 Opened 6 years ago Closed 6 years ago

Add keyword specific telemetry

Categories

(Firefox :: Search, defect, P1)

Product:
Firefox
Component:
Search
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 65
Tracking Flags:
Tracking Status
firefox64 --- fixed
firefox65 --- fixed

People

(Reporter: mkaply, Assigned: adw)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

Bug 1499193 - Add new SEARCH_COUNTS telemetry for internal search engine alias usage.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1499193 - Follow-up: Update metadata for the SEARCH_COUNTS keyed histogram.
47 bytes, text/x-phabricator-request
Details | Review
Request for Data Collection Review.txt v2
2.88 KB, text/plain
chutten
: review+
Details
Combinded Beta/64 patch
15.29 KB, patch
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
Bug 1499193 - Follow-up 2: Update SEARCH_COUNTS key
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review 
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.
Priority: -- → P1
Assignee: nobody → adw
Status: NEW → ASSIGNED
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.
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.
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
https://hg.mozilla.org/mozilla-central/rev/9412ce1e2e93
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: qe-verify-
Flags: in-testsuite+
Attached 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)
Attached file Request for Data Collection Review.txt (obsolete) — 
Requesting data review
Flags: needinfo?(adw)
Attachment #9023496 - Flags: review?(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 ))
Attachment #9023496 - Flags: review?(chutten)
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)
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)
Attachment #9022664 - Attachment is obsolete: true
Flags: needinfo?(adw)
Attachment #9022664 - Flags: approval-mozilla-beta?
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+
[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+
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)
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)
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)
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)
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.
Change the `<engine>.<alias>.urlbar` `SEARCH_COUNTS` keys to `<engine>.alias` as described in bug 1499193 comment 23 and later.
And it looks like the wrong patch ended up landing on beta after the original was backed out.
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.
Flags: needinfo?(mconnor)
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b006cee2f7bd
Follow-up 2: Update SEARCH_COUNTS key r=mkaply
Attached 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?
https://hg.mozilla.org/mozilla-central/rev/b006cee2f7bd
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox65: affected → fixed
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+
Attachment #9024081 - Attachment is obsolete: false
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.

Attachment

General

Creator:
Created:
Updated:
Size: