Add Telemetry Probe for page action menu item

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: abenson, Assigned: Gijs)

Tracking

unspecified
Firefox 58
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Specifically want to track which items people pin to their URL bars (Pocket, Copy URL, etc.) to get a sense of A) is the pinning functionality discoverable in its current design implementation and B) what items are being found most useful?
(Assignee)

Updated

2 years ago
Whiteboard: [photon-structure][triage]
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Iteration: --- → 57.3 - Sep 19
(Assignee)

Updated

2 years ago
Attachment #8910355 - Flags: review?(rweiss)
(Assignee)

Comment 2

2 years ago
Drew: I'm wondering if there are better places to call logTelemetry() from for the buttons that are "special" because they insert their own nodes and so on - I didn't see them, but maybe I'm missing them?
(This comment ended up being a little long, bear with me...)

(In reply to :Gijs from comment #2)
> Drew: I'm wondering if there are better places to call logTelemetry() from
> for the buttons that are "special" because they insert their own nodes and
> so on - I didn't see them, but maybe I'm missing them?

Let's see...

* Bookmark star: The urlbar button is in markup with an onclick handler that calls BrowserPageActions.bookmark.onUrlbarNodeClicked().  The panel button is managed by BrowserPageActions.

* Pocket: The urlbar button is managed by Pocket code with its own click listener, but that listener calls PageAction.doCommand() which calls BrowserPageActions.doCommandForAction(), so its click handling is basically managed by BrowserPageActions.  The panel button is managed by BrowserPageActions.

* Screenshots: Both the urlbar and panel buttons are managed by BrowserPageActions.

* Webcompat: Both the urlbar and panel buttons are managed by BrowserPageActions.

The only outlier above is the bookmark star because it bypasses the "normal" click/command handling, by which I mean basically:

click on DOM node:
  if action.wantsIframe:
    show iframe in activated-action panel
  else:
    call action.onCommand

But internally in browser-pageActions.js, we kind of don't have one single "normal" click/command handler.  We have three separate implementations: on command events for panel buttons, on click events for urlbar buttons, and doCommandForAction().  I added doCommandForAction after the initial implementation, for another bug, but with the intention of at some point making the click/command listeners call it.  I think we should do that now to fix this bug right.

So once the panel button command listener calls doCommandForAction, and the urlbar button listener calls doCommandForAction, then the one right place to add telemetry for clicks/commands is doCommandForAction.

The bookmark star's onclick handler in markup would need to be modified to call BrowserPageActions.doCommandForAction(PageActions.actionForID("bookmark")) so that clicks on it go through doCommandForAction.  And any other actions we add in the future that aren't managed by BrowserPageActions would need to do the same.

Does that all make sense?

Comment 4

2 years ago
mozreview-review
Comment on attachment 8910355 [details]
Bug 1393843 - add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss,

https://reviewboard.mozilla.org/r/181818/#review187218

Clearing r? for now, pending discussion of my earlier comment or a new patch.

::: browser/modules/PageActions.jsm:406
(Diff revision 1)
>  
> +  logTelemetry(type, action, node = null) {
> +    if (type == "used") {
> +      type = node.closest("#urlbar-container") ? "urlbar_used" : "panel_used";
> +    }
> +    const kAllowedLabels = [

Hmm, this basically duplicates the IDs in gBuiltInActions in PageActions.jsm (plus Pocket, screenshots, and webcompat of course).  Do you think you could use those instead (excluding separator actions of course)?  Or would there be a reason not to tie these allowed labels to all the built-in action IDs?  (e.g., maybe we don't necessarily want to log telemetry for all built-in actions)

::: browser/modules/PageActions.jsm:415
(Diff revision 1)
> +    let histogramID = "FX_PAGE_ACTION_" + type.toUpperCase();
> +    try {
> +      let histogram = Services.telemetry.getHistogramById(histogramID);
> +      // "webcompat-reporter-button" is too long for histograms,
> +      // so poor-man-mangle the names.
> +      let actionID = action.id.match(/^[a-zA-Z0-9]+/)[0];

Hmm, this doesn't seem to really match what the code comment says.  Like, maybe there's an action ID that matches this regex but that's still too long.  Seems like either the ID should be truncated, or we could just have a mapping from action IDs to short-enough labels.
Attachment #8910355 - Flags: review?(adw)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8910355 [details]
Bug 1393843 - add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss,

https://reviewboard.mozilla.org/r/181818/#review187258

::: browser/base/content/browser-pageActions.js:364
(Diff revision 3)
>        if (event.button != 0) {
>          return;
>        }

We now do this in 3 places, maybe we should pull it into doCommandForAction.

::: browser/base/content/browser.xul:913
(Diff revision 3)
>                  <hbox id="star-button-box"
>                        hidden="true"
>                        class="urlbar-icon-wrapper urlbar-page-action"
>                        context="pageActionPanelContextMenu"
>                        oncontextmenu="BrowserPageActions.onContextMenu(event);"
> -                      onclick="BrowserPageActions.bookmark.onUrlbarNodeClicked(event);">
> +                      onclick="if (event.button == 0) BrowserPageActions.doCommandForAction(PageActions.actionForID('bookmark'));">

Self-nit: this should pass `event` and `this` as the second and third params.

Comment 8

2 years ago
mozreview-review
Comment on attachment 8910355 [details]
Bug 1393843 - add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss,

https://reviewboard.mozilla.org/r/181818/#review187250

::: browser/base/content/browser-pageActions.js:367
(Diff revision 2)
>      }
>      buttonNode.addEventListener("click", event => {
>        if (event.button != 0) {
>          return;
>        }
> -      if (action.subview || action.wantsIframe) {
> +      BrowserPageActions.doCommandForAction(action, event, buttonNode);

Can you use `this` instead of `BrowserPageActions`?

::: browser/base/content/browser-pageActions.js:459
(Diff revision 2)
>    },
>  
> -  doCommandForAction(action) {
> +  doCommandForAction(action, event, buttonNode) {
> +    PageActions.logTelemetry("used", action, buttonNode);
> +    // If we're in the panel, open a subview inside the panel:
> +    if (action.subview && buttonNode.closest("panel")) {

You should probably null-check buttonNode?  The bookmark star's onclick doesn't pass one in for example.  Of course its action doesn't have a subview either so this conditional will short circuit, but that's only coincidental.

::: browser/base/content/browser-pageActions.js:466
(Diff revision 2)
> +      let panelViewNode = document.getElementById(panelViewNodeID);
> +      action.subview.onShowing(panelViewNode);
> +      this.multiViewNode.showSubView(panelViewNode, buttonNode);
> +      return;
> +    }
> +    // Otherwise, hide whatever popup was open:

this.panelNode is the main panel, so "whatever popup" is misleading here.  (And only closing the main panel should be fine, since the only other panel is the activated-action panel, and clicking outside it will close it.)

::: browser/base/content/browser-pageActions.js:469
(Diff revision 2)
> +      return;
> +    }
> +    // Otherwise, hide whatever popup was open:
> +    this.panelNode.hidePopup();
> +
> +    // Toggle a separate panel if necessary

Nit: a separate -> the activated-action panel

("A separate" is unnecessarily ambiguous IMO)

::: browser/extensions/pocket/bootstrap.js:143
(Diff revision 2)
>            wrapper.addEventListener("click", event => {
>              if (event.type == "click" && event.button != 0) {
>                return;
>              }
> -            this.doCommand(event.target.ownerGlobal);
> +            let {BrowserPageActions} = wrapper.ownerDocument;
> +            BrowserPageActions.doCommandForAction(this, event, wrapper);

Hmm, I guess this is OK.  I think it means that PageAction.prototype.doCommand is now unused.  (TBH I go back and forth between whether there should be methods on PageAction that take browser windows, or whether we should just call into BrowserPageActions directly.)

::: browser/modules/PageActions.jsm:408
(Diff revisions 2 - 3)
>      if (type == "used") {
>        type = node.closest("#urlbar-container") ? "urlbar_used" : "panel_used";
>      }
> -    const kAllowedLabels = [
> -      "bookmark", "pocket", "screenshots", "webcompat-reporter-button",
> -      "copyURL", "emailLink", "sendToDevice"
> +
> +    const kAllowedLabels = ["pocket", "screenshots", "webcompat"].concat(
> +      gBuiltInActions.filter(a => !a._isSeparator).map(a => a.id)

I think you actually need two underscores here (since all options-that-become-properties get prefixed with an underscore, so an option that's already prefixed gets two underscores).  See for example: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pageActions.js#119

Confusing, sorry about that.

::: browser/modules/PageActions.jsm:563
(Diff revisions 2 - 3)
>    setProperties(this, options, {
>      id: true,
>      title: !options._isSeparator,
>      anchorIDOverride: false,
>      iconURL: false,
> +    labelForHistogram: "",

This should be `false` instead of an empty string.  The value is a boolean that's true if the option is required.  (Confusing again, sorry.)
Attachment #8910355 - Flags: review?(adw) → review+
Comment hidden (mozreview-request)
Iteration: 57.3 - Sep 19 → ---
(Assignee)

Comment 10

2 years ago
Comment on attachment 8910355 [details]
Bug 1393843 - add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss,

Hrmpf, the review? flag for data/privacy-review got cleared. :-(
Attachment #8910355 - Flags: review?(rweiss)

Comment 11

2 years ago
This is going to be an easy review, but can you add a file to this bug that has answers to the questions in this doc?
https://docs.google.com/document/d/1SSn5w8DfCSkHWJS8DNTd7ya82diWRxaDUFM5aL4UDDo/edit
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 12

2 years ago
(In reply to Rebecca Weiss from comment #11)
> This is going to be an easy review, but can you add a file to this bug that
> has answers to the questions in this doc?
> https://docs.google.com/document/d/
> 1SSn5w8DfCSkHWJS8DNTd7ya82diWRxaDUFM5aL4UDDo/edit

I've tried, but I haven't seen this form before (though some of the questions seem familiar) and I'm not sure I'm the best person to fill this in. Let me know if you have further questions.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(rweiss)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8910355 [details]
Bug 1393843 - add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss,

https://reviewboard.mozilla.org/r/181818/#review189976

FWIW, this can be default on if you want, as it is category 2 measurement.

1) Is there documentation that describes the schema for the ultimate data set available publicly, complete and accurate? 
Yes, standard Telemetry documentation.

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism)  
Yes, users can opt out of Telemetry in the normal fashion.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?
Not permanent data collection.

4) Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? 
This is type 2.

5) Is the data collection default-on or default-off? 
Eligible for default on, but the request does not specifically ask for this.  

6) Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? 
No.

7) Is the data collection covered by the existing Firefox privacy notice? If unsure: escalate to legal.
Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) 
No
Attachment #8910355 - Flags: review?(rweiss) → review+

Updated

2 years ago
Flags: needinfo?(rweiss)
Comment hidden (mozreview-request)

Comment 15

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a49cb431c7ec
add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss, r=adw,rweiss+418169

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a49cb431c7ec
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Comment 17

2 years ago
Comment on attachment 8910355 [details]
Bug 1393843 - add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss,

Approval Request Comment
[Feature/Bug causing the regression]: we added the page action menu feature
[User impact if declined]: we won't have data and won't know where to focus efforts to improve the feature
[Is this code covered by automated tests?]: yes, though not the telemetry per se
[Has the fix been verified in Nightly?]: not yet. It should be verifiable through the telemetry dist viewer ( https://telemetry.mozilla.org/new-pipeline/dist.html ) once this has been in nightly a few days and the telemetry has made its way to our telemetry servers (it takes a few days).
[Needs manual test from QE? If yes, steps to reproduce]: not really, we can hopefully verify through the dist viewer that this is working on nightly. If we want to manually verify, we can check that:

- activating an action in the url bar
- activating an action in the page action menu
- moving an action to the url bar
- moving an action out of the url bar

all increment their respective histograms using about:telemetry.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: we're only really adding telemetry to an existing feature, a feature that is well-tested in automation, and we have a lot of time to spot issues still.
[String changes made/needed]: nope
Attachment #8910355 - Flags: approval-mozilla-beta?
Comment on attachment 8910355 [details]
Bug 1393843 - add telemetry for page action items and the context menu that enables/disables them, data-review=rweiss,

Moar data is good, taking it
Should be in 57b5
Attachment #8910355 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox57: --- → affected

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/a438ffa660b7
status-firefox57: affected → fixed
You need to log in before you can comment on or make changes to this bug.