Closed
Bug 1316281
Opened 8 years ago
Closed 8 years ago
Record search event in Telemetry
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
(Blocks 1 open bug)
Details
(Whiteboard: [measurement:client])
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
Dexter
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gfritzsche
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gfritzsche
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
As an initial (prototyping) event, we want to record search interactions.
We want to cross-check / match this events data with Activity Stream:
https://docs.google.com/document/d/1QhN79LUf2JgDqGxyn9GWd6CnU7vrTbJpRkBo3wqjVe8/edit#heading=h.1biemupso2tb
We should also match the requests for Javauns search metrics here:
https://docs.google.com/spreadsheets/d/1gDQbcKkMC1qj75DRsN5t5IkRs5oWMUxRYUnXflybJWw/
Assignee | ||
Updated•8 years ago
|
Points: --- → 2
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8814167 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 2•8 years ago
|
||
This adds initial search events along the established engagement code & test paths. You should be familiar enough with this for review?
Attachment #8814168 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8814169 -
Flags: review?(alessio.placitelli)
Assignee | ||
Updated•8 years ago
|
Attachment #8814168 -
Attachment is obsolete: true
Attachment #8814168 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8814169 [details] [diff] [review]
Part 2 - Record search event in Telemetry
Panos, can you take a quick look over this whether it matches your plans?
This records the search events in a format compatible with the search metrics wishlist:
https://docs.google.com/spreadsheets/d/1gDQbcKkMC1qj75DRsN5t5IkRs5oWMUxRYUnXflybJWw/
I didn't add everything yet, but it should be easily extended later.
The resulting event data looks like this:
> [
> [9019, "navigation", "search", "searchbar", "enter", {"engine": "google"}],
> [12598, "navigation", "search", "about_newtab", "enter", {"engine": "google"}],
> [18852, "navigation", "search", "searchbar", "oneoff", {"engine": "ddg"}]
> ]
Attachment #8814169 -
Flags: feedback?(past)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8814176 -
Flags: review?(alessio.placitelli)
Comment 6•8 years ago
|
||
Comment on attachment 8814167 [details] [diff] [review]
Part 1 - Improve error message for unknown events
Review of attachment 8814167 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/telemetry/TelemetryEvent.cpp
@@ +525,5 @@
> + str.Append("\", \"");
> + str.Append(aMethod);
> + str.Append("\", \"");
> + str.Append(aObject);
> + str.Append("\"]");
nit: I wish there was a shorter way to do that :( |str += aCategory + "\", \"" + aMethod + "\", \"" + aObject + "\"]";| doesn't work?
Attachment #8814167 -
Flags: review?(alessio.placitelli) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8814169 [details] [diff] [review]
Part 2 - Record search event in Telemetry
Review of attachment 8814169 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good to me, but I would feel better with :mak taking a look as well, since this is search-related stuff.
::: browser/modules/test/browser_UsageTelemetry_content.js
@@ +75,5 @@
> // Make sure SEARCH_COUNTS contains identical values.
> checkKeyedHistogram(search_hist, 'other-MozSearch.contextmenu', 1);
>
> + // Also check events.
> + let events = Services.telemetry.snapshotBuiltinEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
nit: since event filtering is done in other tests as well, what about defining an helper function in head.js that event snapshotting + filtering?
Attachment #8814169 -
Flags: review?(mak77)
Attachment #8814169 -
Flags: review?(alessio.placitelli)
Attachment #8814169 -
Flags: review+
Comment 8•8 years ago
|
||
Comment on attachment 8814176 [details] [diff] [review]
Part 3 - Fix TelemetrySession filter for test events
Review of attachment 8814176 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1019,5 @@
> clearSubsession);
>
> // Don't return the test events outside of test environments.
> if (!this._testing) {
> + events = events.filter(e => !e[1].startsWith("telemetry.test"));
Whoops, my bad for letting this slip by. Good catch.
Attachment #8814176 -
Flags: review?(alessio.placitelli) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8814169 [details] [diff] [review]
Part 2 - Record search event in Telemetry
Review of attachment 8814169 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM, my only doubt is whether we should rather make up an action for contextmenu that is the only case not having one, but likely doesn't matter.
::: browser/modules/BrowserUsageTelemetry.jsm
@@ +300,5 @@
> + let scalarKey = action ? "search_" + action : "search";
> + Services.telemetry.keyedScalarAdd("browser.engagement.navigation." + source,
> + scalarKey, 1);
> + Services.telemetry.recordEvent("navigation", "search", source, action,
> + { engine: getSearchEngineId(engine) });
What's the output format if action is null? comment 4 doesn't explain that.
::: browser/modules/test/head.js
@@ +103,5 @@
> + events = events.map(e => e.slice(1));
> +
> + for (let i = 0; i < events.length; ++i) {
> + Assert.deepEqual(events[i], expectedEvents[i], "Events should match.");
> + }
nit: I think after the slice you should be able to do Assert.deepEqual directly on the array, and also remove the length check above then.
Attachment #8814169 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #9)
> Comment on attachment 8814169 [details] [diff] [review]
> Part 2 - Record search event in Telemetry
>
> Review of attachment 8814169 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> LGTM, my only doubt is whether we should rather make up an action for
> contextmenu that is the only case not having one, but likely doesn't matter.
>
> ::: browser/modules/BrowserUsageTelemetry.jsm
> @@ +300,5 @@
> > + let scalarKey = action ? "search_" + action : "search";
> > + Services.telemetry.keyedScalarAdd("browser.engagement.navigation." + source,
> > + scalarKey, 1);
> > + Services.telemetry.recordEvent("navigation", "search", source, action,
> > + { engine: getSearchEngineId(engine) });
>
> What's the output format if action is null? comment 4 doesn't explain that.
That field is allowed to be null, so it would be:
> [9019, "navigation", "search", "searchbar", null, {"engine": "google"}]
> ::: browser/modules/test/head.js
> @@ +103,5 @@
> > + events = events.map(e => e.slice(1));
> > +
> > + for (let i = 0; i < events.length; ++i) {
> > + Assert.deepEqual(events[i], expectedEvents[i], "Events should match.");
> > + }
>
> nit: I think after the slice you should be able to do Assert.deepEqual
> directly on the array, and also remove the length check above then.
This way made it easier for me to diagnose failures, because i can directly see in the test log which events didn't match and what their data is.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #6)
> ::: toolkit/components/telemetry/TelemetryEvent.cpp
> @@ +525,5 @@
> > + str.Append("\", \"");
> > + str.Append(aMethod);
> > + str.Append("\", \"");
> > + str.Append(aObject);
> > + str.Append("\"]");
>
> nit: I wish there was a shorter way to do that :( |str += aCategory + "\",
> \"" + aMethod + "\", \"" + aObject + "\"]";| doesn't work?
This would construct a few temporary objects.
Another option would be to use one of our printf-like approaches, but that isn't nice either here.
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #9)
> Comment on attachment 8814169 [details] [diff] [review]
> Part 2 - Record search event in Telemetry
>
> Review of attachment 8814169 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> LGTM, my only doubt is whether we should rather make up an action for
> contextmenu that is the only case not having one, but likely doesn't matter.
Something like "click" or so? That would make things more uniform, right.
A concern would be that we already shipped the scalar measurements without this, so it would mean some special casing here specifically for "contextmenu".
Assignee | ||
Comment 13•8 years ago
|
||
Comment 14•8 years ago
|
||
Comment on attachment 8814169 [details] [diff] [review]
Part 2 - Record search event in Telemetry
Review of attachment 8814169 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, thanks!
::: toolkit/components/telemetry/Events.yaml
@@ +5,5 @@
> + release_channel_collection: opt-out
> + description: Record search navigation event.
> + bug_numbers: [1316281]
> + notification_emails: ["rweiss@mozilla.com"]
> + expiry_version: "58.0"
I think these metrics shouldn't expire as they are important for bizdev long-term.
Attachment #8814169 -
Flags: feedback?(past) → feedback+
Comment 15•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> > ::: toolkit/components/telemetry/TelemetryEvent.cpp
> > @@ +525,5 @@
> > > + str.Append("\", \"");
> > > + str.Append(aMethod);
> > > + str.Append("\", \"");
> > > + str.Append(aObject);
> > > + str.Append("\"]");
You may be able to use c++11 literals to avoid the escapes, at least it could be more readable.
http://clang.llvm.org/extra/clang-tidy/checks/modernize-raw-string-literal.html
Comment 16•8 years ago
|
||
and maybe along with nsPrintfCString the result could be even simpler. Something to try :)
Assignee | ||
Comment 17•8 years ago
|
||
Improved formatting code.
Assignee | ||
Updated•8 years ago
|
Attachment #8814167 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
Updated Events.yaml details for search event.
Assignee | ||
Updated•8 years ago
|
Attachment #8814169 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8814848 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8814854 -
Flags: review+
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Panos Astithas [:past] from comment #14)
> ::: toolkit/components/telemetry/Events.yaml
> @@ +5,5 @@
> > + release_channel_collection: opt-out
> > + description: Record search navigation event.
> > + bug_numbers: [1316281]
> > + notification_emails: ["rweiss@mozilla.com"]
> > + expiry_version: "58.0"
>
> I think these metrics shouldn't expire as they are important for bizdev
> long-term.
I am starting with this as a prototype event and we are still figuring out how exactly we will handle the event volume etc.
I want to use this as a real and useful example event to go on the trains so we have actual data coming in while we figure out the other details.
Once that is resolved, we should revisit this (and other) events and decide about expiry and opt-in/opt-out etc.
Ideally this would fit in well with the search metrics plans and just continue to be used (or extended etc.).
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8814854 [details] [diff] [review]
Part 2 - Record search event in Telemetry
I think Rebecca is still out; Benjamin can you do the data review?
Comment 19 has the motivation for this probe: it is currently a prototype event that we want to ride the trains to validate implementation and expected behavior on the pre-release trains (targetting 52).
Bug 1319102 will limit all the event collection to pre-release channels for now.
Bug 1302666 is updating the documentation with event telemetry details.
The design for events is here:
https://docs.google.com/document/d/1hNuS9lUJMvMqgntZXbFA6xZBU9zBpQgo7x73-sXKRpI/
Attachment #8814854 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8814854 [details] [diff] [review]
Part 2 - Record search event in Telemetry
Rebecca is actually back, redirecting data review.
Attachment #8814854 -
Flags: feedback?(benjamin) → feedback?(rweiss)
Comment 22•8 years ago
|
||
Who is doing the preliminary data analysis on the outcome of this prototype event? I will take the liberty of asserting that it is either :gfritzsche or :dzeber (or, in line with the some of the other engagement measures, by :Dexter).
What will the preliminary data analysis of this prototype event look like? I will also take the liberty of asserting that the analysis will consist of either a Spark notebook that demonstrates that data is coming in and can be evaluated by inspecting the distribution of events. This can then be shown to the sponsors for this new measurement (:past and :javaun) as well as :dzeber as data scientist for a face value validity check, as well as to :gfritzsche and company for instrumentation validity check.
I see that we have documentation for the event design, as well as stated justification for the event measurement, so I think we are covered for data review assuming the above assertions are true. Please comment with a yes or no to both. Consider this a provisional sign off if the answer is "yes" to both.
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Rebecca Weiss from comment #22)
> Who is doing the preliminary data analysis on the outcome of this prototype
> event? I will take the liberty of asserting that it is either :gfritzsche
> or :dzeber (or, in line with the some of the other engagement measures, by
> :Dexter).
Yes, i will do the preliminary analysis.
> What will the preliminary data analysis of this prototype event look like?
> I will also take the liberty of asserting that the analysis will consist of
> either a Spark notebook that demonstrates that data is coming in and can be
> evaluated by inspecting the distribution of events. This can then be shown
> to the sponsors for this new measurement (:past and :javaun) as well as
> :dzeber as data scientist for a face value validity check, as well as to
> :gfritzsche and company for instrumentation validity check.
Yes, preliminary analysis will be through a Spark notebook.
We will start to line up conversations for better search instrumentation starting next week.
We will make decisions about how to go forward with this based on search metrics needs and the data processing plans that informed by this early collection.
Comment 24•8 years ago
|
||
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/823e118b3fad
Part 1 - Improve error message for unknown events. r=dexter
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ea1334c33a
Part 2 - Record search event in Telemetry. r=dexter,mak data-review=rweiss
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c711bb8e373
Part 3 - Fix TelemetrySession filter for test events. r=dexter
Comment 25•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dd01d903f823 for ASan test failures, https://treeherder.mozilla.org/logviewer.html#?job_id=40181720&repo=mozilla-inbound
Assignee | ||
Comment 26•8 years ago
|
||
I missed that we don't collect extended Telemetry in every test configuration.
With the search event being opt-in for now, we need to adjust the tests accordingly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8531346597e888acebee101e3a66307819169961
Comment 27•8 years ago
|
||
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bed0ae598c3f
Part 1 - Improve error message for unknown events. r=dexter
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a33a480ca2e
Part 2 - Record search event in Telemetry. r=dexter,mak data-review=rweiss
https://hg.mozilla.org/integration/mozilla-inbound/rev/81738db2f99b
Part 3 - Fix TelemetrySession filter for test events. r=dexter
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bed0ae598c3f
https://hg.mozilla.org/mozilla-central/rev/1a33a480ca2e
https://hg.mozilla.org/mozilla-central/rev/81738db2f99b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 29•8 years ago
|
||
For uplifting this, we will also need to uplift bug 1318284 and bug 1323628.
Assignee | ||
Updated•8 years ago
|
Whiteboard: [measurement:client] → [measurement:client] [measurement:client:uplift]
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8814848 [details] [diff] [review]
Part 1 - Improve error message for unknown events
Approval Request Comment
[Feature/Bug causing the regression]: Event Telemetry.
[User impact if declined]: Data of Event Telemetry behavior reaches us later, delaying our decision making for the project.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes, bug 1302671.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]:
The individual parts are:
* bug 1302663 - basic Telemetry implementation
* bug 1316810 - adjust event limits
* bug 1318284 - improve metrics that track effects on ping sending
* bug 1323628 - fix
* bug 1316281 - record search event in Telemetry
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's validated on Nightly and the changes are selective and relatively well understood.
[String changes made/needed]: No.
This is part of an uplift need for the initial Event Telemetry [1][2] implementation to Firefox 52.
We want to move this to Beta a little faster to see somewhat realistic data of an example event (search) coming in and verify that it matches our expectations.
I validated that this behaves as expected on Nightly over the last two weeks in bug 1302671.
1: https://docs.google.com/document/d/1hNuS9lUJMvMqgntZXbFA6xZBU9zBpQgo7x73-sXKRpI/
2: https://wiki.mozilla.org/Event_Telemetry
Attachment #8814848 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8814854 [details] [diff] [review]
Part 2 - Record search event in Telemetry
Approval Request Comment
... see comment 30.
Attachment #8814854 -
Flags: feedback?(rweiss) → approval-mozilla-aurora?
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8814176 [details] [diff] [review]
Part 3 - Fix TelemetrySession filter for test events
Approval Request Comment
... see comment 30.
Attachment #8814176 -
Flags: approval-mozilla-aurora?
Comment 33•8 years ago
|
||
Comment on attachment 8814848 [details] [diff] [review]
Part 1 - Improve error message for unknown events
add event telemetry for aurora52
Attachment #8814848 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 34•8 years ago
|
||
Comment on attachment 8814854 [details] [diff] [review]
Part 2 - Record search event in Telemetry
add event telemetry for aurora52
Attachment #8814854 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 35•8 years ago
|
||
Comment on attachment 8814176 [details] [diff] [review]
Part 3 - Fix TelemetrySession filter for test events
add event telemetry for aurora52
Attachment #8814176 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 36•8 years ago
|
||
bugherder uplift |
Assignee | ||
Updated•8 years ago
|
Whiteboard: [measurement:client] [measurement:client:uplift] → [measurement:client]
Comment 37•6 years ago
|
||
This is set to expire:
https://bugzilla.mozilla.org/show_bug.cgi?id=1496764
was there a reason this was set to expire in 65? This seems like something we need in perpetuity.
QA Contact: gfritzsche
Updated•6 years ago
|
QA Contact: gfritzsche
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Mike Kaply [:mkaply] (on PTO until Nov 5) from comment #37)
> This is set to expire:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1496764
>
> was there a reason this was set to expire in 65? This seems like something
> we need in perpetuity.
There is no specific reason except that it was not clear if the event was needed long-term at the time.
You need to log in
before you can comment on or make changes to this bug.
Description
•