From b9b3c1071e6dc89f458a8237d246750cc607c6b7 Mon Sep 17 00:00:00 2001 From: rajeshka Date: Tue, 14 Apr 2020 10:27:28 -0700 Subject: [PATCH] Added Notebook telemetry for livesite notebooks (#9948) * Added Notebook telemetry for livesite * added back the metadata retension code * Unified Telemetry services * fixed names * Fixed texts to use new NullAdsTelemetryService * Validate GUID before sending it * made GUID check a bit more robust Co-authored-by: Rajesh Kamath --- .../telemetry/common/telemetryKeys.ts | 9 ++-- .../notebook/browser/notebook.component.ts | 8 ++-- .../notebookEditorModel.test.ts | 3 +- .../notebookFindModel.test.ts | 3 +- .../electron-browser/notebookModel.test.ts | 45 +++++++++++++------ .../notebook/browser/models/notebookModel.ts | 29 ++++++++++-- 6 files changed, 70 insertions(+), 27 deletions(-) diff --git a/src/sql/platform/telemetry/common/telemetryKeys.ts b/src/sql/platform/telemetry/common/telemetryKeys.ts index a5bb22d629..061f72cd13 100644 --- a/src/sql/platform/telemetry/common/telemetryKeys.ts +++ b/src/sql/platform/telemetry/common/telemetryKeys.ts @@ -60,15 +60,14 @@ export const DeleteAgentAlert = 'DeleteAgentAlert'; export const DeleteAgentOperator = 'DeleteAgentOperator'; export const DeleteAgentProxy = 'DeleteAgentProxy'; -// Notebook Events: -export const NotebookMarkdownRendered = 'NotebookMarkdownRendered'; - export enum TelemetryView { Shell = 'Shell', ExtensionRecommendationDialog = 'ExtensionRecommendationDialog', - ResultsPanel = 'ResultsPanel' + ResultsPanel = 'ResultsPanel', + Notebook = 'Notebook' } export enum TelemetryAction { - Click = 'Click' + Click = 'Click', + Open = 'Open' } diff --git a/src/sql/workbench/contrib/notebook/browser/notebook.component.ts b/src/sql/workbench/contrib/notebook/browser/notebook.component.ts index d9742845e3..1874f9edfb 100644 --- a/src/sql/workbench/contrib/notebook/browser/notebook.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/notebook.component.ts @@ -45,7 +45,6 @@ import { toErrorMessage } from 'vs/base/common/errorMessage'; import { ILogService } from 'vs/platform/log/common/log'; import { ITextFileService } from 'vs/workbench/services/textfile/common/textfiles'; import { LabeledMenuItemActionItem, fillInActions } from 'vs/platform/actions/browser/menuEntryActionViewItem'; -import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; import { Button } from 'sql/base/browser/ui/button/button'; import { isUndefinedOrNull } from 'vs/base/common/types'; import { IBootstrapParams } from 'sql/workbench/services/bootstrap/common/bootstrapParams'; @@ -56,6 +55,7 @@ import { TextCellComponent } from 'sql/workbench/contrib/notebook/browser/cellVi import { NotebookInput } from 'sql/workbench/contrib/notebook/browser/models/notebookInput'; import { IColorTheme } from 'vs/platform/theme/common/themeService'; import { ICommandService } from 'vs/platform/commands/common/commands'; +import { IAdsTelemetryService } from 'sql/platform/telemetry/common/telemetry'; export const NOTEBOOK_SELECTOR: string = 'notebook-component'; @@ -104,8 +104,8 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe @Inject(ICapabilitiesService) private capabilitiesService: ICapabilitiesService, @Inject(ITextFileService) private textFileService: ITextFileService, @Inject(ILogService) private readonly logService: ILogService, - @Inject(ITelemetryService) private telemetryService: ITelemetryService, - @Inject(ICommandService) private commandService: ICommandService + @Inject(ICommandService) private commandService: ICommandService, + @Inject(IAdsTelemetryService) private adstelemetryService: IAdsTelemetryService ) { super(); this.updateProfile(); @@ -310,7 +310,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe layoutChanged: this._notebookParams.input.layoutChanged, capabilitiesService: this.capabilitiesService, editorLoadedTimestamp: this._notebookParams.input.editorOpenedTimestamp - }, this.profile, this.logService, this.notificationService, this.telemetryService); + }, this.profile, this.logService, this.notificationService, this.adstelemetryService); let trusted = await this.notebookService.isNotebookTrustCached(this._notebookParams.notebookUri, this.isDirty()); this._register(model.onError((errInfo: INotification) => this.handleModelError(errInfo))); this._register(model.contentChanged((change) => this.handleContentChanged(change))); diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookEditorModel.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookEditorModel.test.ts index dfbcda52b6..272193d711 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookEditorModel.test.ts +++ b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookEditorModel.test.ts @@ -39,6 +39,7 @@ import { assign } from 'vs/base/common/objects'; import { TestInstantiationService } from 'vs/platform/instantiation/test/common/instantiationServiceMock'; import { IStorageService } from 'vs/platform/storage/common/storage'; import { TestStorageService, TestTextResourcePropertiesService } from 'vs/workbench/test/common/workbenchTestServices'; +import { NullAdsTelemetryService } from 'sql/platform/telemetry/common/adsTelemetryService'; class ServiceAccessor { @@ -877,7 +878,7 @@ suite('Notebook Editor Model', function (): void { let options: INotebookModelOptions = assign({}, defaultModelOptions, >{ factory: mockModelFactory.object }); - notebookModel = new NotebookModel(options, undefined, logService, undefined, undefined); + notebookModel = new NotebookModel(options, undefined, logService, undefined, new NullAdsTelemetryService()); await notebookModel.loadContents(); } diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts index 9a6af3e1ac..902277d015 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts +++ b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts @@ -29,6 +29,7 @@ import { TestStorageService } from 'vs/workbench/test/common/workbenchTestServic import { NotebookEditorContentManager } from 'sql/workbench/contrib/notebook/browser/models/notebookInput'; import { NotebookRange } from 'sql/workbench/services/notebook/browser/notebookService'; import { NotebookMarkdownRenderer } from 'sql/workbench/contrib/notebook/browser/outputs/notebookMarkdown'; +import { NullAdsTelemetryService } from 'sql/platform/telemetry/common/adsTelemetryService'; let expectedNotebookContent: nb.INotebookContents = { cells: [{ @@ -364,7 +365,7 @@ suite('Notebook Find Model', function (): void { mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(contents)); defaultModelOptions.contentManager = mockContentManager.object; // Initialize the model - model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(); await model.requestModelLoad(); } diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts index 0e62f033a0..baeb50c634 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts +++ b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookModel.test.ts @@ -35,6 +35,7 @@ import { SessionManager } from 'sql/workbench/services/notebook/browser/sessionM import { mssqlProviderName } from 'sql/platform/connection/common/constants'; import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile'; import { uriPrefixes } from 'sql/platform/connection/common/utils'; +import { NullAdsTelemetryService } from 'sql/platform/telemetry/common/adsTelemetryService'; let expectedNotebookContent: nb.INotebookContents = { cells: [{ @@ -153,7 +154,7 @@ suite('notebook model', function (): void { mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(emptyNotebook)); defaultModelOptions.contentManager = mockContentManager.object; // When I initialize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(); // Then I expect to have 0 code cell as the contents @@ -169,7 +170,7 @@ suite('notebook model', function (): void { mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); defaultModelOptions.contentManager = mockContentManager.object; // When I initialize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(true); await model.requestModelLoad(); @@ -186,7 +187,7 @@ suite('notebook model', function (): void { // When I initalize the model // Then it should throw - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); assert.equal(model.inErrorState, false); await assert.rejects(async () => { await model.loadContents(); }); assert.equal(model.inErrorState, true); @@ -199,7 +200,7 @@ suite('notebook model', function (): void { defaultModelOptions.contentManager = mockContentManager.object; // When I initalize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(); // Then I expect all cells to be in the model @@ -227,7 +228,7 @@ suite('notebook model', function (): void { defaultModelOptions.providerId = 'jupyter'; // When I initalize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(); // I expect the default provider to be jupyter @@ -237,7 +238,7 @@ suite('notebook model', function (): void { defaultModelOptions.providerId = 'SQL'; // When I initalize the model - model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(); // I expect the default provider to be SQL @@ -262,7 +263,7 @@ suite('notebook model', function (): void { defaultModelOptions.contentManager = mockContentManager.object; // When I initalize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(); let activeCellChangeCount = 0; @@ -319,7 +320,7 @@ suite('notebook model', function (): void { defaultModelOptions.contentManager = mockContentManager.object; // When I initalize the model - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.loadContents(); // Count number of times onError event is fired @@ -376,7 +377,7 @@ suite('notebook model', function (): void { sessionReady.resolve(); let sessionFired = false; - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); model.onClientSessionReady((session) => sessionFired = true); await model.loadContents(); await model.requestModelLoad(); @@ -402,14 +403,14 @@ suite('notebook model', function (): void { }); test('Should sanitize kernel display name when IP is included', async function (): Promise { - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); let displayName = 'PySpark (1.1.1.1)'; let sanitizedDisplayName = model.sanitizeDisplayName(displayName); assert.equal(sanitizedDisplayName, 'PySpark'); }); test('Should sanitize kernel display name properly when IP is not included', async function (): Promise { - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); let displayName = 'PySpark'; let sanitizedDisplayName = model.sanitizeDisplayName(displayName); assert.equal(sanitizedDisplayName, 'PySpark'); @@ -420,7 +421,7 @@ suite('notebook model', function (): void { let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); defaultModelOptions.contentManager = mockContentManager.object; - let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, new NullAdsTelemetryService()); await model.requestModelLoad(); let actualChanged: NotebookContentChange; @@ -480,6 +481,24 @@ suite('notebook model', function (): void { }); + test('Should not delete custom metadata', async function (): Promise { + expectedNotebookContent.metadata['custom-string'] = 'some_string'; + expectedNotebookContent.metadata['custom-object'] = { prop1: 'value1', prop2: 'value2' }; + + // Given a notebook + let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); + mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); + defaultModelOptions.contentManager = mockContentManager.object; + // When I initialize the model + let model = new NotebookModel(defaultModelOptions, undefined, logService, undefined, undefined); + await model.loadContents(); + + let output = model.toJSON(); + assert(output.metadata['custom-string'] === 'some_string', 'Custom metadata was not preserved'); + assert(output.metadata['custom-object']['prop1'] === 'value1', 'Custom metadata for object was not preserved'); + assert(output.metadata['custom-object']['prop2'] === 'value2', 'Custom metadata for object was not preserved'); + }); + async function loadModelAndStartClientSession(): Promise { let mockContentManager = TypeMoq.Mock.ofType(NotebookEditorContentManager); mockContentManager.setup(c => c.loadContent()).returns(() => Promise.resolve(expectedNotebookContent)); @@ -500,7 +519,7 @@ suite('notebook model', function (): void { let options: INotebookModelOptions = assign({}, defaultModelOptions, >{ factory: mockModelFactory.object }); - let model = new NotebookModel(options, undefined, logService, undefined, undefined); + let model = new NotebookModel(options, undefined, logService, undefined, new NullAdsTelemetryService()); model.onClientSessionReady((session) => actualSession = session); await model.requestModelLoad(); diff --git a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts index ab4df12cc4..6a28c71087 100644 --- a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts +++ b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts @@ -24,11 +24,11 @@ import { ConnectionProfile } from 'sql/platform/connection/common/connectionProf import { uriPrefixes } from 'sql/platform/connection/common/utils'; import { keys } from 'vs/base/common/map'; import { ILogService } from 'vs/platform/log/common/log'; -import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; import { getErrorMessage } from 'vs/base/common/errors'; import { find, firstIndex } from 'vs/base/common/arrays'; import { startsWith } from 'vs/base/common/strings'; import { notebookConstants } from 'sql/workbench/services/notebook/browser/interfaces'; +import { IAdsTelemetryService } from 'sql/platform/telemetry/common/telemetry'; /* * Used to control whether a message in a dialog/wizard is displayed as an error, @@ -63,6 +63,7 @@ export class NotebookModel extends Disposable implements INotebookModel { private _cells: ICellModel[]; private _defaultLanguageInfo: nb.ILanguageInfo; private _tags: string[]; + private _existingMetadata = {}; private _language: string; private _onErrorEmitter = new Emitter(); private _savedKernelInfo: nb.IKernelInfo; @@ -87,7 +88,7 @@ export class NotebookModel extends Disposable implements INotebookModel { public connectionProfile: IConnectionProfile | undefined, @ILogService private readonly logService: ILogService, @INotificationService private readonly notificationService: INotificationService, - @ITelemetryService private readonly telemetryService: ITelemetryService + @IAdsTelemetryService private readonly adstelemetryService: IAdsTelemetryService ) { super(); if (!_notebookOptions || !_notebookOptions.notebookUri || !_notebookOptions.notebookManagers) { @@ -303,6 +304,25 @@ export class NotebookModel extends Disposable implements INotebookModel { if (contents) { this._defaultLanguageInfo = contents.metadata && contents.metadata.language_info; this._savedKernelInfo = this.getSavedKernelInfo(contents); + if (contents.metadata) { + //Telemetry of loading notebook + if (contents.metadata.azdata_notebook_guid && contents.metadata.azdata_notebook_guid.length === 36) { + //Verify if it is actual GUID and then send it to the telemetry + let regex = new RegExp('(\{){0,1}[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}(\}){0,1}'); + if (regex.test(contents.metadata.azdata_notebook_guid)) { + this.adstelemetryService.createActionEvent(TelemetryKeys.TelemetryView.Notebook, TelemetryKeys.TelemetryAction.Open) + .withAdditionalProperties({ azdata_notebook_guid: contents.metadata.azdata_notebook_guid }) + .send(); + } + } + Object.keys(contents.metadata).forEach(key => { + let expectedKeys = ['kernelspec', 'language_info', 'tags']; + // If custom metadata is defined, add to the _existingMetadata object + if (expectedKeys.indexOf(key) < 0) { + this._existingMetadata[key] = contents.metadata[key]; + } + }); + } if (contents.cells && contents.cells.length > 0) { this._cells = contents.cells.map(c => { let cellModel = factory.createCell(c, { notebook: this, isTrusted: isTrusted }); @@ -937,7 +957,7 @@ export class NotebookModel extends Disposable implements INotebookModel { if (this._textCellsLoading <= 0) { if (this._notebookOptions.editorLoadedTimestamp) { let markdownRenderingTime = Date.now() - this._notebookOptions.editorLoadedTimestamp; - this.telemetryService.publicLog(TelemetryKeys.NotebookMarkdownRendered, { markdownRenderingElapsedMs: markdownRenderingTime }); + this.adstelemetryService.sendMetricsEvent({ markdownRenderingElapsedMs: markdownRenderingTime }, TelemetryKeys.TelemetryView.Notebook); } } } @@ -971,6 +991,9 @@ export class NotebookModel extends Disposable implements INotebookModel { metadata.kernelspec = this._savedKernelInfo; metadata.language_info = this.languageInfo; metadata.tags = this._tags; + Object.keys(this._existingMetadata).forEach(key => { + metadata[key] = this._existingMetadata[key]; + }); return { metadata, nbformat_minor: this._nbformatMinor,