Bug 1480513: Add telemetry for search shortcut - #4290
Conversation
| "icon_type": ["screenshot_with_icon" | "screenshot" | "tippytop" | "rich_icon" | "no_image"] | ||
| "card_type": ["pinned" | "search"], | ||
| "icon_type": ["screenshot_with_icon" | "screenshot" | "tippytop" | "rich_icon" | "no_image"], | ||
| "search_vendor": ["google" | "amazon" | "baidu" | "yandex"] |
There was a problem hiding this comment.
This needs to be added to Blocking a site and pinning as well I think. Or would pinning be covered by adding a search shortcut?
There was a problem hiding this comment.
Yep, this will be added to those two events.
|
r? @piatra, I figure you're the best reviewer for this patch, since you added both "card_type" and "searchTopSite" :) Note that the "ADD_SEARCH_SHORTCUT" and "EDIT_SEARCH_SHORTCUT" events would be implemented separately by Bug 1480507, I've added the document for them here so that we only need one data review for the whole feature. |
ncloudioj
changed the title
| // Set the card_type as "search" regardless of its pinning status | ||
| value.card_type = "search"; | ||
| // Extract search vendor from the label, i.e. "@search_vendor" => "search_vendor" | ||
| value.search_vendor = this.props.link.label.slice(1); |
There was a problem hiding this comment.
this should just be link.hostname
| "icon_type": ["screenshot_with_icon" | "screenshot" | "tippytop" | "rich_icon" | "no_image"] | ||
| "card_type": ["pinned" | "search"], | ||
| "icon_type": ["screenshot_with_icon" | "screenshot" | "tippytop" | "rich_icon" | "no_image"], | ||
| "search_vendor": ["google" | "amazon"] |
There was a problem hiding this comment.
can you confirm that these are the only ones? spec has more
There was a problem hiding this comment.
I am not sure about the definite vendor list atm, I'll add a note for this field in data_dictionary.md
| | `client_id` | [Required] An identifier for this client. | :one: | ||
| | `card_type` | [Optional] ("bookmark", "pocket", "trending", "pinned") | :one: | ||
| | `card_type` | [Optional] ("bookmark", "pocket", "trending", "pinned", "search") | :one: | ||
| | `search_vendor` | [Optional] the vendor of the search shortcut, one of ("google", "amazon", "wikipedia", "duckduckgo", "bing", etc.). This field only exists when `card_type = "search"` | :one: |
There was a problem hiding this comment.
I might be misreading the code in the previous file, but wouldn't this.props.link.hostname return google.com, amazon.com or wikipedia.org?
There was a problem hiding this comment.
I believe hostname is already shortened by shortURL
|
Data review+ at https://bugzilla.mozilla.org/show_bug.cgi?id=1480513#c5 |
Fix Bug 1480513 : part 2 - add telemetry for search shortcut
4 participants
This documents the telemetry part for search shortcuts in Top Sites of Activity Stream.
@fmarier Could you take a look at this please? The data review request was attached at here