Skip to content

fix: ensure data is sent to analytics on preuninstall #4989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/common/commands/preuninstall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
Expand Down
7 changes: 7 additions & 0 deletions lib/common/declarations.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}

/**
Expand Down Expand Up @@ -688,6 +694,7 @@ interface IAnalyticsService {
setStatus(settingName: string, enabled: boolean): Promise<void>;
getStatusMessage(settingName: string, jsonFormat: boolean, readableSettingName: string): Promise<string>;
isEnabled(settingName: string): Promise<boolean>;
finishTracking(): Promise<void>;

/**
* Tracks the answer of question if user allows to be tracked.
Expand Down
10 changes: 9 additions & 1 deletion lib/common/test/unit-tests/preuninstall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ describe("preuninstall", () => {
});

testInjector.register("analyticsService", {
trackEventActionInGoogleAnalytics: async (data: IEventActionData): Promise<void> => undefined
trackEventActionInGoogleAnalytics: async (data: IEventActionData): Promise<void> => undefined,
finishTracking: async(): Promise<void> => undefined
});

testInjector.registerCommand("dev-preuninstall", PreUninstallCommand);
Expand Down Expand Up @@ -83,15 +84,22 @@ describe("preuninstall", () => {
trackedData.push(data);
};

let isFinishTrackingCalled = false;
analyticsService.finishTracking = async (): Promise<void> => {
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 = [];
}
});
Expand Down
7 changes: 6 additions & 1 deletion lib/detached-processes/detached-process-enums.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}

/**
Expand Down
19 changes: 19 additions & 0 deletions lib/services/analytics/analytics-broker-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
};
Expand Down Expand Up @@ -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` });
Expand Down
28 changes: 28 additions & 0 deletions lib/services/analytics/analytics-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,34 @@ export class AnalyticsService implements IAnalyticsService, IDisposable {
await this.trackInGoogleAnalytics(eventActionData);
}

public async finishTracking(): Promise<void> {
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<void> {
gaSettings.customDimensions = gaSettings.customDimensions || {};
gaSettings.customDimensions[GoogleAnalyticsCustomDimensions.client] = this.$options.analyticsClient || (isInteractive() ? AnalyticsClients.Cli : AnalyticsClients.Unknown);
Expand Down
3 changes: 3 additions & 0 deletions preuninstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"})

Expand Down
25 changes: 21 additions & 4 deletions test/preuninstall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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, []);

Expand All @@ -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");
Expand Down