Closed
Bug 1465703
Opened 6 years ago
Closed 6 years ago
Add bookmark and follow_bookmark event telemetry for Savant Shield study
Categories
(Firefox :: General, enhancement, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 62
People
(Reporter: bdanforth, Assigned: bdanforth)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
adw
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details |
This is one of a series of bugs that together comprise the Savant Shield study (tracking bug 1457226).
This bug will register and record (for the duration of the study only):
* When the user saves a bookmark.
* When the user removes a bookmark.
* When the user opens a bookmark.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bdanforth
Status: NEW → ASSIGNED
Updated•6 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
:pdehaan, could you provide QA review on this patch? Instructions for obtaining the test build are here: https://github.com/biancadanforth/gecko-dev/issues/5
Flags: needinfo?(pdehaan)
Assignee | ||
Comment 3•6 years ago
|
||
I neglected to mention that for the event telemetry to show up in `about:telemetry#events-tab`, the study pref `shield.savant.enabled` needs to be set to `true`. Normandy will take care of turning this pref ON and OFF when the study is officially launched.
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8983736 [details]
Bug 1465703 - Add bookmark and follow_bookmark probes for Savant Shield study;
https://reviewboard.mozilla.org/r/249564/#review256050
Looks good, just some nits.
::: browser/modules/SavantShieldStudy.jsm:197
(Diff revision 1)
> + this.METHOD_1 = "bookmark";
> + this.EXTRA_SUBCATEGORY_1 = "feature";
> + this.METHOD_2 = "follow_bookmark";
> + this.EXTRA_SUBCATEGORY_2 = "navigation";
> + // nsINavBookmarksService constant
> + this.TYPE_BOOKMARK = 1;
Any reason to hardcode 1 instead of using Ci.nsINavBookmarksService.TYPE_BOOKMARK?
::: browser/modules/SavantShieldStudy.jsm:201
(Diff revision 1)
> + // nsINavBookmarksService constant
> + this.TYPE_BOOKMARK = 1;
> + }
> +
> + init() {
> + this.observer = {
Using a separate this.observer object is fine, but it would be slightly simpler to pass this BookmarkObserver object directly to add/removeObserver() and implement the observer methods on BookmarkObserver directly. Up to you.
::: browser/modules/SavantShieldStudy.jsm:206
(Diff revision 1)
> + this.observer = {
> + // Ignore "fake" bookmarks created for bookmark tags
> + skipTags: true,
> +
> + onItemAdded: (itemID, parentID, index, itemType, uri, title, dateAdded, guid, parentGUID, source) => {
> + if (itemType === this.TYPE_BOOKMARK && !uri.schemeIs("place")
The implementations of this method and onItemRemoved are very similar. Have you thought about factoring it out into a single method and calling it from both places? No big deal, up to you.
Attachment #8983736 -
Flags: review?(adw) → review+
Updated•6 years ago
|
Attachment #8983736 -
Flags: review?(mak77)
Comment 5•6 years ago
|
||
Clearing the r? on Marco since I think you wanted one of us to review first come first serve, but feel free to look at this too Marco of course
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8983736 [details]
Bug 1465703 - Add bookmark and follow_bookmark probes for Savant Shield study;
https://reviewboard.mozilla.org/r/249564/#review256074
Attachment #8983736 -
Flags: review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8983736 -
Flags: review?(mak77)
Comment 8•6 years ago
|
||
R+
Me thinks this looks good. I was fiddling w/ some bookmarken and saw the following noise in my about:telemetry tab:
timestamp | category | method | object | value | extra
----------|----------|-----------------|--------|-------|-------
153193 | savant | follow_bookmark | open | null | {subcategory: "navigation}
284500 | savant | bookmark | save | null | {subcategory: "feature"}
299093 | savant | bookmark | remove | null | {subcategory: "feature"}
So as long as we're kosher w/ the "follow_bookmark" versus "bookmark" methods and the different subcategories, I think this looks great.
Flags: needinfo?(pdehaan)
Assignee | ||
Comment 9•6 years ago
|
||
Hi Peter,
Thank you for pointing this out. This is the expected distinction[1], as a follow_bookmark event means a user has visited a bookmark, and we consider that to be in the broad category of navigation, whereas adding/removing bookmarks is considered using bookmarks as a feature.
[1]:https://docs.google.com/spreadsheets/d/192unlFYdQ_nc5UVaG79YhY2bp6bV7XZm1G4BW-etOLE/edit?usp=sharing
Flags: needinfo?(pdehaan)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(pdehaan)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8983736 -
Flags: review?(mak77)
Comment 11•6 years ago
|
||
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7deab88709c7
Add bookmark and follow_bookmark probes for Savant Shield study; r=adw
Comment 12•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Assignee | ||
Comment 13•6 years ago
|
||
Comment on attachment 8983736 [details]
Bug 1465703 - Add bookmark and follow_bookmark probes for Savant Shield study;
Approval Request Comment
[Feature/Bug causing the regression]: There is no regression. The tracking bug for this study is 1457226.
[User impact if declined]: None. This patch is part of a priority Shield study.
[Is this code covered by automated tests?]: There are no study-specific tests, but all code has been run on the try server for regression testing and has been approved by both a Firefox peer and QA (pdehaan).
[Has the fix been verified in Nightly?]: The study’s functionality has been verified in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: This request applies to all patches in this bug. In general, here is a comprehensive list of bugs whose patches need to be uplifted (in this order) to complete the study: 1462725, 1465698, 1465707, 1465703, 1465704, 1465697, 1465685, 1465694
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is an observational pref-flip study with no branches and no UI. It adds some event telemetry probes in various places and a study module to enable/disable collection. Each patch has been QA tested and R+ by pdehaan.
[String changes made/needed]: None.
Attachment #8983736 -
Flags: approval-mozilla-beta?
Comment 14•6 years ago
|
||
Comment on attachment 8983736 [details]
Bug 1465703 - Add bookmark and follow_bookmark probes for Savant Shield study;
Telemetry code needed to support the Fx61 Savant work. Approved for 61.0b13.
Attachment #8983736 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 15•6 years ago
|
||
bugherder uplift |
status-firefox61:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•