1470128 - Instrument usage of webcomponent inspection

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Julian Descottes [:jdescottes]]

We should try to measure usage and adoption of the inspector support for shadowdom/webcomponents.

Things we could measure:
- number of times a shadowroot element is displayed
- number of times a shadowroot element is expanded
- number of times a reveal link is clicked

We will use regular scalars for this.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1470128 - Instrument inspector shadowdom support;

Review commit: https://reviewboard.mozilla.org/r/252724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252724/

Comment 2

[Julian Descottes [:jdescottes]]

I currently implemented as explained in the summary, with one twist for "number of times a shadowroot element is expanded". For this one, it is only recorded once per container. This means that if a user keeps expanding/collapsing a shadow-root, it will still only count as 1. The intent is to be able to compare this to the first "displayed" scalar.

Note that we could also count the number of "reveal" links displayed, and compare it in the same way with the number of clicks.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

https://reviewboard.mozilla.org/r/252724/#review259544

Thanks Julian. You will also need a data review on this.

::: devtools/client/inspector/markup/markup.js:1240
(Diff revision 1)
>          // Could not expand the node, the markup-view was destroyed in the meantime. Just
>          // silently give up.
>          return;
>        }
> +
> +      if (container.node.isShadowRoot && !container._recordedShadowRootExpanded) {

The word record might imply various things here, I would prefer if you made this variable name more specifically related to telemetry.
Not a big deal though, and I can't really come up with a variable name that I like. `_expandedTelemetryRecorded`?

Anyway, I was also thinking that this whole if statement might be better placed inside the container's `setExpanded` method instead.
This way no need to access a private property from here, everything can be done in isolation inside the container class.

::: devtools/client/inspector/markup/views/slotted-node-container.js:46
(Diff revision 1)
>  
>      const selection = this.markup.inspector.selection;
>      if (selection.nodeFront != this.node || selection.isSlotted()) {
>        const reason = "reveal-from-slot";
>        this.markup.inspector.selection.setNodeFront(this.node, { reason });
> +      this.markup.telemetry.scalarAdd("devtools.shadowdom.reveal_link_clicked", 1);

Could the new `event` telemetry type be better suited for this? After all, this is a click event, and I think this new probe type was added specifically for tracking these kinds of things.
Now, having said that, your 3 scalar probes do make sense if recorded together in a similar fashion, because we'll mostly want to compare them. So let's keep this is a scalar.

::: toolkit/components/telemetry/Scalars.yaml:969
(Diff revision 1)
> +      - 'main'
> +  shadow_root_expanded:
> +    bug_numbers:
> +      - 1470128
> +    description: >
> +      Number of times a shadow root element was expanded in the markup view.

Maybe rephrase to make it obvious that this is recorded just once per shadow root, not everytime one is expanded.

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252724/diff/1-2/

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

https://reviewboard.mozilla.org/r/252724/#review259554


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/markup/views/markup-container.js:339
(Diff revision 2)
>      }
>  
>      if (this.showExpander) {
>        this.tagLine.setAttribute("aria-expanded", this.expanded);
>      }
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

https://reviewboard.mozilla.org/r/252724/#review259544

Thanks!

> The word record might imply various things here, I would prefer if you made this variable name more specifically related to telemetry.
> Not a big deal though, and I can't really come up with a variable name that I like. `_expandedTelemetryRecorded`?
> 
> Anyway, I was also thinking that this whole if statement might be better placed inside the container's `setExpanded` method instead.
> This way no need to access a private property from here, everything can be done in isolation inside the container class.

Good point. Also setExpanded is just a better spot overall because some codepaths could call setExpanded without going through _expandContainer and we would have missed those.

Renamed the property to _shadowRootExpandedTelemetryRecorded

> Could the new `event` telemetry type be better suited for this? After all, this is a click event, and I think this new probe type was added specifically for tracking these kinds of things.
> Now, having said that, your 3 scalar probes do make sense if recorded together in a similar fashion, because we'll mostly want to compare them. So let's keep this is a scalar.

Harald mentioned that we should stick to "traditional probes" for this until we figure out how to properly use Event telemetry. There is still a bit of confusion regarding the naming of the events etc...

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252724/diff/2-3/

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252724/diff/3-4/

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

Re-flagging for review. Changed the implementation to use scalarSet here. 

I think scalarAdd would make the data harder to understand and less meaningful.
If a user has a markupview with a lot of shadowroot elements but only ever needs to work on one at a moment (which would often be the case), the scalarAdd implementation would translate this as "user rarely expands shadowroot" nodes. 

scalarSet will aggregate this information by session. In the long run I think this will be more useful.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug_1470128_data_review.txt

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

https://reviewboard.mozilla.org/r/252724/#review259562

Comment 12

[Chris H-C :chutten]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

https://reviewboard.mozilla.org/r/252724/#review259774

A drive-by since I'm in the neighbourhood...

::: toolkit/components/telemetry/Scalars.yaml:956
(Diff revision 4)
> +devtools.shadowdom:
> +  shadow_root_displayed:
> +    bug_numbers:
> +      - 1470128
> +    description: >
> +      Number of times shadow root elements are displayed in the markup view.

If it's the number of times, shouldn't the call sites be using scalarAdd instead of scalarSet?

::: toolkit/components/telemetry/Scalars.yaml:965
(Diff revision 4)
> +      - [e-mail]
> +      - [e-mail]
> +    release_channel_collection: opt-out
> +    record_in_processes:
> +      - 'main'
> +  shadow_root_expanded:

A line of whitespace between scalar definitions is typical in this file.

Comment 13

[Chris H-C :chutten]

Comment on attachment 8987760 [details]
bug_1470128_data_review.txt

DATA COLLECTION REVIEW RESPONSE

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Standard Telemetry mechanisms apply.

    Is there a control mechanism that allows the user to turn the data collection on and off?

Standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

N/A

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Category 2

    Is the data collection request for default-on or default-off?

Default on

    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.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes. 

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :jdescottes will be responsible for checking in on this and determining whether to renew or remove.

---
Result: datareview+

Comment 14

[Julian Descottes [:jdescottes]]

(In reply to Chris H-C :chutten from comment #12)
> Comment on attachment 8987479 [details]
> Bug 1470128 - Instrument inspector shadowdom support;
> 
> https://reviewboard.mozilla.org/r/252724/#review259774
> 
> A drive-by since I'm in the neighbourhood...
> 
> ::: toolkit/components/telemetry/Scalars.yaml:956
> (Diff revision 4)
> > +devtools.shadowdom:
> > +  shadow_root_displayed:
> > +    bug_numbers:
> > +      - 1470128
> > +    description: >
> > +      Number of times shadow root elements are displayed in the markup view.
> 
> If it's the number of times, shouldn't the call sites be using scalarAdd
> instead of scalarSet?
> 

Thanks for having a look, I definitely appreciate having data-peer feedback on the patch itself!

I changed scalarAdd to scalarSet in the last version of my patch because I think it will make the scalars easier to compare. The main question I am trying to answer here is "for developers who use webcomponents, what % of them will expand the nodes in the inspector, and what % will use the `reveal` link feature". Using scalarSet and therefore only recording 1 scalar per session makes this easier to analyze in my opinion. If I simply switch to scalarAdd, the "displayed" will be hard to compare to the "expanded" and "clicked" ones, because "displayed" would be increased for every "shadow-root" element that gets displayed, and there might be several displayed at once. So I could also complexify the code and call scalarAdd only once per "page", and the same for expand etc... But I think using scalarSet achieves a similar goal without having to complexify the telemetry code.

Of course I can update the comment to mention it is "per session".

Comment 15

[Chris H-C :chutten]

It will actually be per _subsession_, not per session (though many sessions are only one subsession long), but similar logic applies: If you are wondering whether a given unit of user time contains relative proportions of the actions under measurement, this'll get the job done.

Boolean scalars may make more sense here and a rewording to "whether the user who displays shadow roots expands/clicks on one" or something to that extent, rather than "number of times...".

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8987479 [details]
Bug 1470128 - Instrument inspector shadowdom support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252724/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d04c63b5a03b
Instrument inspector shadowdom support;r=pbro

Comment 18

[Raul Gurzau (:RaulG)]

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