Merge pull request #78 from tiktok/feat-sparo-metric

Feat sparo metric

apps/sparo-lib/src/services/CommandService.ts

@@ -21,6 +21,7 @@ export class CommandService {
   @inject(HelpTextService) private _helpTextService!: HelpTextService;
   @inject(TerminalService) private _terminalService!: TerminalService;
   @inject(TelemetryService) private _telemetryService!: TelemetryService;
+  private _hasInternalError: boolean = false;
 
   public register<O extends {}>(command: ICommand<O>): void {
     const { cmd, description, builder, handler, getHelp } = command;
@@ -36,19 +37,23 @@ export class CommandService {
       },
       async (args) => {
         process.exitCode = 1;
+        this._hasInternalError = false;
         try {
           terminal.writeVerboseLine(`Invoking command "${commandName}" with args ${JSON.stringify(args)}`);
           const stopwatch: Stopwatch = Stopwatch.start();
           await handler(args, terminalService);
           terminal.writeVerboseLine(`Invoked command "${commandName}" done (${stopwatch.toString()})`);
           stopwatch.stop();
-          this._telemetryService.collectTelemetry({
-            commandName,
-            args: process.argv.slice(2),
-            durationInSeconds: stopwatch.duration,
-            startTimestampMs: stopwatch.startTime,
-            endTimestampMs: stopwatch.endTime
-          });
+          if (!this._hasInternalError) {
+            // Only report success data
+            this._telemetryService.collectTelemetry({
+              commandName,
+              args: process.argv.slice(2),
+              durationInSeconds: stopwatch.duration,
+              startTimestampMs: stopwatch.startTime,
+              endTimestampMs: stopwatch.endTime
+            });
+          }
           // eslint-disable-next-line require-atomic-updates
           process.exitCode = 0;
         } catch (e) {
@@ -60,4 +65,8 @@ export class CommandService {
     );
     this._helpTextService.set(commandName, getHelp());
   }
+
+  public setHasInternalError(): void {
+    this._hasInternalError = true;
+  }
 }

apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts

@@ -18,7 +18,7 @@ export class GitRemoteFetchConfigService {
   @inject(GracefulShutdownService) private _gracefulShutdownService!: GracefulShutdownService;
 
   public addRemoteBranchIfNotExists(remote: string, branch: string): void {
-    const remoteFetchGitConfig: string[] | undefined = this._loadForRemote(remote);
+    const remoteFetchGitConfig: string[] | undefined = this._getRemoteFetchInGitConfig(remote);
 
     if (remoteFetchGitConfig) {
       const targetConfig: string = `+refs/heads/${branch}:refs/remotes/${remote}/${branch}`;
@@ -38,7 +38,7 @@ export class GitRemoteFetchConfigService {
   }
 
   public pruneRemoteBranchesInGitConfigAsync = async (remote: string): Promise<void> => {
-    const remoteFetchConfig: string[] | undefined = this._loadForRemote(remote);
+    const remoteFetchConfig: string[] | undefined = this._getRemoteFetchInGitConfig(remote);
     if (!remoteFetchConfig) {
       return;
     }
@@ -50,6 +50,8 @@ export class GitRemoteFetchConfigService {
     );
     const checkBranches: string[] = Array.from(branchToValues.keys()).filter((x) => x !== '*');
 
+    this._terminalService.terminal.writeLine(`Checking tracking branches...`);
+
     const remoteBranchExistenceInfo: Record<string, boolean> =
       await this._gitService.checkRemoteBranchesExistenceAsync(remote, checkBranches);
 
@@ -103,12 +105,12 @@ export class GitRemoteFetchConfigService {
    * It's used in "sparo fetch --all" command
    */
   public revertSingleBranchIfNecessary = (remote: string): (() => void) | undefined => {
-    let remoteFetchGitConfig: string[] | undefined = this._loadForRemote(remote);
+    let remoteFetchGitConfig: string[] | undefined = this._getRemoteFetchInGitConfig(remote);
     let callback: (() => void) | undefined;
     if (remoteFetchGitConfig && !remoteFetchGitConfig.includes(`+refs/heads/*:refs/remotes/${remote}/*`)) {
       this._setAllBranchFetch(remote);
 
-      callback = () => {
+      callback = (): void => {
         if (remoteFetchGitConfig) {
           this._setRemoteFetchInGitConfig(remote, remoteFetchGitConfig);
 
@@ -147,7 +149,7 @@ export class GitRemoteFetchConfigService {
     return branchToValues;
   }
 
-  private _loadForRemote(remote: string): string[] | undefined {
+  private _getRemoteFetchInGitConfig(remote: string): string[] | undefined {
     const result: string | undefined = this._gitService.getGitConfig(`remote.${remote}.fetch`, {
       array: true
     });
@@ -160,9 +162,10 @@ export class GitRemoteFetchConfigService {
    * So, delete all remote.origin.fetch configuration and restores expected value
    */
   private _setRemoteFetchInGitConfig(remote: string, remoteFetchGitConfig: string[]): void {
-    this._gitService.unsetGitConfig(`remote.${remote}.fetch`);
+    const key: string = `remote.${remote}.fetch`;
+    this._gitService.unsetGitConfig(key);
     for (const value of remoteFetchGitConfig) {
-      this._gitService.setGitConfig(`remote.${remote}.fetch`, value, { add: true });
+      this._gitService.setGitConfig(key, value, { add: true });
     }
   }
 

apps/sparo-lib/src/services/GitService.ts

@@ -6,6 +6,7 @@ import { Service } from '../decorator';
 import { TerminalService } from './TerminalService';
 import { Stopwatch } from '../logic/Stopwatch';
 import { TelemetryService } from './TelemetryService';
+import { CommandService } from './CommandService';
 
 /**
  * @alpha
@@ -34,6 +35,7 @@ export class GitService {
   private _isSparseCheckoutMode: boolean | undefined;
   @inject(TerminalService) private _terminalService!: TerminalService;
   @inject(TelemetryService) private _telemetryService!: TelemetryService;
+  @inject(CommandService) private _commandService!: CommandService;
 
   public setGitConfig(
     k: string,
@@ -248,14 +250,18 @@ export class GitService {
     this._terminalService.terminal.writeDebugLine(`Invoked git command done (${stopwatch.toString()})`);
     this._terminalService.writeTaskFooter();
     stopwatch.stop();
-    this._telemetryService.collectTelemetry({
-      commandName: args[0],
-      args: args.slice(1),
-      durationInSeconds: stopwatch.duration,
-      startTimestampMs: stopwatch.startTime,
-      endTimestampMs: stopwatch.endTime,
-      isRawGitCommand: true
-    });
+    if (result.status === 0) {
+      this._telemetryService.collectTelemetry({
+        commandName: args[0],
+        args: args.slice(1),
+        durationInSeconds: stopwatch.duration,
+        startTimestampMs: stopwatch.startTime,
+        endTimestampMs: stopwatch.endTime,
+        isRawGitCommand: true
+      });
+    } else {
+      this._commandService.setHasInternalError();
+    }
     return result;
   }
 
@@ -274,14 +280,16 @@ export class GitService {
     });
     this._terminalService.terminal.writeDebugLine(`Invoked git command done (${stopwatch.toString()})`);
     stopwatch.stop();
-    this._telemetryService.collectTelemetry({
-      commandName: args[0],
-      args: args.slice(1),
-      durationInSeconds: stopwatch.duration,
-      startTimestampMs: stopwatch.startTime,
-      endTimestampMs: stopwatch.endTime,
-      isRawGitCommand: true
-    });
+    if (result.status === 0) {
+      this._telemetryService.collectTelemetry({
+        commandName: args[0],
+        args: args.slice(1),
+        durationInSeconds: stopwatch.duration,
+        startTimestampMs: stopwatch.startTime,
+        endTimestampMs: stopwatch.endTime,
+        isRawGitCommand: true
+      });
+    }
     this._processResult(result);
     return result.stdout.toString();
   }
@@ -487,31 +495,54 @@ Please specify a directory on the command line
   public checkRemoteBranchExistenceAsync = async (remote: string, branch: string): Promise<boolean> => {
     const gitPath: string = this.getGitPathOrThrow();
     const currentWorkingDirectory: string = this.getRepoInfo().root;
-    const childProcess: child_process.ChildProcess = Executable.spawn(
-      gitPath,
-      ['ls-remote', '--exit-code', remote, branch],
-      {
-        currentWorkingDirectory,
-        stdio: ['ignore', 'pipe', 'pipe']
-      }
-    );
+    const isDebug: boolean = this._terminalService.isDebug;
+    const lsRemoteArgs: string[] = ['ls-remote', '--exit-code', '--heads', remote, branch];
+    const { terminal } = this._terminalService;
+    terminal.writeDebugLine(`Running git ${lsRemoteArgs.join(' ')}...`);
+    const childProcess: child_process.ChildProcess = Executable.spawn(gitPath, lsRemoteArgs, {
+      currentWorkingDirectory,
+      stdio: ['ignore', 'pipe', 'pipe']
+    });
     if (!childProcess.stdout || !childProcess.stderr) {
-      this._terminalService.terminal.writeDebugLine(`Failed to spawn git process, fallback to spawnSync`);
+      terminal.writeDebugLine(`Failed to spawn git process, fallback to spawnSync`);
       const result: string = this.executeGitCommandAndCaptureOutput({
         args: ['ls-remote', remote, branch]
       }).trim();
       return Promise.resolve(!!result);
     }
+    if (isDebug) {
+      childProcess.stdout.on('data', (data: Buffer) => {
+        const text: string = data.toString();
+        terminal.writeDebugLine(text);
+      });
+      childProcess.stderr.on('data', (data: Buffer) => {
+        const text: string = data.toString();
+        terminal.writeDebugLine(text);
+      });
+    }
     return await new Promise((resolve, reject) => {
       // Only care about exit code since specifying --exit-code
       childProcess.on('close', (exitCode: number | null) => {
-        if (exitCode) {
-          this._terminalService.terminal.writeDebugLine(`Branch "${branch}" doesn't exist remotely`);
-          resolve(false);
-        } else {
-          this._terminalService.terminal.writeDebugLine(`Branch "${branch}" exists remotely`);
-          resolve(true);
+        // Allow exitCode 128. It indicates permission issue
+        switch (exitCode) {
+          case 0: {
+            terminal.writeDebugLine(`Branch "${branch}" exists remotely`);
+            break;
+          }
+          case 2: {
+            terminal.writeDebugLine(`Branch "${branch}" doesn't exist remotely`);
+            return resolve(false);
+          }
+          case 128: {
+            terminal.writeDebugLine(`Check "${branch}" failed because of permission issue`);
+            break;
+          }
+          default: {
+            terminal.writeDebugLine(`Check "${branch}" returns unknown exit code ${exitCode}`);
+            break;
+          }
         }
+        resolve(true);
       });
     });
   };

apps/sparo-lib/src/services/TerminalService.ts

@@ -65,6 +65,10 @@ export class TerminalService {
     this._terminal.writeLine(Colorize.gray('-'.repeat(this._asciiHeaderWidth)));
     this._terminal.writeLine();
   }
+
+  public get isDebug(): boolean {
+    return this._terminalProvider.debugEnabled;
+  }
 }
 
 export type { ITerminal };

common/changes/sparo/feat-sparo-metric_2024-06-27-23-23.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "sparo",
+      "comment": "Improve logic to check existence of a branch",
+      "type": "none"
+    }
+  ],
+  "packageName": "sparo"
+}

common/changes/sparo/feat-sparo-metric_2024-06-27-23-46.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "sparo",
+      "comment": "Enhance metric logic to accurately report success data",
+      "type": "none"
+    }
+  ],
+  "packageName": "sparo"
+}

common/reviews/api/sparo-lib.api.md

@@ -122,6 +122,8 @@ export class Sparo {
 export class TerminalService {
     constructor();
     // (undocumented)
+    get isDebug(): boolean;
+    // (undocumented)
     setIsDebug(value: boolean): void;
     // (undocumented)
     setIsVerbose(value: boolean): void;