Record setting access events in the frontend

We aim at getting a distribution of the settings values across sessions and users, so we log only the first "get" but all "sets".

Bug: 400345550
Change-Id: Icbd9479a15c5bed779f41c9b831551aeaa56ca48
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6317766
Commit-Queue: Danil Somsikov <[email protected]>
Auto-Submit: Danil Somsikov <[email protected]>
Reviewed-by: Simon Zünd <[email protected]>

Author: danilsomsikov

front_end/core/common/Settings.test.ts

@@ -619,3 +619,45 @@ describe('updateVersionFrom37To38', () => {
     assert.isFalse(onboardingFinished.get());
   });
 });
+
+describe('access logging', () => {
+  let settings: Common.Settings.Settings;
+  let logSettingAccess!: sinon.SinonSpy;
+
+  beforeEach(() => {
+    const mockStore = new MockStore();
+    const syncedStorage = new Common.Settings.SettingsStorage({}, mockStore);
+    const globalStorage = new Common.Settings.SettingsStorage({}, mockStore);
+    const localStorage = new Common.Settings.SettingsStorage({}, mockStore);
+    logSettingAccess = sinon.spy();
+    settings = Common.Settings.Settings.instance({
+      forceNew: true,
+      syncedStorage,
+      globalStorage,
+      localStorage,
+      logSettingAccess,
+    });
+  });
+
+  it('logs access on the first read', async () => {
+    const setting = settings.createSetting('test-setting', false);
+    assert.isFalse(logSettingAccess.called);
+
+    setting.get();
+    assert.isTrue(logSettingAccess.calledOnceWith('test-setting', false));
+
+    setting.get();
+    assert.isTrue(logSettingAccess.calledOnce);
+  });
+
+  it('logs access on the every write', async () => {
+    const setting = settings.createSetting('test-setting', false);
+
+    setting.set(true);
+    assert.isTrue(logSettingAccess.calledOnceWith('test-setting', true));
+
+    setting.set(false);
+    assert.isTrue(logSettingAccess.calledTwice);
+    assert.deepEqual(logSettingAccess.secondCall.args, ['test-setting', false]);
+  });
+});

front_end/core/common/Settings.ts

@@ -58,11 +58,13 @@ export class Settings {
   #eventSupport: ObjectWrapper<GenericEvents>;
   #registry: Map<string, Setting<unknown>>;
   readonly moduleSettings: Map<string, Setting<unknown>>;
+  #logSettingAccess?: (name: string, value: number|string|boolean) => Promise<void>;
 
   private constructor(
       readonly syncedStorage: SettingsStorage,
       readonly globalStorage: SettingsStorage,
       readonly localStorage: SettingsStorage,
+      logSettingAccess?: (name: string, value: number|string|boolean) => Promise<void>,
   ) {
     this.#sessionStorage = new SettingsStorage({});
 
@@ -73,6 +75,7 @@ export class Settings {
     this.#eventSupport = new ObjectWrapper<GenericEvents>();
     this.#registry = new Map();
     this.moduleSettings = new Map();
+    this.#logSettingAccess = logSettingAccess;
 
     for (const registration of this.getRegisteredSettings()) {
       const {settingName, defaultValue, storageType} = registration;
@@ -107,14 +110,15 @@ export class Settings {
     syncedStorage: SettingsStorage|null,
     globalStorage: SettingsStorage|null,
     localStorage: SettingsStorage|null,
+    logSettingAccess?: (name: string, value: number|string|boolean) => Promise<void>,
   } = {forceNew: null, syncedStorage: null, globalStorage: null, localStorage: null}): Settings {
-    const {forceNew, syncedStorage, globalStorage, localStorage} = opts;
+    const {forceNew, syncedStorage, globalStorage, localStorage, logSettingAccess} = opts;
     if (!settingsInstance || forceNew) {
       if (!syncedStorage || !globalStorage || !localStorage) {
         throw new Error(`Unable to create settings: global and local storage must be provided: ${new Error().stack}`);
       }
 
-      settingsInstance = new Settings(syncedStorage, globalStorage, localStorage);
+      settingsInstance = new Settings(syncedStorage, globalStorage, localStorage, logSettingAccess);
     }
 
     return settingsInstance;
@@ -192,7 +196,7 @@ export class Settings {
     const storage = this.storageFromType(storageType);
     let setting = (this.#registry.get(key) as Setting<T>);
     if (!setting) {
-      setting = new Setting(key, defaultValue, this.#eventSupport, storage);
+      setting = new Setting(key, defaultValue, this.#eventSupport, storage, this.#logSettingAccess);
       this.#registry.set(key, setting);
     }
     return setting;
@@ -206,7 +210,10 @@ export class Settings {
       RegExpSetting {
     if (!this.#registry.get(key)) {
       this.#registry.set(
-          key, new RegExpSetting(key, defaultValue, this.#eventSupport, this.storageFromType(storageType), regexFlags));
+          key,
+          new RegExpSetting(
+              key, defaultValue, this.#eventSupport, this.storageFromType(storageType), regexFlags,
+              this.#logSettingAccess));
     }
     return this.#registry.get(key) as RegExpSetting;
   }
@@ -368,11 +375,15 @@ export class Setting<V> {
   #hadUserAction?: boolean;
   #disabled?: boolean;
   #deprecation: Deprecation|null = null;
+  #loggedInitialAccess = false;
+  #logSettingAccess?: (name: string, value: number|string|boolean) => Promise<void>;
 
   constructor(
       readonly name: string, readonly defaultValue: V, private readonly eventSupport: ObjectWrapper<GenericEvents>,
-      readonly storage: SettingsStorage) {
+      readonly storage: SettingsStorage,
+      logSettingAccess?: (name: string, value: number|string|boolean) => Promise<void>) {
     storage.register(this.name);
+    this.#logSettingAccess = logSettingAccess;
   }
 
   setSerializer(serializer: Serializer<unknown, V>): void {
@@ -438,12 +449,30 @@ export class Setting<V> {
     this.eventSupport.dispatchEventToListeners(this.name);
   }
 
+  #maybeLogAccess(value: V): void {
+    const valueToLog = typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean' ?
+        value :
+        this.#serializer?.stringify(value);
+    if (valueToLog !== undefined && this.#logSettingAccess) {
+      void this.#logSettingAccess(this.name, valueToLog);
+    }
+  }
+
+  #maybeLogInitialAccess(value: V): void {
+    if (!this.#loggedInitialAccess) {
+      this.#maybeLogAccess(value);
+      this.#loggedInitialAccess = true;
+    }
+  }
+
   get(): V {
     if (this.#requiresUserAction && !this.#hadUserAction) {
+      this.#maybeLogInitialAccess(this.defaultValue);
       return this.defaultValue;
     }
 
     if (typeof this.#value !== 'undefined') {
+      this.#maybeLogInitialAccess(this.#value);
       return this.#value;
     }
 
@@ -455,6 +484,7 @@ export class Setting<V> {
         this.storage.remove(this.name);
       }
     }
+    this.#maybeLogInitialAccess(this.#value);
     return this.#value;
   }
 
@@ -485,10 +515,12 @@ export class Setting<V> {
       this.eventSupport.dispatchEventToListeners(this.name, this.#value);
     }
 
+    this.#maybeLogInitialAccess(this.#value);
     return this.#value;
   }
 
   set(value: V): void {
+    this.#maybeLogAccess(value);
     this.#hadUserAction = true;
     this.#value = value;
     try {
@@ -600,8 +632,8 @@ export class RegExpSetting extends Setting<any> {
 
   constructor(
       name: string, defaultValue: string, eventSupport: ObjectWrapper<GenericEvents>, storage: SettingsStorage,
-      regexFlags?: string) {
-    super(name, defaultValue ? [{pattern: defaultValue}] : [], eventSupport, storage);
+      regexFlags?: string, logSettingAccess?: (name: string, value: number|string|boolean) => Promise<void>) {
+    super(name, defaultValue ? [{pattern: defaultValue}] : [], eventSupport, storage, logSettingAccess);
     this.#regexFlags = regexFlags;
   }
 

front_end/core/host/InspectorFrontendHost.ts

@@ -55,6 +55,7 @@ import {
   type KeyDownEvent,
   type LoadNetworkResourceResult,
   type ResizeEvent,
+  type SettingAccessEvent,
   type ShowSurveyResult,
   type SyncInformation,
 } from './InspectorFrontendHostAPI.js';
@@ -549,6 +550,8 @@ export class InspectorFrontendHostStub implements InspectorFrontendHostAPI {
   }
   recordKeyDown(event: KeyDownEvent): void {
   }
+  recordSettingAccess(event: SettingAccessEvent): void {
+  }
 }
 
 // @ts-expect-error Global injected by devtools_compatibility.js

front_end/core/host/InspectorFrontendHostAPI.ts

@@ -199,6 +199,12 @@ export interface KeyDownEvent {
   context?: number;
 }
 
+export interface SettingAccessEvent {
+  name: string;
+  numericValue?: number;
+  stringValue?: string;
+}
+
 // While `EventDescriptors` are used to dynamically dispatch host binding events,
 // the `EventTypes` "type map" is used for type-checking said events by TypeScript.
 // `EventTypes` is not used at runtime.
@@ -384,6 +390,7 @@ export interface InspectorFrontendHostAPI {
   recordDrag(event: DragEvent): void;
   recordChange(event: ChangeEvent): void;
   recordKeyDown(event: KeyDownEvent): void;
+  recordSettingAccess(event: SettingAccessEvent): void;
 }
 
 export interface AcceleratorDescriptor {

front_end/entrypoints/main/MainImpl.ts

@@ -273,7 +273,8 @@ export class MainImpl {
     // setting can't change storage buckets during a single DevTools session.
     const syncedStorage = new Common.Settings.SettingsStorage(prefs, hostSyncedStorage, storagePrefix);
     const globalStorage = new Common.Settings.SettingsStorage(prefs, hostUnsyncedStorage, storagePrefix);
-    Common.Settings.Settings.instance({forceNew: true, syncedStorage, globalStorage, localStorage});
+    Common.Settings.Settings.instance(
+        {forceNew: true, syncedStorage, globalStorage, localStorage, logSettingAccess: VisualLogging.logSettingAccess});
 
     if (!Host.InspectorFrontendHost.isUnderTest()) {
       new Common.Settings.VersionController().updateVersion();

front_end/ui/visual_logging/Debugging.ts

@@ -126,7 +126,7 @@ function processElementForDebugging(element: HTMLElement, loggingState: LoggingS
   }
 }
 
-type EventType = 'Click'|'Drag'|'Hover'|'Change'|'KeyDown'|'Resize';
+type EventType = 'Click'|'Drag'|'Hover'|'Change'|'KeyDown'|'Resize'|'SettingAccess';
 export function processEventForDebugging(
     event: EventType, state: LoggingState|null, extraInfo?: EventAttributes): void {
   const format = localStorage.getItem('veDebugLoggingEnabled');
@@ -163,7 +163,9 @@ export function processEventForIntuitiveDebugging(
 
 export function processEventForTestDebugging(
     event: EventType, state: LoggingState|null, _extraInfo?: EventAttributes): void {
-  lastImpressionLogEntry = null;
+  if (event !== 'SettingAccess') {
+    lastImpressionLogEntry = null;
+  }
   maybeLogDebugEvent(
       {interaction: `${event}: ${veTestKeys.get(state?.veid || 0) || (state?.veid ? '<UNKNOWN>' : '')}`});
   checkPendingEventExpectation();
@@ -194,6 +196,9 @@ export interface EventAttributes {
   height?: number;
   mouseButton?: number;
   doubleClick?: boolean;
+  name?: string;
+  numericValue?: number;
+  stringValue?: string;
 }
 
 interface VisualElementAttributes {

front_end/ui/visual_logging/LoggingEvents.test.ts

@@ -242,4 +242,19 @@ describe('LoggingEvents', () => {
     VisualLogging.LoggingEvents.logResize(element, new DOMRect(0, 0, 100, 50));
     assert.deepEqual(recordResize.firstCall.firstArg, {veid, width: 100, height: 50});
   });
+
+  it('calls UI binding to log a setting access event', async () => {
+    const recordSettingAccess = sinon.stub(
+        Host.InspectorFrontendHost.InspectorFrontendHostInstance,
+        'recordSettingAccess',
+    );
+    await VisualLogging.LoggingEvents.logSettingAccess('test-setting', 'test-value');
+    assert.deepEqual(
+        recordSettingAccess.lastCall.firstArg,
+        {name: 'test-setting', numericValue: undefined, stringValue: 'test-value'});
+
+    await VisualLogging.LoggingEvents.logSettingAccess('test-setting', 123);
+    assert.deepEqual(
+        recordSettingAccess.lastCall.firstArg, {name: 'test-setting', numericValue: 123, stringValue: undefined});
+  });
 });

front_end/ui/visual_logging/LoggingEvents.ts

@@ -169,3 +169,16 @@ export async function contextAsNumber(context: string|undefined): Promise<number
   const digest = await crypto.subtle.digest('SHA-1', data);
   return new DataView(digest).getInt32(0, true);
 }
+
+export async function logSettingAccess(name: string, value: number|string|boolean): Promise<void> {
+  let numericValue: number|undefined = undefined;
+  let stringValue: string|undefined = undefined;
+  if (typeof value === 'string') {
+    stringValue = value;
+  } else if (typeof value === 'number' || typeof value === 'boolean') {
+    numericValue = Number(value);
+  }
+  const settingAccessEvent: Host.InspectorFrontendHostAPI.SettingAccessEvent = {name, numericValue, stringValue};
+  Host.InspectorFrontendHost.InspectorFrontendHostInstance.recordSettingAccess(settingAccessEvent);
+  processEventForDebugging('SettingAccess', null, {name, numericValue, stringValue});
+}

front_end/ui/visual_logging/visual_logging.ts

@@ -11,7 +11,7 @@ import * as NonDomState from './NonDomState.js';
 export type Loggable = LoggableModule.Loggable;
 export {DebugLoggingFormat, setVeDebuggingEnabled, setVeDebugLoggingEnabled} from './Debugging.js';
 export {addDocument, startLogging, stopLogging} from './LoggingDriver.js';
-export {logImpressions} from './LoggingEvents.js';
+export {logImpressions, logSettingAccess} from './LoggingEvents.js';
 export const logClick = (loggable: Loggable, event: Event, options: {doubleClick?: boolean} = {}): void =>
     LoggingEvents.logClick(LoggingDriver.clickLogThrottler)(loggable, event, options);