fix(cli): drop invalid commands from telemetry (#997)

We should only capture CLI sessions that were invoked with a valid
subcommand.

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

---------

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: iliapolo

packages/@aws-cdk-testing/cli-integ/tests/telemetry-integ-tests/cdk-invalid-command-telemetry.integtest.ts

@@ -0,0 +1,15 @@
+import * as path from 'path';
+import * as fs from 'fs-extra';
+import { integTest, withDefaultFixture } from '../../lib';
+
+jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
+
+integTest('no telemetry is collected if command is invalid', withDefaultFixture(async (fixture) => {
+  const telemetryFile = path.join(fixture.integTestDir, `telemetry-${Date.now()}.json`);
+
+  const commandOutput = await fixture.cdk(['invalid-command', `--telemetry-file=${telemetryFile}`], { verboseLevel: 3, allowErrExit: true }); // trace mode
+
+  expect(commandOutput).toContain('Session instantiated with an invalid command');
+  expect(fs.existsSync(telemetryFile)).toBeFalsy();
+}),
+);

packages/@aws-cdk-testing/cli-integ/tests/telemetry-integ-tests/cdk-synth-telemetry-with-errors.integtest.ts

@@ -1,5 +1,6 @@
 import * as path from 'path';
 import * as fs from 'fs-extra';
+import { CURRENT_TELEMETRY_VERSION } from './constants';
 import { integTest, withDefaultFixture } from '../../lib';
 
 jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
@@ -51,7 +52,7 @@ integTest(
         identifiers: expect.objectContaining({
           installationId: expect.anything(),
           sessionId: expect.anything(),
-          telemetryVersion: '1.0',
+          telemetryVersion: CURRENT_TELEMETRY_VERSION,
           cdkCliVersion: expect.anything(),
           cdkLibraryVersion: fixture.library.requestedVersion(),
           region: expect.anything(),
@@ -99,7 +100,7 @@ integTest(
         identifiers: expect.objectContaining({
           installationId: expect.anything(),
           sessionId: expect.anything(),
-          telemetryVersion: '1.0',
+          telemetryVersion: CURRENT_TELEMETRY_VERSION,
           cdkCliVersion: expect.anything(),
           cdkLibraryVersion: fixture.library.requestedVersion(),
           region: expect.anything(),

packages/@aws-cdk-testing/cli-integ/tests/telemetry-integ-tests/cdk-synth-telemetry.integtest.ts

@@ -1,5 +1,6 @@
 import * as path from 'path';
 import * as fs from 'fs-extra';
+import { CURRENT_TELEMETRY_VERSION } from './constants';
 import { integTest, withDefaultFixture } from '../../lib';
 
 jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
@@ -48,7 +49,7 @@ integTest(
         identifiers: expect.objectContaining({
           installationId: expect.anything(),
           sessionId: expect.anything(),
-          telemetryVersion: '1.0',
+          telemetryVersion: CURRENT_TELEMETRY_VERSION,
           cdkCliVersion: expect.anything(),
           cdkLibraryVersion: fixture.library.requestedVersion(),
           region: expect.anything(),
@@ -96,7 +97,7 @@ integTest(
         identifiers: expect.objectContaining({
           installationId: expect.anything(),
           sessionId: expect.anything(),
-          telemetryVersion: '1.0',
+          telemetryVersion: CURRENT_TELEMETRY_VERSION,
           cdkCliVersion: expect.anything(),
           cdkLibraryVersion: fixture.library.requestedVersion(),
           region: expect.anything(),

packages/@aws-cdk-testing/cli-integ/tests/telemetry-integ-tests/constants.ts

@@ -0,0 +1 @@
+export const CURRENT_TELEMETRY_VERSION = '2.0';

packages/aws-cdk/lib/cli/io-host/cli-io-host.ts

@@ -26,14 +26,14 @@ export type { IIoHost, IoMessage, IoMessageCode, IoMessageLevel, IoRequest };
  * The current action being performed by the CLI. 'none' represents the absence of an action.
  */
 type CliAction =
-| ToolkitAction
-| 'context'
-| 'docs'
-| 'flags'
-| 'notices'
-| 'version'
-| 'cli-telemetry'
-| 'none';
+  | ToolkitAction
+  | 'context'
+  | 'docs'
+  | 'flags'
+  | 'notices'
+  | 'version'
+  | 'cli-telemetry'
+  | 'none';
 
 export interface CliIoHostProps {
   /**
@@ -189,6 +189,17 @@ export class CliIoHost implements IIoHost {
   }
 
   public async startTelemetry(args: any, context: Context, proxyAgent?: Agent) {
+    // eslint-disable-next-line @typescript-eslint/no-require-imports
+    const config = require('../cli-type-registry.json');
+    const validCommands = Object.keys(config.commands);
+    const cmd = args._[0];
+    if (!validCommands.includes(cmd)) {
+      // the user typed in an invalid command - no need for telemetry since the invocation is going to fail
+      // imminently anyway.
+      await this.asIoHelper().defaults.trace(`Session instantiated with an invalid command (${cmd}). Not starting telemetry.`);
+      return;
+    }
+
     let sinks: ITelemetrySink[] = [];
     const telemetryFilePath = args['telemetry-file'];
     if (telemetryFilePath) {

packages/aws-cdk/lib/cli/telemetry/session.ts

@@ -48,7 +48,7 @@ export class TelemetrySession {
       identifiers: {
         installationId: await getOrCreateInstallationId(this.ioHost.asIoHelper()),
         sessionId: randomUUID(),
-        telemetryVersion: '1.0',
+        telemetryVersion: '2.0',
         cdkCliVersion: versionNumber(),
         cdkLibraryVersion: await getLibraryVersion(this.ioHost.asIoHelper()),
       },
@@ -126,7 +126,7 @@ export class TelemetrySession {
       duration: {
         total: event.duration,
       },
-      ...( event.error ? {
+      ...(event.error ? {
         error: {
           name: event.error.name,
         },

packages/aws-cdk/test/cli/io-host/cli-io-host.test.ts

@@ -14,7 +14,7 @@ let passThrough: PassThrough;
 const originalProcessOn = process.on;
 
 // Mock process.on to be a no-op function that returns process for chaining
-process.on = jest.fn().mockImplementation(function() {
+process.on = jest.fn().mockImplementation(function () {
   return process;
 }) as any;
 
@@ -279,6 +279,16 @@ describe('CliIoHost', () => {
     });
   });
 
+  test('telemetry should not be instantiated with an invalid command', async () => {
+    const telemetryIoHost = CliIoHost.instance({
+      logLevel: 'trace',
+    }, true);
+
+    await telemetryIoHost.startTelemetry({ _: ['invalid'] }, new Context());
+
+    expect(telemetryIoHost.telemetry).toBeUndefined();
+  });
+
   describe('telemetry', () => {
     let telemetryIoHost: CliIoHost;
     let telemetryEmitSpy: jest.SpyInstance;
@@ -293,7 +303,7 @@ describe('CliIoHost', () => {
       telemetryIoHost = CliIoHost.instance({
         logLevel: 'trace',
       }, true);
-      await telemetryIoHost.startTelemetry({ '_': 'init', 'telemetry-file': telemetryFilePath }, new Context());
+      await telemetryIoHost.startTelemetry({ '_': ['init'], 'telemetry-file': telemetryFilePath }, new Context());
 
       expect(telemetryIoHost.telemetry).toBeDefined();