Bug 1807898 - [devtools] Set Telemetry sessionID by the Telemetry helper. r=devtools-reviewers,nchevobbe

This will help simplify all callsites manually setting this attribute.
I inlined simplification of all toolOpened/toolClosed in this changeset,
but all usages of recordEvent will be simplified in the next changeset.

Toolbox.sessionId is now only used by console performance markers in
order to help differentiate toolbox instances.

Note that about:debugging was also using a dedicated sessionId.

Differential Revision: https://phabricator.services.mozilla.com/D165935

devtools/client/aboutdebugging/src/middleware/event-recording.js

@@ -5,10 +5,10 @@
 "use strict";
 
 const Telemetry = require("resource://devtools/client/shared/telemetry.js");
-loader.lazyGetter(this, "telemetry", () => new Telemetry());
-// This is a unique id that should be submitted with all about:debugging events.
-loader.lazyGetter(this, "sessionId", () =>
-  parseInt(telemetry.msSinceProcessStart(), 10)
+loader.lazyGetter(
+  this,
+  "telemetry",
+  () => new Telemetry({ useSessionId: true })
 );
 
 const {
@@ -33,19 +33,17 @@ const {
 } = require("resource://devtools/client/aboutdebugging/src/modules/runtimes-state-helper.js");
 
 function recordEvent(method, details) {
-  // Add the session id to the event details.
-  const eventDetails = Object.assign({}, details, { session_id: sessionId });
-  telemetry.recordEvent(method, "aboutdebugging", null, eventDetails);
+  telemetry.recordEvent(method, "aboutdebugging", null, details);
 
   // For close and open events, also ping the regular telemetry helpers used
   // for all DevTools UIs.
   if (method === "open_adbg") {
-    telemetry.toolOpened("aboutdebugging", sessionId, window.AboutDebugging);
+    telemetry.toolOpened("aboutdebugging", window.AboutDebugging);
   } else if (method === "close_adbg") {
     // XXX: Note that aboutdebugging has no histogram created for
     // TIME_ACTIVE_SECOND, so calling toolClosed will not actually
     // record anything.
-    telemetry.toolClosed("aboutdebugging", sessionId, window.AboutDebugging);
+    telemetry.toolClosed("aboutdebugging", window.AboutDebugging);
   }
 }
 

devtools/client/accessibility/picker.js

@@ -145,11 +145,7 @@ class Picker {
     this.pickerButton.isChecked = false;
 
     await this.accessibilityProxy.cancelPick();
-    this._telemetry.toolClosed(
-      "accessibility_picker",
-      this.toolbox.sessionId,
-      this
-    );
+    this._telemetry.toolClosed("accessibility_picker", this);
     this.emit("picker-stopped");
   }
 
@@ -174,11 +170,7 @@ class Picker {
       this.onPickerAccessibleCanceled
     );
 
-    this._telemetry.toolOpened(
-      "accessibility_picker",
-      this.toolbox.sessionId,
-      this
-    );
+    this._telemetry.toolOpened("accessibility_picker", this);
     this.emit("picker-started");
   }
 

devtools/client/framework/browser-toolbox/Launcher.sys.mjs

@@ -367,9 +367,7 @@ export class BrowserToolboxLauncher extends EventEmitter {
       proc => {
         this.#dbgProcess = proc;
 
-        // jsbrowserdebugger is not connected with a toolbox so we pass -1 as the
-        // toolbox session id.
-        this.#telemetry.toolOpened("jsbrowserdebugger", -1, this);
+        this.#telemetry.toolOpened("jsbrowserdebugger", this);
 
         dumpn("Chrome toolbox is now running...");
         this.emit("run", this, proc, this.#dbgProfilePath);
@@ -436,9 +434,7 @@ export class BrowserToolboxLauncher extends EventEmitter {
     this.#dbgProcess.stdout.close();
     await this.#dbgProcess.kill();
 
-    // jsbrowserdebugger is not connected with a toolbox so we pass -1 as the
-    // toolbox session id.
-    this.#telemetry.toolClosed("jsbrowserdebugger", -1, this);
+    this.#telemetry.toolClosed("jsbrowserdebugger", this);
 
     dumpn("Chrome toolbox is now closed...");
     processes.delete(this);

devtools/client/framework/test/browser_toolbox_telemetry_open_event.js

@@ -17,9 +17,12 @@ add_task(async function() {
   await onToolboxReady;
 
   const snapshot = Services.telemetry.snapshotEvents(ALL_CHANNELS, true);
+  // The telemetry is sent by DevToolsStartup and so isn't flaged against any session id
   const events = snapshot.parent.filter(
     event =>
-      event[1] === "devtools.main" && event[2] === "open" && event[4] === null
+      event[1] === "devtools.main" &&
+      event[2] === "open" &&
+      event[5].session_id == -1
   );
 
   is(events.length, 1, "Telemetry open event was logged");

devtools/client/framework/toolbox.js

@@ -238,7 +238,9 @@ function Toolbox(
   this._win = contentWindow;
   this.frameId = frameId;
   this.selection = new Selection();
-  this.telemetry = new Telemetry();
+  this.telemetry = new Telemetry({ useSessionId: true });
+  // This attribute helps identify one particular toolbox instance.
+  this.sessionId = this.telemetry.sessionId;
 
   // This attribute is meant to be a public attribute on the Toolbox object
   // It exposes commands modules listed in devtools/shared/commands/index.js
@@ -247,11 +249,6 @@ function Toolbox(
   this.commands = commands;
   this._descriptorFront = commands.descriptorFront;
 
-  // The session ID is used to determine which telemetry events belong to which
-  // toolbox session. Because we use Amplitude to analyse the telemetry data we
-  // must use the time since the system wide epoch as the session ID.
-  this.sessionId = msSinceProcessStart;
-
   // If the user opened the toolbox, we can now enable the F12 shortcut.
   if (Services.prefs.getBoolPref(DEVTOOLS_F12_DISABLED_PREF, false)) {
     // If the toolbox is opening while F12 was disabled, the user might have
@@ -1468,7 +1465,7 @@ Toolbox.prototype = {
 
   _pingTelemetry() {
     Services.prefs.setBoolPref("devtools.everOpened", true);
-    this.telemetry.toolOpened("toolbox", this.sessionId, this);
+    this.telemetry.toolOpened("toolbox", this);
 
     this.telemetry
       .getHistogramById(HOST_HISTOGRAM)
@@ -1487,7 +1484,6 @@ Toolbox.prototype = {
       "shortcut",
       "splitconsole",
       "width",
-      "session_id",
     ]);
     this.telemetry.addEventProperty(
       browserWin,
@@ -2791,7 +2787,7 @@ Toolbox.prototype = {
       this.additionalToolDefinitions.get(id)
     ) {
       if (this.currentToolId) {
-        this.telemetry.toolClosed(this.currentToolId, this.sessionId, this);
+        this.telemetry.toolClosed(this.currentToolId, this);
       }
 
       this._pingTelemetrySelectTool(id, reason);
@@ -2825,14 +2821,7 @@ Toolbox.prototype = {
     const panelName = this.getTelemetryPanelNameOrOther(id);
     const prevPanelName = this.getTelemetryPanelNameOrOther(this.currentToolId);
     const cold = !this.getPanel(id);
-    const pending = [
-      "host",
-      "width",
-      "start_state",
-      "panel_name",
-      "cold",
-      "session_id",
-    ];
+    const pending = ["host", "width", "start_state", "panel_name", "cold"];
 
     // On first load this.currentToolId === undefined so we need to skip sending
     // a devtools.main.exit telemetry event.
@@ -2891,7 +2880,7 @@ Toolbox.prototype = {
       );
     }
 
-    this.telemetry.toolOpened(id, this.sessionId, this);
+    this.telemetry.toolOpened(id, this);
   },
 
   /**
@@ -4023,7 +4012,7 @@ Toolbox.prototype = {
 
     // We normally handle toolClosed from selectTool() but in the event of the
     // toolbox closing we need to handle it here instead.
-    this.telemetry.toolClosed(this.currentToolId, this.sessionId, this);
+    this.telemetry.toolClosed(this.currentToolId, this);
 
     this._lastFocusedElement = null;
     this._pausedTargets = null;
@@ -4121,7 +4110,7 @@ Toolbox.prototype = {
     const width = Math.ceil(win.outerWidth / 50) * 50;
     const prevPanelName = this.getTelemetryPanelNameOrOther(this.currentToolId);
 
-    this.telemetry.toolClosed("toolbox", this.sessionId, this);
+    this.telemetry.toolClosed("toolbox", this);
     this.telemetry.recordEvent("exit", prevPanelName, null, {
       host,
       width,

devtools/client/inspector/shared/highlighters-overlay.js

@@ -270,11 +270,7 @@ class HighlightersOverlay {
       case TYPES.GRID:
         const toolID = TELEMETRY_TOOL_IDS[type];
         if (toolID) {
-          this.telemetry.toolOpened(
-            toolID,
-            this.inspector.toolbox.sessionId,
-            this
-          );
+          this.telemetry.toolOpened(toolID, this);
         }
 
         const scalar = TELEMETRY_SCALARS[type]?.[options?.trigger];
@@ -393,11 +389,7 @@ class HighlightersOverlay {
         };
 
         if (toolID && conditions[type].call(this)) {
-          this.telemetry.toolClosed(
-            toolID,
-            this.inspector.toolbox.sessionId,
-            this
-          );
+          this.telemetry.toolClosed(toolID, this);
         }
 
         break;

devtools/client/inspector/toolsidebar.js

@@ -230,22 +230,19 @@ ToolSidebar.prototype = {
       return;
     }
 
-    const sessionId = this._toolPanel._toolbox.sessionId;
-
     currentToolId = this.getTelemetryPanelNameOrOther(currentToolId);
 
     if (previousToolId) {
       previousToolId = this.getTelemetryPanelNameOrOther(previousToolId);
-      this._telemetry.toolClosed(previousToolId, sessionId, this);
+      this._telemetry.toolClosed(previousToolId, this);
 
       this._telemetry.recordEvent("sidepanel_changed", "inspector", null, {
         oldpanel: previousToolId,
         newpanel: currentToolId,
         os: this._telemetry.osNameAndVersion,
-        session_id: sessionId,
       });
     }
-    this._telemetry.toolOpened(currentToolId, sessionId, this);
+    this._telemetry.toolOpened(currentToolId, this);
   },
 
   /**
@@ -313,8 +310,7 @@ ToolSidebar.prototype = {
     this.emit("destroy");
 
     if (this._currentTool && this._telemetry) {
-      const sessionId = this._toolPanel._toolbox.sessionId;
-      this._telemetry.toolClosed(this._currentTool, sessionId, this);
+      this._telemetry.toolClosed(this._currentTool, this);
     }
 
     this._toolPanel.emit("sidebar-destroyed", this);

devtools/client/responsive/index.js

@@ -58,9 +58,7 @@ const bootstrap = {
   store: null,
 
   async init() {
-    // responsive is not connected with a toolbox so we pass -1 as the
-    // toolbox session id.
-    this.telemetry.toolOpened("responsive", -1, this);
+    this.telemetry.toolOpened("responsive", this);
 
     const store = (this.store = Store());
     const provider = createElement(Provider, { store }, App());
@@ -77,9 +75,7 @@ const bootstrap = {
 
     this.store = null;
 
-    // responsive is not connected with a toolbox so we pass -1 as the
-    // toolbox session id.
-    this.telemetry.toolClosed("responsive", -1, this);
+    this.telemetry.toolClosed("responsive", this);
     this.telemetry = null;
   },
 

devtools/client/shared/telemetry.js

@@ -25,8 +25,22 @@ const CATEGORY = "devtools.main";
 const PENDING_EVENT_PROPERTIES = new WeakMapMap();
 const PENDING_EVENTS = new WeakMapMap();
 
+/**
+ * Instantiate a new Telemetry helper class.
+ *
+ * @param {Object} options [optional]
+ * @param {Boolean} options.useSessionId [optional]
+ *        If true, this instance will automatically generate a unique "sessionId"
+ *        and use it to aggregate all records against this unique session.
+ *        This helps aggregate all data coming from a single toolbox instance for ex.
+ */
 class Telemetry {
-  constructor() {
+  constructor({ useSessionId = false } = {}) {
+    // Note that native telemetry APIs expect a string
+    this.sessionId = String(
+      useSessionId ? parseInt(this.msSinceProcessStart(), 10) : -1
+    );
+
     // Bind pretty much all functions so that callers do not need to.
     this.msSystemNow = this.msSystemNow.bind(this);
     this.getHistogramById = this.getHistogramById.bind(this);
@@ -611,6 +625,15 @@ class Telemetry {
         extra[name] = val;
       }
     }
+    // Automatically flag the record with the session ID
+    // if the current Telemetry instance relates to a toolbox
+    // so that data can be aggregated per toolbox instance.
+    // Note that we also aggregate data per about:debugging instance.
+    if (!extra) {
+      extra = {};
+    }
+    extra.session_id = this.sessionId;
+
     Services.telemetry.recordEvent(CATEGORY, method, object, value, extra);
   }
 
@@ -619,9 +642,6 @@ class Telemetry {
    *
    * @param {String} id
    *        The ID of the tool opened.
-   * @param {String} sessionId
-   *        Toolbox session id used when we need to ensure a tool really has a
-   *        timer before calculating a delta.
    * @param {Object} obj
    *        The telemetry event or ping is associated with this object, meaning
    *        that multiple events or pings for the same histogram may be run
@@ -631,11 +651,7 @@ class Telemetry {
    *       one of those probes being a counter and the other a timer. If you
    *       only have one probe you should be using another method.
    */
-  toolOpened(id, sessionId, obj) {
-    if (typeof sessionId === "undefined") {
-      throw new Error(`toolOpened called without a sessionId parameter.`);
-    }
-
+  toolOpened(id, obj) {
     const charts = getChartsFromToolId(id);
 
     if (!charts) {
@@ -646,7 +662,6 @@ class Telemetry {
       this.preparePendingEvent(obj, "tool_timer", id, null, [
         "os",
         "time_open",
-        "session_id",
       ]);
       this.addEventProperty(
         obj,
@@ -673,8 +688,6 @@ class Telemetry {
    *
    * @param {String} id
    *        The ID of the tool opened.
-   * @param {String} sessionId
-   *        Toolbox session id.
    * @param {Object} obj
    *        The telemetry event or ping is associated with this object, meaning
    *        that multiple events or pings for the same histogram may be run
@@ -684,11 +697,7 @@ class Telemetry {
    *       one of those probes being a counter and the other a timer. If you
    *       only have one probe you should be using another method.
    */
-  toolClosed(id, sessionId, obj) {
-    if (typeof sessionId === "undefined") {
-      throw new Error(`toolClosed called without a sessionId parameter.`);
-    }
-
+  toolClosed(id, obj) {
     const charts = getChartsFromToolId(id);
 
     if (!charts) {
@@ -703,7 +712,6 @@ class Telemetry {
       this.addEventProperties(obj, "tool_timer", id, null, {
         time_open: time,
         os: this.osNameAndVersion,
-        session_id: sessionId,
       });
     }
 

devtools/client/webconsole/browser-console.js

@@ -65,9 +65,7 @@ class BrowserConsole extends WebConsole {
       // Only add the shutdown observer if we've opened a Browser Console window.
       ShutdownObserver.init();
 
-      // browserconsole is not connected with a toolbox so we pass -1 as the
-      // toolbox session id.
-      this.#telemetry.toolOpened("browserconsole", -1, this);
+      this.#telemetry.toolOpened("browserconsole", this);
 
       await super.init(false);
 
@@ -91,9 +89,7 @@ class BrowserConsole extends WebConsole {
     }
 
     this.#bcDestroyer = (async () => {
-      // browserconsole is not connected with a toolbox so we pass -1 as the
-      // toolbox session id.
-      this.#telemetry.toolClosed("browserconsole", -1, this);
+      this.#telemetry.toolClosed("browserconsole", this);
 
       this.commands.targetCommand.destroy();
       await super.destroy();

devtools/docs/contributor/frontend/telemetry.md

@@ -152,19 +152,22 @@ let Telemetry = require("devtools/client/shared/telemetry");
 Create a telemetry instance on the tool constructor:
 
 ```js
-this._telemetry = new Telemetry();
+this._telemetry = new Telemetry({ useSessionId: true });
 ```
+`useSessionId` allows to aggregate all records behind a randomly unique "session_id"
+extra attribute. For example, this helps aggregate all data recorded for one precise
+toolbox instance.
 
 And use the instance to report e.g. tool opening...
 
 ```js
-this._telemetry.toolOpened("mytoolname", sessionId, this);
+this._telemetry.toolOpened("mytoolname", this);
 ```
 
 ... or closing:
 
 ```js
-this._telemetry.toolClosed("mytoolname", sessionId, this);
+this._telemetry.toolClosed("mytoolname", this);
 ```
 
 Note that `mytoolname` is the id we declared in the `telemetry.js` module.
@@ -201,7 +204,7 @@ this._telemetry = new Telemetry();
 And use the instance to report e.g. tool opening...
 
 ```js
-this._telemetry.toolOpened("mytoolname", sessionId, this);
+this._telemetry.toolOpened("mytoolname", this);
 ```
 
 Notes:
@@ -248,7 +251,6 @@ this._telemetry.recordEvent("open", "tools", null, {
   "host": "bottom",
   "splitconsole": false,
   "width": 1024,
-  "session_id": this.toolbox.sessionId
 });
 
 // If your "extra" properties are in different code paths you will need to
@@ -315,7 +317,7 @@ To see these warnings, you need to have the `browser.dom.window.dump.enabled` br
 Then, try doing things that trigger telemetry calls (e.g. opening a tool). Imagine we had a typo when reporting the tool was opened:
 
 ```js
-this._telemetry.toolOpened('mytoolnmae', sessionId, this);
+this._telemetry.toolOpened('mytoolnmae', this);
                                   ^^^^ typo, should be *mytoolname*
 ```