From 242d3526cc51b756ad09ede25ccc428d3531d9c1 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 30 Jan 2024 10:27:40 +0100 Subject: [PATCH] send errors to telemetry --- .../src/managers/applicationManager.spec.ts | 26 +++++++++++++++++-- .../src/managers/applicationManager.ts | 9 +++++++ packages/backend/src/managers/playground.ts | 10 ++++++- packages/backend/src/studio-api-impl.spec.ts | 11 ++++++-- packages/backend/src/studio.ts | 2 +- 5 files changed, 52 insertions(+), 6 deletions(-) diff --git a/packages/backend/src/managers/applicationManager.spec.ts b/packages/backend/src/managers/applicationManager.spec.ts index 46084ae5d..9f9b40125 100644 --- a/packages/backend/src/managers/applicationManager.spec.ts +++ b/packages/backend/src/managers/applicationManager.spec.ts @@ -30,6 +30,7 @@ const mocks = vi.hoisted(() => { deleteContainerMock: vi.fn(), inspectContainerMock: vi.fn(), logUsageMock: vi.fn(), + logErrorMock: vi.fn(), }; }); vi.mock('../models/AIConfig', () => ({ @@ -155,6 +156,7 @@ describe('pullApplication', () => { } as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); doDownloadModelWrapperSpy = vi.spyOn(manager, 'doDownloadModelWrapper'); @@ -302,6 +304,7 @@ describe('doCheckout', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); await manager.doCheckout('repo', 'folder', taskUtils); @@ -332,6 +335,7 @@ describe('doCheckout', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); await manager.doCheckout('repo', 'folder', taskUtils); @@ -357,6 +361,7 @@ describe('getConfiguration', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); vi.spyOn(fs, 'existsSync').mockReturnValue(false); @@ -373,6 +378,7 @@ describe('getConfiguration', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); vi.spyOn(fs, 'existsSync').mockReturnValue(true); @@ -410,6 +416,7 @@ describe('downloadModel', () => { { isModelOnDisk: isModelOnDiskMock } as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const doDownloadModelWrapperMock = vi @@ -449,6 +456,7 @@ describe('downloadModel', () => { } as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); await manager.downloadModel( @@ -498,6 +506,7 @@ describe('filterContainers', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const containers = manager.filterContainers(aiConfig); @@ -536,6 +545,7 @@ describe('filterContainers', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const containers = manager.filterContainers(aiConfig); @@ -584,6 +594,7 @@ describe('filterContainers', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const containers = manager.filterContainers(aiConfig); @@ -602,6 +613,7 @@ describe('getRandomName', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const randomName = manager.getRandomName('base'); @@ -616,6 +628,7 @@ describe('getRandomName', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const randomName = manager.getRandomName(''); @@ -641,16 +654,18 @@ describe('buildImages', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); - test('setTaskState should be called with error if context does not exist', async () => { + test('setTaskState should be called with error and telemetry seent if context does not exist', async () => { vi.spyOn(fs, 'existsSync').mockReturnValue(false); mocks.listImagesMock.mockRejectedValue([]); await expect(manager.buildImages(containers, 'config', taskUtils)).rejects.toThrow( 'Context configured does not exist.', ); + expect(mocks.logErrorMock).toHaveBeenCalled(); }); - test('setTaskState should be called with error if buildImage executon fails', async () => { + test('setTaskState should be called with error and telemetry sent if buildImage executon fails', async () => { vi.spyOn(fs, 'existsSync').mockReturnValue(true); mocks.builImageMock.mockRejectedValue('error'); mocks.listImagesMock.mockRejectedValue([]); @@ -658,6 +673,7 @@ describe('buildImages', () => { 'Something went wrong while building the image: error', ); expect(setTaskStateMock).toBeCalledWith('container1', 'error'); + expect(mocks.logErrorMock).toHaveBeenCalled(); }); test('setTaskState should be called with error if unable to find the image after built', async () => { vi.spyOn(fs, 'existsSync').mockReturnValue(true); @@ -713,6 +729,7 @@ describe('createPod', async () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); test('throw an error if there is no sample image', async () => { @@ -770,6 +787,7 @@ describe('createApplicationPod', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const images = [imageInfo1, imageInfo2]; @@ -825,6 +843,7 @@ describe('doDownloadModelWrapper', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); test('returning model path if model has been downloaded', async () => { @@ -879,6 +898,7 @@ describe('restartContainerWhenModelServiceIsUp', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); test('restart container if endpoint is alive', async () => { @@ -896,6 +916,7 @@ describe('runApplication', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const pod: PodInfo = { @@ -953,6 +974,7 @@ describe('createAndAddContainersToPod', () => { {} as unknown as ModelsManager, { logUsage: mocks.logUsageMock, + logError: mocks.logErrorMock, } as unknown as TelemetryLogger, ); const pod: PodInfo = { diff --git a/packages/backend/src/managers/applicationManager.ts b/packages/backend/src/managers/applicationManager.ts index 0980bb6a4..e2d60222c 100644 --- a/packages/backend/src/managers/applicationManager.ts +++ b/packages/backend/src/managers/applicationManager.ts @@ -168,6 +168,7 @@ export class ApplicationManager { await timeout(5000); await this.restartContainerWhenModelServiceIsUp(engineId, modelServiceEndpoint, container).catch( (error: unknown) => { + this.telemetry.logError('recipe.pull', { message: 'error monitoring endpoint', error: error }); console.error('Error monitoring endpoint', error); }, ); @@ -185,6 +186,7 @@ export class ApplicationManager { state: 'error', name: 'Creating application', }); + this.telemetry.logError('recipe.pull', { message: 'error creating pod', error: e }); throw e; } @@ -204,6 +206,7 @@ export class ApplicationManager { state: 'error', name: 'Creating application', }); + this.telemetry.logError('recipe.pull', { message: 'error adding containers to pod', error: e }); throw e; } @@ -352,6 +355,7 @@ export class ApplicationManager { if (!fs.existsSync(context)) { console.error('The context provided does not exist.'); taskUtil.setTaskState(container.name, 'error'); + this.telemetry.logError('recipe.pull', { message: 'configured context does not exist' }); throw new Error('Context configured does not exist.'); } @@ -367,6 +371,7 @@ export class ApplicationManager { // todo: do something with the event if (event === 'error' || (event === 'finish' && data !== '')) { console.error('Something went wrong while building the image: ', data); + this.telemetry.logError('recipe.pull', { message: 'error building image' }); taskUtil.setTaskState(container.name, 'error'); } }, @@ -374,6 +379,7 @@ export class ApplicationManager { ) .catch((err: unknown) => { console.error('Something went wrong while building the image: ', err); + this.telemetry.logError('recipe.pull', { message: 'error building image', error: err }); taskUtil.setTaskState(container.name, 'error'); throw new Error(`Something went wrong while building the image: ${String(err)}`); }); @@ -432,6 +438,7 @@ export class ApplicationManager { } catch (e) { loadingConfiguration.state = 'error'; taskUtil.setTask(loadingConfiguration); + this.telemetry.logError('recipe.pull', { message: 'error loading configuration', error: e }); throw e; } @@ -445,6 +452,7 @@ export class ApplicationManager { // Mark as failure. loadingConfiguration.state = 'error'; taskUtil.setTask(loadingConfiguration); + this.telemetry.logError('recipe.pull', { message: 'no container available' }); throw new Error('No containers available.'); } @@ -485,6 +493,7 @@ export class ApplicationManager { 'model-pulling': model.id, }, }); + this.telemetry.logError('model.download', { message: 'error downloading model', error: e }); throw e; } } else { diff --git a/packages/backend/src/managers/playground.ts b/packages/backend/src/managers/playground.ts index 596d830fa..22704364e 100644 --- a/packages/backend/src/managers/playground.ts +++ b/packages/backend/src/managers/playground.ts @@ -22,6 +22,7 @@ import { type Webview, type ProviderContainerConnection, type ImageInfo, + type TelemetryLogger, } from '@podman-desktop/api'; import type { LocalModelInfo } from '@shared/src/models/ILocalModelInfo'; import type { ModelResponse } from '@shared/src/models/IModelResponse'; @@ -51,7 +52,10 @@ export class PlayGroundManager { private playgrounds: Map; private queries: Map; - constructor(private webview: Webview) { + constructor( + private webview: Webview, + private telemetry: TelemetryLogger, + ) { this.playgrounds = new Map(); this.queries = new Map(); } @@ -99,6 +103,7 @@ export class PlayGroundManager { const connection = findFirstProvider(); if (!connection) { await this.setPlaygroundStatus(modelId, 'error'); + this.telemetry.logError('playground.start', { message: 'unable to find an engine to start playground' }); throw new Error('Unable to find an engine to start playground'); } @@ -108,6 +113,7 @@ export class PlayGroundManager { image = await this.selectImage(connection, PLAYGROUND_IMAGE); if (!image) { await this.setPlaygroundStatus(modelId, 'error'); + this.telemetry.logError('playground.start', { message: 'unable to find playground image' }); throw new Error(`Unable to find ${PLAYGROUND_IMAGE} image`); } } @@ -168,6 +174,7 @@ export class PlayGroundManager { }) .catch(async (error: unknown) => { console.error(error); + this.telemetry.logError('playground.stop', { message: 'error stopping playground', error: error }); await this.setPlaygroundStatus(modelId, 'error'); }); } @@ -211,6 +218,7 @@ export class PlayGroundManager { const result = JSON.parse(resBody); const q = this.queries.get(query.id); if (!q) { + this.telemetry.logError('playground.ask', { message: 'error reading response' }); throw new Error('query not found in state'); } q.response = result as ModelResponse; diff --git a/packages/backend/src/studio-api-impl.spec.ts b/packages/backend/src/studio-api-impl.spec.ts index 7b7343e1f..70beec3b0 100644 --- a/packages/backend/src/studio-api-impl.spec.ts +++ b/packages/backend/src/studio-api-impl.spec.ts @@ -60,6 +60,7 @@ vi.mock('@podman-desktop/api', () => { const mocks = vi.hoisted(() => ({ withProgressMock: vi.fn(), + logUsageMock: vi.fn(), })); vi.mock('@podman-desktop/api', async () => { @@ -98,7 +99,9 @@ beforeEach(async () => { {} as unknown as PlayGroundManager, catalogManager, {} as unknown as ModelsManager, - {} as TelemetryLogger, + { + logUsage: mocks.logUsageMock, + } as unknown as TelemetryLogger, ); vi.resetAllMocks(); vi.mock('node:fs'); @@ -149,7 +152,7 @@ test('expect correct model is returned with valid id when the user catalog is va expect(model.url).toEqual('https://model1.example.com'); }); -test('expect pull application to call the withProgress api method', async () => { +test('expect pull application to call the withProgress api method and send telemetry', async () => { vi.spyOn(fs, 'existsSync').mockReturnValue(true); vi.spyOn(fs.promises, 'readFile').mockResolvedValue(JSON.stringify(userContent)); @@ -158,4 +161,8 @@ test('expect pull application to call the withProgress api method', async () => await catalogManager.loadCatalog(); await studioApiImpl.pullApplication('recipe 1'); expect(mocks.withProgressMock).toHaveBeenCalledOnce(); + expect(mocks.logUsageMock).toHaveBeenNthCalledWith(1, 'recipe.pull', { + 'recipe.id': 'recipe 1', + 'recipe.name': 'Recipe 1', + }); }); diff --git a/packages/backend/src/studio.ts b/packages/backend/src/studio.ts index be6bbed4f..e3b55021b 100644 --- a/packages/backend/src/studio.ts +++ b/packages/backend/src/studio.ts @@ -111,7 +111,7 @@ export class Studio { const gitManager = new GitManager(); const taskRegistry = new TaskRegistry(); const recipeStatusRegistry = new RecipeStatusRegistry(taskRegistry, this.#panel.webview); - this.playgroundManager = new PlayGroundManager(this.#panel.webview); + this.playgroundManager = new PlayGroundManager(this.#panel.webview, this.telemetry); // Create catalog manager, responsible for loading the catalog files and watching for changes this.catalogManager = new CatalogManager(appUserDirectory, this.#panel.webview); this.modelsManager = new ModelsManager(appUserDirectory, this.#panel.webview, this.catalogManager, this.telemetry);