From 7f18de6a896bcad8abd62c46519343396ab1f9e5 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 28 Aug 2019 18:53:53 +0300 Subject: [PATCH 1/3] feat: enable to possibility to ensure analytics tracking is finished Introudce finishTracking method in `analyticsService` which will ensure all pending information is send to Google Analytics. This is required for some rare cases in which we want to be sure all the information has been sent. --- lib/common/declarations.d.ts | 7 +++++ .../detached-process-enums.d.ts | 7 ++++- .../analytics/analytics-broker-process.ts | 19 +++++++++++++ lib/services/analytics/analytics-service.ts | 28 +++++++++++++++++++ 4 files changed, 60 insertions(+), 1 deletion(-) diff --git a/lib/common/declarations.d.ts b/lib/common/declarations.d.ts index 3f9db042b0..93d14faedb 100644 --- a/lib/common/declarations.d.ts +++ b/lib/common/declarations.d.ts @@ -207,6 +207,12 @@ declare const enum TrackingTypes { * Defines that the broker process should get and track the data from preview app to Google Analytics */ PreviewAppData = "PreviewAppData", + + /** + * Defines that the broker process should send all the pending information to Analytics. + * After that the process should send information it has finished tracking and die gracefully. + */ + FinishTracking = "FinishTracking", } /** @@ -688,6 +694,7 @@ interface IAnalyticsService { setStatus(settingName: string, enabled: boolean): Promise; getStatusMessage(settingName: string, jsonFormat: boolean, readableSettingName: string): Promise; isEnabled(settingName: string): Promise; + finishTracking(): Promise; /** * Tracks the answer of question if user allows to be tracked. diff --git a/lib/detached-processes/detached-process-enums.d.ts b/lib/detached-processes/detached-process-enums.d.ts index c8e27edede..1c250b7cb9 100644 --- a/lib/detached-processes/detached-process-enums.d.ts +++ b/lib/detached-processes/detached-process-enums.d.ts @@ -5,7 +5,12 @@ declare const enum DetachedProcessMessages { /** * The detached process is initialized and is ready to receive information for tracking. */ - ProcessReadyToReceive = "ProcessReadyToReceive" + ProcessReadyToReceive = "ProcessReadyToReceive", + + /** + * The detached process finished its tasks and will now exit. + */ + ProcessFinishedTasks = "ProcessFinishedTasks" } /** diff --git a/lib/services/analytics/analytics-broker-process.ts b/lib/services/analytics/analytics-broker-process.ts index 5e7439af26..cdff02ceb3 100644 --- a/lib/services/analytics/analytics-broker-process.ts +++ b/lib/services/analytics/analytics-broker-process.ts @@ -29,6 +29,10 @@ const finishTracking = async (data?: ITrackingInformation) => { analyticsLoggingService.logData({ message: `analytics-broker-process finish tracking started` }); await trackingQueue; analyticsLoggingService.logData({ message: `analytics-broker-process tracking finished` }); + +}; + +const killCurrentProcessGracefully = () => { $injector.dispose(); process.exit(); }; @@ -68,12 +72,27 @@ process.on("message", async (data: ITrackingInformation) => { return; } + if (data.type === TrackingTypes.FinishTracking) { + await finishTracking(); + + if (process.connected) { + analyticsLoggingService.logData({ message: `analytics-broker-process will send ${DetachedProcessMessages.ProcessFinishedTasks} message` }); + process.send(DetachedProcessMessages.ProcessFinishedTasks, () => { + analyticsLoggingService.logData({ message: `analytics-broker-process sent ${DetachedProcessMessages.ProcessFinishedTasks} message and will exit gracefully now` }); + killCurrentProcessGracefully(); + }); + } + + return; + } + await sendDataForTracking(data); }); process.on("disconnect", async () => { analyticsLoggingService.logData({ message: "analytics-broker-process received process.disconnect event" }); await finishTracking(); + killCurrentProcessGracefully(); }); analyticsLoggingService.logData({ message: `analytics-broker-process will send ${DetachedProcessMessages.ProcessReadyToReceive} message` }); diff --git a/lib/services/analytics/analytics-service.ts b/lib/services/analytics/analytics-service.ts index a620f2c209..cdcbbda5be 100644 --- a/lib/services/analytics/analytics-service.ts +++ b/lib/services/analytics/analytics-service.ts @@ -146,6 +146,34 @@ export class AnalyticsService implements IAnalyticsService, IDisposable { await this.trackInGoogleAnalytics(eventActionData); } + public async finishTracking(): Promise { + return new Promise((resolve, reject) => { + if (this.brokerProcess && this.brokerProcess.connected) { + let timer: NodeJS.Timer; + + const handler = (data: string) => { + if (data === DetachedProcessMessages.ProcessFinishedTasks) { + this.brokerProcess.removeListener("message", handler); + clearTimeout(timer); + resolve(); + } + }; + + timer = setTimeout(() => { + this.brokerProcess.removeListener("message", handler); + resolve(); + }, 3000); + + this.brokerProcess.on("message", handler); + + const msg = { type: TrackingTypes.FinishTracking }; + this.brokerProcess.send(msg, (err: Error) => this.$logger.trace(`Error while sending ${JSON.stringify(msg)}`)); + } else { + resolve(); + } + }); + } + private forcefullyTrackInGoogleAnalytics(gaSettings: IGoogleAnalyticsData): Promise { gaSettings.customDimensions = gaSettings.customDimensions || {}; gaSettings.customDimensions[GoogleAnalyticsCustomDimensions.client] = this.$options.analyticsClient || (isInteractive() ? AnalyticsClients.Cli : AnalyticsClients.Unknown); From 2d2e0a3fe7dc415449d88724ea325b86012a236a Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 28 Aug 2019 19:00:26 +0300 Subject: [PATCH 2/3] fix: ensure data is sent to analytics on preuninstall CLI's analytics process is a detached one, so during `preuninstall` command, CLI just sends information to the detached process what should be tracked and finishes CLI's execution. After that `npm` starts removing the CLI package and its `node_modules`. At this point the analytics process may still work and trying to send data, but as it relies on components in node_modules, which npm currently deletes, it fails to send information to Google Analytics. To ensure correct data is send to Analytics, ensure tracking is finished before ending the preuninstall command. --- lib/common/commands/preuninstall.ts | 1 + lib/common/test/unit-tests/preuninstall.ts | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/common/commands/preuninstall.ts b/lib/common/commands/preuninstall.ts index a808f41513..478458f47a 100644 --- a/lib/common/commands/preuninstall.ts +++ b/lib/common/commands/preuninstall.ts @@ -27,6 +27,7 @@ export class PreUninstallCommand implements ICommand { } this.$fs.deleteFile(path.join(this.$settingsService.getProfileDir(), "KillSwitches", "cli")); + await this.$analyticsService.finishTracking(); } private async handleIntentionalUninstall(): Promise { diff --git a/lib/common/test/unit-tests/preuninstall.ts b/lib/common/test/unit-tests/preuninstall.ts index 2f2ae3a978..d1ca796b27 100644 --- a/lib/common/test/unit-tests/preuninstall.ts +++ b/lib/common/test/unit-tests/preuninstall.ts @@ -30,7 +30,8 @@ describe("preuninstall", () => { }); testInjector.register("analyticsService", { - trackEventActionInGoogleAnalytics: async (data: IEventActionData): Promise => undefined + trackEventActionInGoogleAnalytics: async (data: IEventActionData): Promise => undefined, + finishTracking: async(): Promise => undefined }); testInjector.registerCommand("dev-preuninstall", PreUninstallCommand); @@ -83,15 +84,22 @@ describe("preuninstall", () => { trackedData.push(data); }; + let isFinishTrackingCalled = false; + analyticsService.finishTracking = async (): Promise => { + isFinishTrackingCalled = true; + }; + const preUninstallCommand: ICommand = testInjector.resolveCommand("dev-preuninstall"); for (const testCase of testData) { helpers.isInteractive = () => testCase.isInteractive; helpers.doesCurrentNpmCommandMatch = () => testCase.isIntentionalUninstall; + isFinishTrackingCalled = false; await preUninstallCommand.execute([]); assert.deepEqual(trackedData, [{ action: "Uninstall CLI", additionalData: testCase.expecteEventLabelData }]); + assert.isTrue(isFinishTrackingCalled, "At the end of the command, finishTracking must be called"); trackedData = []; } }); From bab894d521843c6ed2cf322719e2cf4c64353e23 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Wed, 28 Aug 2019 19:30:31 +0300 Subject: [PATCH 3/3] feat: create analyticsLogFile during preuninstall In case the environment variable `NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE` is set and `npm un -g nativescript` or `npm i -g nativescript` is executed, CLI will use the value of the variable as path for the analyticsLogFile. This way it will be easy to check what information is tracked during install/uninstall of the CLI. --- preuninstall.js | 3 +++ test/preuninstall.ts | 25 +++++++++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/preuninstall.js b/preuninstall.js index de893183a3..5d111abae3 100644 --- a/preuninstall.js +++ b/preuninstall.js @@ -3,6 +3,9 @@ var path = require("path"); var child_process = require("child_process"); var commandArgs = [path.join(__dirname, "bin", "tns"), "dev-preuninstall"]; +if (process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE) { + commandArgs.push("--analyticsLogFile", process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE); +} var childProcess = child_process.spawn(process.execPath, commandArgs, { stdio: "inherit"}) diff --git a/test/preuninstall.ts b/test/preuninstall.ts index 27f29f8aa5..9977d76165 100644 --- a/test/preuninstall.ts +++ b/test/preuninstall.ts @@ -6,6 +6,7 @@ const childProcess = require("child_process"); import { SpawnOptions, ChildProcess } from "child_process"; import * as path from "path"; import { EventEmitter } from "events"; +import { readFileSync } from "fs"; describe("preuninstall.js", () => { let isSpawnCalled = false; @@ -46,10 +47,7 @@ describe("preuninstall.js", () => { const expectedPathToCliExecutable = path.join(__dirname, "..", "bin", "tns"); - assert.isTrue(argsPassedToSpawn.indexOf(expectedPathToCliExecutable) !== -1, `The spawned args must contain path to TNS. - Expected path is: ${expectedPathToCliExecutable}, current args are: ${argsPassedToSpawn}.`); - assert.isTrue(argsPassedToSpawn.indexOf("dev-preuninstall") !== -1, `The spawned args must contain the name of the preuninstall command. - Expected path is: ${expectedPathToCliExecutable}, current args are: ${argsPassedToSpawn}.`); + assert.deepEqual(argsPassedToSpawn, [expectedPathToCliExecutable, "dev-preuninstall"]); assert.deepEqual(optionsPassedToSpawn, [{ stdio: "inherit" }], "The stdio must be inherit as this way CLI's command can determine correctly if terminal is in interactive mode."); assert.deepEqual(dataPassedToConsoleError, []); @@ -58,6 +56,25 @@ describe("preuninstall.js", () => { assert.deepEqual(dataPassedToConsoleError, [`Failed to complete all pre-uninstall steps. Error is ${errMsg}`]); }); + it("passes --analyticsLogFile option when NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE is set", () => { + const content = readFileSync(path.join(__dirname, "..", "preuninstall.js")).toString(); + const originalEnvValue = process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE; + process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE = "value from env analyticsLog.txt"; + /* tslint:disable:no-eval */ + eval(content); + /* tslint:enable:no-eval */ + process.env.NS_CLI_PREUNINSTALL_ANALYTICS_LOG_FILE = originalEnvValue; + assert.isTrue(isSpawnCalled, "child_process.spawn must be called from preuninstall.js"); + + // NOTE: As the script is eval'd, the `__dirname` in it is resolved to current file's location, + // so the expectedPathToCliExecutable should be set as it is located in current dir. + const expectedPathToCliExecutable = path.join(__dirname, "bin", "tns"); + + assert.deepEqual(argsPassedToSpawn, [expectedPathToCliExecutable, "dev-preuninstall", "--analyticsLogFile", "value from env analyticsLog.txt"]); + assert.deepEqual(optionsPassedToSpawn, [{ stdio: "inherit" }], "The stdio must be inherit as this way CLI's command can determine correctly if terminal is in interactive mode."); + assert.deepEqual(dataPassedToConsoleError, []); + }); + it("ensure package.json has correct preuninstall script", () => { const packageJsonData = require("../package.json"); assert.equal(packageJsonData.scripts.preuninstall, "node preuninstall.js");