toolOpened: may call: T.getHistogramById().add(), T.scalarSet(), T.keyedScalarAdd() and startTimer() toolClosed: may call stopTimer() There is no equivalent to toolOpened/toolClosed. However we abuse this method at the moment, calling it in situations which have nothing to do with a tool "opening": toolOpened("toolbox"), toolOpened("gridInspectorShowGridAreasOverlayChecked"), toolOpened("copyuniquecssselector") etc... Anything that doesn't have a timer should not use toolOpened().

It is best to remove these methods because they are too vulnerable to abuse and it is good for us to see exactly what we are sending to telemetry.

Comment on attachment 8974327 [details] Bug 1458206 - telemetry.js: Cleanup toolOpened/toolClosed https://reviewboard.mozilla.org/r/242658/#review248982 Thanks for the patch! I think it's clear that we still need an abstraction layer for opening/closing tools, and I feel it would be better to keep it in our telemetry helper. The plan suggested by the RFC was : `Anything that doesn't have a timer should not use toolOpened(). After this cleanup is performed, maybe we could reassess toolOpened and see if it fits better in toolbox.js than in telemetry.js?`. What do you think about reverting the changes in sidebar.js, toolbox.js and toolsidebar.js and keeping toolOpened/toolClosed in telemetry.js? ::: devtools/client/accessibility/picker.js:10 (Diff revision 3) > "use strict"; > > const { L10N } = require("./utils/l10n"); > > +const TELEMETRY_ACCESSIBILITY_PICKER_COUNT = "devtools.accessibility.picker_used_count"; > +const TELEMETRY_ACCESSIBILITY_PICKER_TIME_OPEN = nit: maybe rename this const to TIME_ACTIVE rather than TIME_OPEN to match the name of the histogram?

(In reply to Julian Descottes [:jdescottes][:julian] from comment #8) > Comment on attachment 8974327 [details] > Bug 1458206 - telemetry.js: Remove toolOpened/toolClosed > > https://reviewboard.mozilla.org/r/242658/#review248982 > > Thanks for the patch! I think it's clear that we still need an abstraction > layer for opening/closing tools, and I feel it would be better to keep it in > our telemetry helper. > The plan suggested by the RFC was : `Anything that doesn't have a timer > should not use toolOpened(). After this cleanup is performed, maybe we could > reassess toolOpened and see if it fits better in toolbox.js than in > telemetry.js?`. > > What do you think about reverting the changes in sidebar.js, toolbox.js and > toolsidebar.js and keeping toolOpened/toolClosed in telemetry.js? > > ::: devtools/client/accessibility/picker.js:10 > (Diff revision 3) > > "use strict"; > > > > const { L10N } = require("./utils/l10n"); > > > > +const TELEMETRY_ACCESSIBILITY_PICKER_COUNT = "devtools.accessibility.picker_used_count"; > > +const TELEMETRY_ACCESSIBILITY_PICKER_TIME_OPEN = > > nit: maybe rename this const to TIME_ACTIVE rather than TIME_OPEN to match > the name of the histogram? Sure, we can do that. I am nervous that the method may be abused again though. I think that if we auto generate the histogram IDs from the tool name we force devs to use both histogram types so that should limit the abuse.

Review ping... I have made all the changes you requested.

Comment on attachment 8974327 [details] Bug 1458206 - telemetry.js: Cleanup toolOpened/toolClosed https://reviewboard.mozilla.org/r/242658/#review250508 Sorry about the delay for the review. This looks good, thanks Mike! ::: devtools/client/shared/telemetry.js:522 (Diff revision 6) > + * NOTE: This method is designed for tools that send multiple probes on open, > + * one of those probes being a counter and the other a timer. If you > + * only have one probe you should be using another method. Both here and in toolClosed we could throw if timerHist is not defined in order to enforce this. ::: devtools/client/shared/telemetry.js:565 (Diff revision 6) > + * NOTE: This method is designed for tools that send multiple probes on open, > + * one of those probes being a counter and the other a timer. If you only > + * have one probe you should be using another method. this method is not exposed outside of this module so we can probably remove the NOTE/warning.

Comment on attachment 8974327 [details] Bug 1458206 - telemetry.js: Cleanup toolOpened/toolClosed https://reviewboard.mozilla.org/r/242658/#review251338 ::: devtools/client/shared/telemetry.js:522 (Diff revision 6) > + * NOTE: This method is designed for tools that send multiple probes on open, > + * one of those probes being a counter and the other a timer. If you > + * only have one probe you should be using another method. Agreed and done. ::: devtools/client/shared/telemetry.js:522 (Diff revision 6) > + * NOTE: This method is designed for tools that send multiple probes on open, > + * one of those probes being a counter and the other a timer. If you > + * only have one probe you should be using another method. Agreed and done. ::: devtools/client/shared/telemetry.js:565 (Diff revision 6) > + * NOTE: This method is designed for tools that send multiple probes on open, > + * one of those probes being a counter and the other a timer. If you only > + * have one probe you should be using another method. Agreed and done.

Pushed by mratcliffe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6b6bdb43cdc3 telemetry.js: Cleanup toolOpened/toolClosed r=jdescottes