From 6a72d1a519708e12a9444d0d847f97833f089af0 Mon Sep 17 00:00:00 2001 From: Sandeep Somavarapu Date: Sat, 8 Feb 2020 20:58:43 +0100 Subject: [PATCH] - move telemetry logging to abstract synchroniser - improve logging --- .../common/abstractSynchronizer.ts | 18 +++++++++++++++++- .../userDataSync/common/extensionsSync.ts | 10 ++++++---- .../userDataSync/common/globalStateSync.ts | 9 +++++---- .../userDataSync/common/keybindingsSync.ts | 10 +++++----- .../userDataSync/common/settingsSync.ts | 9 +++++---- .../userDataSync/common/userDataSyncService.ts | 14 -------------- 6 files changed, 38 insertions(+), 32 deletions(-) diff --git a/src/vs/platform/userDataSync/common/abstractSynchronizer.ts b/src/vs/platform/userDataSync/common/abstractSynchronizer.ts index 386ccb45299..6377afae63c 100644 --- a/src/vs/platform/userDataSync/common/abstractSynchronizer.ts +++ b/src/vs/platform/userDataSync/common/abstractSynchronizer.ts @@ -13,6 +13,11 @@ import { joinPath, dirname } from 'vs/base/common/resources'; import { toLocalISOString } from 'vs/base/common/date'; import { ThrottledDelayer } from 'vs/base/common/async'; import { Emitter, Event } from 'vs/base/common/event'; +import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; + +type SyncConflictsClassification = { + source?: { classification: 'SystemMetaData', purpose: 'FeatureInsight', isMeasurement: true }; +}; export abstract class AbstractSynchroniser extends Disposable { @@ -34,6 +39,7 @@ export abstract class AbstractSynchroniser extends Disposable { @IFileService protected readonly fileService: IFileService, @IEnvironmentService environmentService: IEnvironmentService, @IUserDataSyncStoreService protected readonly userDataSyncStoreService: IUserDataSyncStoreService, + @ITelemetryService private readonly telemetryService: ITelemetryService, ) { super(); this.syncFolder = joinPath(environmentService.userDataSyncHome, source); @@ -43,8 +49,17 @@ export abstract class AbstractSynchroniser extends Disposable { protected setStatus(status: SyncStatus): void { if (this._status !== status) { + const oldStatus = this._status; this._status = status; this._onDidChangStatus.fire(status); + if (status === SyncStatus.HasConflicts) { + // Log to telemetry when there is a sync conflict + this.telemetryService.publicLog2<{ source: string }, SyncConflictsClassification>('sync/conflictsDetected', { source: this.source }); + } + if (oldStatus === SyncStatus.HasConflicts && status === SyncStatus.Idle) { + // Log to telemetry when conflicts are resolved + this.telemetryService.publicLog2<{ source: string }, SyncConflictsClassification>('sync/conflictsResolved', { source: this.source }); + } } } @@ -112,8 +127,9 @@ export abstract class AbstractFileSynchroniser extends AbstractSynchroniser { @IFileService fileService: IFileService, @IEnvironmentService environmentService: IEnvironmentService, @IUserDataSyncStoreService userDataSyncStoreService: IUserDataSyncStoreService, + @ITelemetryService telemetryService: ITelemetryService, ) { - super(source, fileService, environmentService, userDataSyncStoreService); + super(source, fileService, environmentService, userDataSyncStoreService, telemetryService); this._register(this.fileService.watch(dirname(file))); this._register(Event.filter(this.fileService.onFileChanges, e => e.contains(file))(() => this._onDidChangeLocal.fire())); } diff --git a/src/vs/platform/userDataSync/common/extensionsSync.ts b/src/vs/platform/userDataSync/common/extensionsSync.ts index 3cf230cb66f..c732720838d 100644 --- a/src/vs/platform/userDataSync/common/extensionsSync.ts +++ b/src/vs/platform/userDataSync/common/extensionsSync.ts @@ -15,6 +15,7 @@ import { localize } from 'vs/nls'; import { merge } from 'vs/platform/userDataSync/common/extensionsMerge'; import { isNonEmptyArray } from 'vs/base/common/arrays'; import { AbstractSynchroniser } from 'vs/platform/userDataSync/common/abstractSynchronizer'; +import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; interface ISyncPreviewResult { readonly added: ISyncExtension[]; @@ -41,8 +42,9 @@ export class ExtensionsSynchroniser extends AbstractSynchroniser implements IUse @IUserDataSyncLogService private readonly logService: IUserDataSyncLogService, @IExtensionGalleryService private readonly extensionGalleryService: IExtensionGalleryService, @IConfigurationService private readonly configurationService: IConfigurationService, + @ITelemetryService telemetryService: ITelemetryService, ) { - super(SyncSource.Extensions, fileService, environmentService, userDataSyncStoreService); + super(SyncSource.Extensions, fileService, environmentService, userDataSyncStoreService, telemetryService); this._register( Event.debounce( Event.any( @@ -114,15 +116,15 @@ export class ExtensionsSynchroniser extends AbstractSynchroniser implements IUse async sync(): Promise { if (!this.configurationService.getValue('sync.enableExtensions')) { - this.logService.trace('Extensions: Skipping synchronizing extensions as it is disabled.'); + this.logService.info('Extensions: Skipping synchronizing extensions as it is disabled.'); return; } if (!this.extensionGalleryService.isEnabled()) { - this.logService.trace('Extensions: Skipping synchronizing extensions as gallery is disabled.'); + this.logService.info('Extensions: Skipping synchronizing extensions as gallery is disabled.'); return; } if (this.status !== SyncStatus.Idle) { - this.logService.trace('Extensions: Skipping synchronizing extensions as it is running already.'); + this.logService.info('Extensions: Skipping synchronizing extensions as it is running already.'); return; } diff --git a/src/vs/platform/userDataSync/common/globalStateSync.ts b/src/vs/platform/userDataSync/common/globalStateSync.ts index bd125de964a..e8b73b53b5d 100644 --- a/src/vs/platform/userDataSync/common/globalStateSync.ts +++ b/src/vs/platform/userDataSync/common/globalStateSync.ts @@ -15,6 +15,7 @@ import { merge } from 'vs/platform/userDataSync/common/globalStateMerge'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { parse } from 'vs/base/common/json'; import { AbstractSynchroniser } from 'vs/platform/userDataSync/common/abstractSynchronizer'; +import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; const argvProperties: string[] = ['locale']; @@ -33,8 +34,9 @@ export class GlobalStateSynchroniser extends AbstractSynchroniser implements IUs @IUserDataSyncLogService private readonly logService: IUserDataSyncLogService, @IEnvironmentService private readonly environmentService: IEnvironmentService, @IConfigurationService private readonly configurationService: IConfigurationService, + @ITelemetryService telemetryService: ITelemetryService, ) { - super(SyncSource.GlobalState, fileService, environmentService, userDataSyncStoreService); + super(SyncSource.GlobalState, fileService, environmentService, userDataSyncStoreService, telemetryService); this._register(this.fileService.watch(dirname(this.environmentService.argvResource))); this._register(Event.filter(this.fileService.onFileChanges, e => e.contains(this.environmentService.argvResource))(() => this._onDidChangeLocal.fire())); } @@ -98,12 +100,11 @@ export class GlobalStateSynchroniser extends AbstractSynchroniser implements IUs async sync(): Promise { if (!this.configurationService.getValue('sync.enableUIState')) { - this.logService.trace('UI State: Skipping synchronizing UI state as it is disabled.'); + this.logService.info('UI State: Skipping synchronizing UI state as it is disabled.'); return; } - if (this.status !== SyncStatus.Idle) { - this.logService.trace('UI State: Skipping synchronizing ui state as it is running already.'); + this.logService.info('UI State: Skipping synchronizing ui state as it is running already.'); return; } diff --git a/src/vs/platform/userDataSync/common/keybindingsSync.ts b/src/vs/platform/userDataSync/common/keybindingsSync.ts index 464f50fc04d..9f56513c80c 100644 --- a/src/vs/platform/userDataSync/common/keybindingsSync.ts +++ b/src/vs/platform/userDataSync/common/keybindingsSync.ts @@ -18,6 +18,7 @@ import { isUndefined } from 'vs/base/common/types'; import { FormattingOptions } from 'vs/base/common/jsonFormatter'; import { isNonEmptyArray } from 'vs/base/common/arrays'; import { AbstractFileSynchroniser } from 'vs/platform/userDataSync/common/abstractSynchronizer'; +import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; interface ISyncContent { mac?: string; @@ -46,8 +47,9 @@ export class KeybindingsSynchroniser extends AbstractFileSynchroniser implements @IFileService fileService: IFileService, @IEnvironmentService private readonly environmentService: IEnvironmentService, @IUserDataSyncUtilService private readonly userDataSyncUtilService: IUserDataSyncUtilService, + @ITelemetryService telemetryService: ITelemetryService, ) { - super(environmentService.keybindingsResource, SyncSource.Keybindings, fileService, environmentService, userDataSyncStoreService); + super(environmentService.keybindingsResource, SyncSource.Keybindings, fileService, environmentService, userDataSyncStoreService, telemetryService); } protected getRemoteDataResourceKey(): string { return 'keybindings'; } @@ -137,18 +139,16 @@ export class KeybindingsSynchroniser extends AbstractFileSynchroniser implements async sync(): Promise { if (!this.configurationService.getValue('sync.enableKeybindings')) { - this.logService.trace('Keybindings: Skipping synchronizing keybindings as it is disabled.'); + this.logService.info('Keybindings: Skipping synchronizing keybindings as it is disabled.'); return; } - if (this.status !== SyncStatus.Idle) { - this.logService.trace('Keybindings: Skipping synchronizing keybindings as it is running already.'); + this.logService.info('Keybindings: Skipping synchronizing keybindings as it is running already.'); return; } this.logService.trace('Keybindings: Started synchronizing keybindings...'); this.setStatus(SyncStatus.Syncing); - return this.doSync(); } diff --git a/src/vs/platform/userDataSync/common/settingsSync.ts b/src/vs/platform/userDataSync/common/settingsSync.ts index 9b223976b9a..17ca8153c80 100644 --- a/src/vs/platform/userDataSync/common/settingsSync.ts +++ b/src/vs/platform/userDataSync/common/settingsSync.ts @@ -20,6 +20,7 @@ import * as objects from 'vs/base/common/objects'; import { isEmptyObject, isUndefinedOrNull } from 'vs/base/common/types'; import { edit } from 'vs/platform/userDataSync/common/content'; import { AbstractFileSynchroniser } from 'vs/platform/userDataSync/common/abstractSynchronizer'; +import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; interface ISyncPreviewResult { readonly fileContent: IFileContent | null; @@ -50,8 +51,9 @@ export class SettingsSynchroniser extends AbstractFileSynchroniser implements IS @IUserDataSyncLogService private readonly logService: IUserDataSyncLogService, @IUserDataSyncUtilService private readonly userDataSyncUtilService: IUserDataSyncUtilService, @IConfigurationService private readonly configurationService: IConfigurationService, + @ITelemetryService telemetryService: ITelemetryService, ) { - super(environmentService.settingsResource, SyncSource.Settings, fileService, environmentService, userDataSyncStoreService); + super(environmentService.settingsResource, SyncSource.Settings, fileService, environmentService, userDataSyncStoreService, telemetryService); } protected getRemoteDataResourceKey(): string { return 'settings'; } @@ -165,12 +167,11 @@ export class SettingsSynchroniser extends AbstractFileSynchroniser implements IS async sync(): Promise { if (!this.configurationService.getValue('sync.enableSettings')) { - this.logService.trace('Settings: Skipping synchronizing settings as it is disabled.'); + this.logService.info('Settings: Skipping synchronizing settings as it is disabled.'); return; } - if (this.status !== SyncStatus.Idle) { - this.logService.trace('Settings: Skipping synchronizing settings as it is running already.'); + this.logService.info('Settings: Skipping synchronizing settings as it is running already.'); return; } diff --git a/src/vs/platform/userDataSync/common/userDataSyncService.ts b/src/vs/platform/userDataSync/common/userDataSyncService.ts index 10a4c4599c7..61e294a68cf 100644 --- a/src/vs/platform/userDataSync/common/userDataSyncService.ts +++ b/src/vs/platform/userDataSync/common/userDataSyncService.ts @@ -14,10 +14,6 @@ import { toErrorMessage } from 'vs/base/common/errorMessage'; import { localize } from 'vs/nls'; import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; -type SyncConflictsClassification = { - source?: { classification: 'SystemMetaData', purpose: 'FeatureInsight', isMeasurement: true }; -}; - type SyncErrorClassification = { source: { classification: 'SystemMetaData', purpose: 'FeatureInsight', isMeasurement: true }; }; @@ -259,18 +255,8 @@ export class UserDataSyncService extends Disposable implements IUserDataSyncServ private updateStatus(): void { const status = this.computeStatus(); if (this._status !== status) { - const oldStatus = this._status; - const oldConflictsSource = this._conflictsSource; this._conflictsSource = this.computeConflictsSource(); this._status = status; - if (status === SyncStatus.HasConflicts) { - // Log to telemetry when there is a sync conflict - this.telemetryService.publicLog2<{ source: string }, SyncConflictsClassification>('sync/conflictsDetected', { source: this._conflictsSource! }); - } - if (oldStatus === SyncStatus.HasConflicts && status === SyncStatus.Idle) { - // Log to telemetry when conflicts are resolved - this.telemetryService.publicLog2<{ source: string }, SyncConflictsClassification>('sync/conflictsResolved', { source: oldConflictsSource! }); - } this._onDidChangeStatus.fire(status); } } -- GitLab