diff --git a/devtools/client/aboutdebugging/src/middleware/event-recording.js b/devtools/client/aboutdebugging/src/middleware/event-recording.js index 71dcff4b39cd..f926100b8bb2 100644 --- a/devtools/client/aboutdebugging/src/middleware/event-recording.js +++ b/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); } } diff --git a/devtools/client/accessibility/picker.js b/devtools/client/accessibility/picker.js index 57f46909bf9d..2516d13a1074 100644 --- a/devtools/client/accessibility/picker.js +++ b/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"); } diff --git a/devtools/client/framework/browser-toolbox/Launcher.sys.mjs b/devtools/client/framework/browser-toolbox/Launcher.sys.mjs index 6b7dcf17c520..80cb70dbabb2 100644 --- a/devtools/client/framework/browser-toolbox/Launcher.sys.mjs +++ b/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); diff --git a/devtools/client/framework/test/browser_toolbox_telemetry_open_event.js b/devtools/client/framework/test/browser_toolbox_telemetry_open_event.js index b1dacbf6cdd3..173902258a21 100644 --- a/devtools/client/framework/test/browser_toolbox_telemetry_open_event.js +++ b/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"); diff --git a/devtools/client/framework/toolbox.js b/devtools/client/framework/toolbox.js index 668e4394ee23..b5b782cce1e6 100644 --- a/devtools/client/framework/toolbox.js +++ b/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, diff --git a/devtools/client/inspector/shared/highlighters-overlay.js b/devtools/client/inspector/shared/highlighters-overlay.js index 2d84d146f727..8290e6925555 100644 --- a/devtools/client/inspector/shared/highlighters-overlay.js +++ b/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; diff --git a/devtools/client/inspector/toolsidebar.js b/devtools/client/inspector/toolsidebar.js index 8b3e4f75b60e..cff5eb96fa17 100644 --- a/devtools/client/inspector/toolsidebar.js +++ b/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); diff --git a/devtools/client/responsive/index.js b/devtools/client/responsive/index.js index c55a4262ef51..5df50fa665b8 100644 --- a/devtools/client/responsive/index.js +++ b/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; }, diff --git a/devtools/client/shared/telemetry.js b/devtools/client/shared/telemetry.js index a0bf2b37d137..e88221908a49 100644 --- a/devtools/client/shared/telemetry.js +++ b/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, }); } diff --git a/devtools/client/webconsole/browser-console.js b/devtools/client/webconsole/browser-console.js index 0871d6b94981..5d10b19c2148 100644 --- a/devtools/client/webconsole/browser-console.js +++ b/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(); diff --git a/devtools/docs/contributor/frontend/telemetry.md b/devtools/docs/contributor/frontend/telemetry.md index 16f1f1039422..26193a41843b 100644 --- a/devtools/docs/contributor/frontend/telemetry.md +++ b/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* ```