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/

This adds initial search events along the established engagement code & test paths. You should be familiar enough with this for review?

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"}] > ]

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?

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?

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.

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.

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

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

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

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.

(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

and maybe along with nsPrintfCString the result could be even simpler. Something to try :)

Improved formatting code.

Updated Events.yaml details for search event.

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

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/

Comment on attachment 8814854 [details] [diff] [review] Part 2 - Record search event in Telemetry Rebecca is actually back, redirecting data review.

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.

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

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

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

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

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

https://hg.mozilla.org/mozilla-central/rev/bed0ae598c3f https://hg.mozilla.org/mozilla-central/rev/1a33a480ca2e https://hg.mozilla.org/mozilla-central/rev/81738db2f99b

For uplifting this, we will also need to uplift bug 1318284 and bug 1323628.

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

Comment on attachment 8814854 [details] [diff] [review] Part 2 - Record search event in Telemetry Approval Request Comment ... see comment 30.

Comment on attachment 8814176 [details] [diff] [review] Part 3 - Fix TelemetrySession filter for test events Approval Request Comment ... see comment 30.

Comment on attachment 8814848 [details] [diff] [review] Part 1 - Improve error message for unknown events add event telemetry for aurora52

Comment on attachment 8814854 [details] [diff] [review] Part 2 - Record search event in Telemetry add event telemetry for aurora52

Comment on attachment 8814176 [details] [diff] [review] Part 3 - Fix TelemetrySession filter for test events add event telemetry for aurora52

https://hg.mozilla.org/releases/mozilla-aurora/rev/f817cd7da1c1 https://hg.mozilla.org/releases/mozilla-aurora/rev/52653af186a7 https://hg.mozilla.org/releases/mozilla-aurora/rev/72904e12c789

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.

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