952585 - Collect UITelemetry on customization events and durations with Australis

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

This is the Australis version of bug 942274.

We want to collect counts on the following events:

1) Customization start
2) Item is moved
3) Item is removed to palette
4) Item is added from palette

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I'm gonna pack duration in here as well for kicks.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

This was extremely easy thanks to CustomizableUI. :D

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8350763 [details] [diff] [review]
Patch v1

Hm, I've just realized that this cannot differentiate between the user adding, moving or removing widgets, and an add-on adding, moving or removing widgets programmatically...

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Ok, going from the other direction here - instead of detecting widget movements via the listener (which might include programmatic movements), we call BrowserUITelemetry.countCustomizationEvent within CustomizeMode.jsm on drop events.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8359250 [details] [diff] [review]
Patch v2

What do you think of this, Gijs?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8359250 [details] [diff] [review]
Patch v2

Review of attachment 8359250 [details] [diff] [review]:
-----------------------------------------------------------------

I don't need to see another patch here but please note the comments; there's some bits that definitely want adjusting before landing.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1154,5 @@
>      // Is the target the customization area itself? If so, we just add the
>      // widget to the end of the area.
>      if (aTargetNode == aTargetArea.customizationTarget) {
>        CustomizableUI.addWidgetToArea(aDraggedItemId, aTargetArea.id);
> +      let custEventType = aTargetArea == aOriginArea ? "move" : "add";

This isn't the same as the thing below. You presumably want to check aOriginArea.id !== kPaletteId again, and have the same comment.

@@ +1195,5 @@
> +
> +    // For the purposes of BrowserUITelemetry, we consider both moving a widget
> +    // within the same area, and adding a widget from one area to another area
> +    // as a "move". An "add" is only when we move an item from the palette into
> +    // an area.

Nit: shortened version of this comment above where you want to use the same logic?

::: browser/modules/BrowserUITelemetry.jsm
@@ +202,5 @@
>      return parent;
>    },
>  
>    _countableEvents: {},
> +  _countEvent: function(aCategory, aAction) {

Nit: if you make this _countEvent: function(aKeyArray) {
you can use it from _countMouseUpEvent and consolidate.

On the other hand, only two lines of code, not sure the abstraction necessarily makes sense.

@@ +431,5 @@
>  
>    getToolbarMeasures: function() {
>      let result = this._firstWindowMeasurements || {};
>      result.countableEvents = this._countableEvents;
> +    result.durations = this._durations;

So this file is called BrowserUITelemetry.jsm, and while this is about toolbars and will get logged in telemetry as such, I wonder if we should name this variable customizationDurations or something else equally descriptive. Thoughts?

@@ +445,5 @@
> +  },
> +
> +  onCustomizeStart: function(aWindow) {
> +    this._countEvent("customize", "start");
> +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);

So you only set this in the callback above. And here you just assume it exists. While that is likely, my paranoia would be more at ease if you just set it upon use if it's null here:

if (!durationMap) {
  durationMap = {};
  WINDOW_DURATION_MAP.set(aWindow, durationMap);
}

@@ +450,5 @@
> +    durationMap.customization = aWindow.performance.now();
> +  },
> +
> +  onCustomizeEnd: function(aWindow) {
> +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);

Ditto, except here you should probably just bail if durationMap is null.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8359250 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8359250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't need to see another patch here but please note the comments; there's
> some bits that definitely want adjusting before landing.
> 
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +1154,5 @@
> >      // Is the target the customization area itself? If so, we just add the
> >      // widget to the end of the area.
> >      if (aTargetNode == aTargetArea.customizationTarget) {
> >        CustomizableUI.addWidgetToArea(aDraggedItemId, aTargetArea.id);
> > +      let custEventType = aTargetArea == aOriginArea ? "move" : "add";
> 
> This isn't the same as the thing below. You presumably want to check
> aOriginArea.id !== kPaletteId again, and have the same comment.
> 

Yeah, you're absolutely right. Thanks for catching that.

> @@ +1195,5 @@
> > +
> > +    // For the purposes of BrowserUITelemetry, we consider both moving a widget
> > +    // within the same area, and adding a widget from one area to another area
> > +    // as a "move". An "add" is only when we move an item from the palette into
> > +    // an area.
> 
> Nit: shortened version of this comment above where you want to use the same
> logic?

Yep, good plan.

> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +202,5 @@
> >      return parent;
> >    },
> >  
> >    _countableEvents: {},
> > +  _countEvent: function(aCategory, aAction) {
> 
> Nit: if you make this _countEvent: function(aKeyArray) {
> you can use it from _countMouseUpEvent and consolidate.
> 
> On the other hand, only two lines of code, not sure the abstraction
> necessarily makes sense.
>

I'll give it a shot and see what we get.
 
> @@ +431,5 @@
> >  
> >    getToolbarMeasures: function() {
> >      let result = this._firstWindowMeasurements || {};
> >      result.countableEvents = this._countableEvents;
> > +    result.durations = this._durations;
> 
> So this file is called BrowserUITelemetry.jsm, and while this is about
> toolbars and will get logged in telemetry as such, I wonder if we should
> name this variable customizationDurations or something else equally
> descriptive. Thoughts?
> 

Well, what we're going to get is a durations object, and inside that a "customization" array of the durations on that. I guess this keeps us open for other durations (I know UX was interested in eventually getting durations on things like...window drag sessions and stuff), so I think this should probably be OK. Lemme know if there's anything else that concerns you about this.

> @@ +445,5 @@
> > +  },
> > +
> > +  onCustomizeStart: function(aWindow) {
> > +    this._countEvent("customize", "start");
> > +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);
> 
> So you only set this in the callback above. And here you just assume it
> exists. While that is likely, my paranoia would be more at ease if you just
> set it upon use if it's null here:
> 
> if (!durationMap) {
>   durationMap = {};
>   WINDOW_DURATION_MAP.set(aWindow, durationMap);
> }
> 

Yeah OK - I can see a possible (but highly unlikely) scenario in which the durationMap could be queried for before it exists. I'll put in the safeguard.

> @@ +450,5 @@
> > +    durationMap.customization = aWindow.performance.now();
> > +  },
> > +
> > +  onCustomizeEnd: function(aWindow) {
> > +    let durationMap = WINDOW_DURATION_MAP.get(aWindow);
> 
> Ditto, except here you should probably just bail if durationMap is null.

Good call.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Thanks! Landed alterations as:

https://hg.mozilla.org/integration/fx-team/rev/a021dc363852

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a021dc363852