We already record the action in BrowserUsageTelemetry::_recordSearch event and UITelemetry, but we also want to count how many times each position index was selected. We want the same count for the search bar, too.

This would probably be implemented as an enumerated histogram.

Comment on attachment 8848693 [details] Bug 1334633 - Record the engine position in the list when a one-off button is clicked. f?gfritzsche Georg, do you mind giving me feedback on this to make sure I'm using telemetry right? * This adds a new enumerated histogram to Histograms.json called FX_SEARCH_ONE_OFF_INDEX. * Its n_values is 32, which should be more than enough to capture even extreme cases where people have lots of engines. Or is there any penalty for choosing such a large n_values? * It modifies BrowserUsageTelemetry._recordSearch to get the histogram and add the engine index. Note that _recordSearch already records two other types of telemetry: a keyed scalar and an event. * BrowserSearch.recordOneoffSearchInTelemetry is what calls BrowserUsageTelemetry.recordSearch, but that method also calls BrowserUITelemetry.countOneoffSearchEvent. This patch doesn't include the engine index in the browser UI telemetry because my understanding is that browser UI telemetry is basically deprecated, is that right? Finally, an unrelated thing I noticed while working on this is that BrowserUsageTelemetry doesn't handle two one-off sources generated by search.xml: "oneoff-context-searchbar" and "oneoff-context-urlbar". Those sources are generated when you search via the one-off context menu. I'm not sure whether you're the right person to ask, but that seems like an oversight. Do you know anything about that? Thanks!

Oh also: * It sets the histogram to expire in version 58. I'm not sure what we want for that.

(In reply to Drew Willcoxon :adw from comment #4) > Oh also: > > * It sets the histogram to expire in version 58. I'm not sure what we want > for that. The plan for these search probes is to start using them as standard instrumentation for evaluating search, so if possible I'd prefer them not to expire (expires in version "never"). If that's not feasible, maybe we can set it 10 versions out.

Sorry for the very long turn-around, this request slipped through between too many tasks and PTO. (In reply to Drew Willcoxon :adw from comment #3) > * This adds a new enumerated histogram to Histograms.json called > FX_SEARCH_ONE_OFF_INDEX. > > * Its n_values is 32, which should be more than enough to capture even > extreme cases where people have lots of engines. Or is there any penalty > for choosing such a large n_values? We serialize & submit histograms sparsely, so this is fine. We only worry a bit for value/bucket counts >100 for analysis impact. > * It modifies BrowserUsageTelemetry._recordSearch to get the histogram and > add the engine index. Note that _recordSearch already records two other > types of telemetry: a keyed scalar and an event. > > * BrowserSearch.recordOneoffSearchInTelemetry is what calls > BrowserUsageTelemetry.recordSearch, but that method also calls > BrowserUITelemetry.countOneoffSearchEvent. This patch doesn't include the > engine index in the browser UI telemetry because my understanding is that > browser UI telemetry is basically deprecated, is that right? Right, we should only add to it for exceptional cases where dzeber et al would still need to use it. > Finally, an unrelated thing I noticed while working on this is that > BrowserUsageTelemetry doesn't handle two one-off sources generated by > search.xml: "oneoff-context-searchbar" and "oneoff-context-urlbar". Those > sources are generated when you search via the one-off context menu. I'm not > sure whether you're the right person to ask, but that seems like an > oversight. Do you know anything about that? Do those actually fire separately? Or do they show up as "oneoff-urlbar"/"oneoff-searchbar"? Mak, do you know if these are used and should be covered? We should add Telemetry & test coverage if we need them. (In reply to Dave Zeber [:dzeber] from comment #5) > (In reply to Drew Willcoxon :adw from comment #4) > > Oh also: > > > > * It sets the histogram to expire in version 58. I'm not sure what we want > > for that. > > The plan for these search probes is to start using them as standard > instrumentation for evaluating search, so if possible I'd prefer them not to > expire (expires in version "never"). If that's not feasible, maybe we can > set it 10 versions out. So, you could start out with "never" and flag for data review [1] after the technical review. Worst-case you need to change the expiry before landing. 1: https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval

Comment on attachment 8848693 [details] Bug 1334633 - Record the engine position in the list when a one-off button is clicked. f?gfritzsche https://reviewboard.mozilla.org/r/121594/#review129364 ::: browser/base/content/browser.js:3958 (Diff revision 1) > + * @param engineIndex > + * (integer) The index of the engine/button that was chosen in the list > + * of one-off buttons, or -1 if the search was performed without > + * choosing a one-off or the one-off is unknown. > */ > - recordOneoffSearchInTelemetry(engine, source, type, where) { > + recordOneoffSearchInTelemetry(engine, source, type, where, engineIndex = -1) { I assume test coverage for the various scenarios is still coming? ::: browser/modules/BrowserUsageTelemetry.jsm:407 (Diff revision 1) > Services.telemetry.keyedScalarAdd("browser.engagement.navigation." + source, > scalarKey, 1); > Services.telemetry.recordEvent("navigation", "search", source, action, > { engine: getSearchEngineId(engine) }); > + if (oneOffEngineIndex >= 0) { > + oneOffEngineIndex = Math.min(oneOffEngineIndex, 32); I think you don't need to do the boxing. Telemetry should handle this and put it into an overflow bucket for your histogram. Without reading up on the code, i recommend to just quickly test this and check the results in about:telemetry. ::: toolkit/components/telemetry/Histograms.json:5495 (Diff revision 1) > + "bug_numbers": [1334633], > + "alert_emails": ["adw@mozilla.com"], > + "expires_in_version": "58", > + "kind": "enumerated", > + "n_values": 32, > + "description": "Index of the selected engine in one-off searches." What are the typical/expected value ranges?

(In reply to Georg Fritzsche [:gfritzsche] from comment #6) > > Finally, an unrelated thing I noticed while working on this is that > > BrowserUsageTelemetry doesn't handle two one-off sources generated by > > search.xml: "oneoff-context-searchbar" and "oneoff-context-urlbar". Those > > sources are generated when you search via the one-off context menu. I'm not > > sure whether you're the right person to ask, but that seems like an > > oversight. Do you know anything about that? > > Do those actually fire separately? Or do they show up as > "oneoff-urlbar"/"oneoff-searchbar"? > Mak, do you know if these are used and should be covered? > We should add Telemetry & test coverage if we need them. I don't know if these are intended to be used, that's probably a question for Dave/Javaun. I think currently BrowserUsageTelemetry.jsm throws on them, and we just reportError that (TBConfirmed). Off-hand it sounds like a bug to me, they were probably not part of the initial analysis and thus were left out of KNOWN_ONEOFF_SOURCES. I'd sugges to file a bug for discussing whether we need/want those and act accordingly.

Unassigning myself from non-P1 search bugs for now so I have more time for Photon bugs.

Moving to p3 because no activity for at least 1 year(s). See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information