Merge pull request #1178 from github/aeisenberg/log-history

Save log files to the query history directory

Author: aeisenberg

extensions/ql-vscode/CHANGELOG.md

@@ -5,6 +5,8 @@
 - Fix a bug where database upgrades could not be resolved if some of the target pack's dependencies are outside of the workspace. [#1138](https://github.com/github/vscode-codeql/pull/1138)
 - Open the query server logs for query errors (instead of the extension log). This will make it easier to track down query errors. [#1158](https://github.com/github/vscode-codeql/pull/1158)
 - Fix a bug where queries took a long time to run if there are no folders in the workspace. [#1157](https://github.com/github/vscode-codeql/pull/1157)
+- [BREAKING CHANGE] The `codeQL.runningQueries.customLogDirectory` setting is deprecated and no longer has any function. Instead, all query log files will be stored in the query history directory, next to the query results. [#1178](https://github.com/github/vscode-codeql/pull/1178)
+- Add a _Open query directory_ command for query items. This command opens the directory containing all artifacts for a query. [#1179](https://github.com/github/vscode-codeql/pull/1179)
 
 ## 1.5.11 - 10 February 2022
 

extensions/ql-vscode/package.json

@@ -208,7 +208,8 @@
             null
           ],
           "default": null,
-          "description": "Path to a directory where the CodeQL extension should store query server logs. If empty, the extension stores logs in a temporary workspace folder and deletes the contents after each run."
+          "description": "Path to a directory where the CodeQL extension should store query server logs. If empty, the extension stores logs in a temporary workspace folder and deletes the contents after each run.",
+          "markdownDeprecationMessage": "This property is deprecated and no longer has any effect. All query logs are stored in the query history folder next to the query results."
         },
         "codeQL.runningQueries.quickEvalCodelens": {
           "type": "boolean",
@@ -507,6 +508,10 @@
         "command": "codeQLQueryHistory.showQueryLog",
         "title": "Show Query Log"
       },
+      {
+        "command": "codeQLQueryHistory.openQueryDirectory",
+        "title": "Open query directory"
+      },
       {
         "command": "codeQLQueryHistory.cancel",
         "title": "Cancel"
@@ -696,6 +701,11 @@
           "group": "9_qlCommands",
           "when": "viewItem == rawResultsItem || viewItem == interpretedResultsItem"
         },
+        {
+          "command": "codeQLQueryHistory.openQueryDirectory",
+          "group": "9_qlCommands",
+          "when": "view == codeQLQueryHistory && !hasRemoteServer"
+        },
         {
           "command": "codeQLQueryHistory.showQueryText",
           "group": "9_qlCommands",
@@ -886,6 +896,10 @@
           "command": "codeQLQueryHistory.showQueryLog",
           "when": "false"
         },
+        {
+          "command": "codeQLQueryHistory.openQueryDirectory",
+          "when": "false"
+        },
         {
           "command": "codeQLQueryHistory.cancel",
           "when": "false"

extensions/ql-vscode/src/extension.ts

@@ -1111,9 +1111,6 @@ function getContextStoragePath(ctx: ExtensionContext) {
 }
 
 async function initializeLogging(ctx: ExtensionContext): Promise<void> {
-  const storagePath = getContextStoragePath(ctx);
-  await logger.setLogStoragePath(storagePath, false);
-  await ideServerLogger.setLogStoragePath(storagePath, false);
   ctx.subscriptions.push(logger);
   ctx.subscriptions.push(queryServerLogger);
   ctx.subscriptions.push(ideServerLogger);

extensions/ql-vscode/src/logging.ts

@@ -1,4 +1,4 @@
-import { window as Window, OutputChannel, Progress, Disposable } from 'vscode';
+import { window as Window, OutputChannel, Progress } from 'vscode';
 import { DisposableObject } from './pure/disposable-object';
 import * as fs from 'fs-extra';
 import * as path from 'path';
@@ -26,18 +26,6 @@ export interface Logger {
    * @param location log to remove
    */
   removeAdditionalLogLocation(location: string | undefined): void;
-
-  /**
-   * The base location where all side log files are stored.
-   */
-  getBaseLocation(): string | undefined;
-
-  /**
-   * Sets the location where logs are stored.
-   * @param storagePath The path where logs are stored.
-   * @param isCustomLogDirectory Whether the logs are stored in a custom, user-specified directory.
-   */
-  setLogStoragePath(storagePath: string, isCustomLogDirectory: boolean): Promise<void>;
 }
 
 export type ProgressReporter = Progress<{ message: string }>;
@@ -46,27 +34,15 @@ export type ProgressReporter = Progress<{ message: string }>;
 export class OutputChannelLogger extends DisposableObject implements Logger {
   public readonly outputChannel: OutputChannel;
   private readonly additionalLocations = new Map<string, AdditionalLogLocation>();
-  private additionalLogLocationPath: string | undefined;
   isCustomLogDirectory: boolean;
 
-  constructor(private title: string) {
+  constructor(title: string) {
     super();
     this.outputChannel = Window.createOutputChannel(title);
     this.push(this.outputChannel);
     this.isCustomLogDirectory = false;
   }
 
-  async setLogStoragePath(storagePath: string, isCustomLogDirectory: boolean): Promise<void> {
-    this.additionalLogLocationPath = path.join(storagePath, this.title);
-
-    this.isCustomLogDirectory = isCustomLogDirectory;
-
-    if (!this.isCustomLogDirectory) {
-      // clear out any old state from previous runs
-      await fs.remove(this.additionalLogLocationPath);
-    }
-  }
-
   /**
    * This function is asynchronous and will only resolve once the message is written
    * to the side log (if required). It is not necessary to await the results of this
@@ -84,18 +60,20 @@ export class OutputChannelLogger extends DisposableObject implements Logger {
         this.outputChannel.append(message);
       }
 
-      if (this.additionalLogLocationPath && options.additionalLogLocation) {
-        const logPath = path.join(this.additionalLogLocationPath, options.additionalLogLocation);
+      if (options.additionalLogLocation) {
+        if (!path.isAbsolute(options.additionalLogLocation)) {
+          throw new Error(`Additional Log Location must be an absolute path: ${options.additionalLogLocation}`);
+        }
+        const logPath = options.additionalLogLocation;
         let additional = this.additionalLocations.get(logPath);
         if (!additional) {
           const msg = `| Log being saved to ${logPath} |`;
           const separator = new Array(msg.length).fill('-').join('');
           this.outputChannel.appendLine(separator);
           this.outputChannel.appendLine(msg);
           this.outputChannel.appendLine(separator);
-          additional = new AdditionalLogLocation(logPath, !this.isCustomLogDirectory);
+          additional = new AdditionalLogLocation(logPath);
           this.additionalLocations.set(logPath, additional);
-          this.track(additional);
         }
 
         await additional.log(message, options);
@@ -115,26 +93,15 @@ export class OutputChannelLogger extends DisposableObject implements Logger {
   }
 
   removeAdditionalLogLocation(location: string | undefined): void {
-    if (this.additionalLogLocationPath && location) {
-      const logPath = location.startsWith(this.additionalLogLocationPath)
-        ? location
-        : path.join(this.additionalLogLocationPath, location);
-      const additional = this.additionalLocations.get(logPath);
-      if (additional) {
-        this.disposeAndStopTracking(additional);
-        this.additionalLocations.delete(logPath);
-      }
+    if (location) {
+      this.additionalLocations.delete(location);
     }
   }
-
-  getBaseLocation() {
-    return this.additionalLogLocationPath;
-  }
 }
 
-class AdditionalLogLocation extends Disposable {
-  constructor(private location: string, private shouldDeleteLogs: boolean) {
-    super(() => { /**/ });
+class AdditionalLogLocation {
+  constructor(private location: string) {
+    /**/
   }
 
   async log(message: string, options = {} as LogOptions): Promise<void> {
@@ -147,12 +114,6 @@ class AdditionalLogLocation extends Disposable {
       encoding: 'utf8'
     });
   }
-
-  async dispose(): Promise<void> {
-    if (this.shouldDeleteLogs) {
-      await fs.remove(this.location);
-    }
-  }
 }
 
 /** The global logger for the extension. */

extensions/ql-vscode/src/query-history.ts

@@ -400,6 +400,12 @@ export class QueryHistoryManager extends DisposableObject {
         this.handleShowQueryLog.bind(this)
       )
     );
+    this.push(
+      commandRunner(
+        'codeQLQueryHistory.openQueryDirectory',
+        this.handleOpenQueryDirectory.bind(this)
+      )
+    );
     this.push(
       commandRunner(
         'codeQLQueryHistory.cancel',
@@ -703,6 +709,34 @@ export class QueryHistoryManager extends DisposableObject {
     }
   }
 
+  async handleOpenQueryDirectory(
+    singleItem: QueryHistoryInfo,
+    multiSelect: QueryHistoryInfo[]
+  ) {
+    const { finalSingleItem, finalMultiSelect } = this.determineSelection(singleItem, multiSelect);
+
+    if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem) {
+      return;
+    }
+
+    let p: string | undefined;
+    if (finalSingleItem.t === 'local') {
+      if (finalSingleItem.completedQuery) {
+        p = finalSingleItem.completedQuery.query.querySaveDir;
+      }
+    } else if (finalSingleItem.t === 'remote') {
+      p = path.join(this.queryStorageDir, finalSingleItem.queryId);
+    }
+
+    if (p) {
+      try {
+        await commands.executeCommand('revealFileInOS', Uri.file(p));
+      } catch (e) {
+        throw new Error(`Failed to open ${p}: ${e.message}`);
+      }
+    }
+  }
+
   async handleCancel(
     singleItem: QueryHistoryInfo,
     multiSelect: QueryHistoryInfo[]

extensions/ql-vscode/src/queryserver-client.ts

@@ -1,5 +1,7 @@
 import * as cp from 'child_process';
 import * as path from 'path';
+import * as fs from 'fs-extra';
+
 import { DisposableObject } from './pure/disposable-object';
 import { Disposable, CancellationToken, commands } from 'vscode';
 import { createMessageConnection, MessageConnection, RequestType } from 'vscode-jsonrpc';
@@ -9,8 +11,6 @@ import { Logger, ProgressReporter } from './logging';
 import { completeQuery, EvaluationResult, progress, ProgressMessage, WithProgressId } from './pure/messages';
 import * as messages from './pure/messages';
 import { ProgressCallback, ProgressTask } from './commandRunner';
-import * as fs from 'fs-extra';
-import * as helpers from './helpers';
 
 type ServerOpts = {
   logger: Logger;
@@ -68,7 +68,7 @@ export class QueryServerClient extends DisposableObject {
     this.queryServerStartListeners.push(e);
   }
 
-  public activeQueryName: string | undefined;
+  public activeQueryLogFile: string | undefined;
 
   constructor(
     readonly config: QueryServerConfig,
@@ -89,26 +89,6 @@ export class QueryServerClient extends DisposableObject {
     this.evaluationResultCallbacks = {};
   }
 
-  async initLogger() {
-    let storagePath = this.opts.contextStoragePath;
-    let isCustomLogDirectory = false;
-    if (this.config.customLogDirectory) {
-      try {
-        if (!(await fs.pathExists(this.config.customLogDirectory))) {
-          await fs.mkdir(this.config.customLogDirectory);
-        }
-        void this.logger.log(`Saving query server logs to user-specified directory: ${this.config.customLogDirectory}.`);
-        storagePath = this.config.customLogDirectory;
-        isCustomLogDirectory = true;
-      } catch (e) {
-        void helpers.showAndLogErrorMessage(`${this.config.customLogDirectory} is not a valid directory. Logs will be stored in a temporary workspace directory instead.`);
-      }
-    }
-
-    await this.logger.setLogStoragePath(storagePath, isCustomLogDirectory);
-
-  }
-
   get logger(): Logger {
     return this.opts.logger;
   }
@@ -150,7 +130,6 @@ export class QueryServerClient extends DisposableObject {
 
   /** Starts a new query server process, sending progress messages to the given reporter. */
   private async startQueryServerImpl(progressReporter: ProgressReporter): Promise<void> {
-    await this.initLogger();
     const ramArgs = await this.cliServer.resolveRam(this.config.queryMemoryMb, progressReporter);
     const args = ['--threads', this.config.numThreads.toString()].concat(ramArgs);
 
@@ -172,10 +151,13 @@ export class QueryServerClient extends DisposableObject {
     }
 
     if (await this.cliServer.cliConstraints.supportsStructuredEvalLog()) {
+      const structuredLogFile = `${this.opts.contextStoragePath}/structured-evaluator-log.json`;
+      await fs.ensureFile(structuredLogFile);
+
       args.push('--evaluator-log');
-      args.push(`${this.opts.contextStoragePath}/structured-evaluator-log.json`);
+      args.push(structuredLogFile);
 
-      // We hard-code the verbosity level to 5 and minify to false. 
+      // We hard-code the verbosity level to 5 and minify to false.
       // This will be the behavior of the per-query structured logging in the CLI after 2.8.3.
       args.push('--evaluator-log-level');
       args.push('5');
@@ -186,7 +168,7 @@ export class QueryServerClient extends DisposableObject {
     }
 
     if (cli.shouldDebugQueryServer()) {
-      args.push('-J=-agentlib:jdwp=transport=dt_socket,address=localhost:9010,server=y,suspend=n,quiet=y');
+      args.push('-J=-agentlib:jdwp=transport=dt_socket,address=localhost:9010,server=n,suspend=y,quiet=y');
     }
 
     const child = cli.spawnServer(
@@ -197,7 +179,7 @@ export class QueryServerClient extends DisposableObject {
       this.logger,
       data => this.logger.log(data.toString(), {
         trailingNewline: false,
-        additionalLogLocation: this.activeQueryName
+        additionalLogLocation: this.activeQueryLogFile
       }),
       undefined, // no listener for stdout
       progressReporter
@@ -208,10 +190,6 @@ export class QueryServerClient extends DisposableObject {
       if (!(res.runId in this.evaluationResultCallbacks)) {
         void this.logger.log(`No callback associated with run id ${res.runId}, continuing without executing any callback`);
       } else {
-        const baseLocation = this.logger.getBaseLocation();
-        if (baseLocation && this.activeQueryName) {
-          res.logFileLocation = path.join(baseLocation, this.activeQueryName);
-        }
         this.evaluationResultCallbacks[res.runId](res);
       }
       return {};
@@ -272,8 +250,11 @@ export class QueryServerClient extends DisposableObject {
    */
   private updateActiveQuery(method: string, parameter: any): void {
     if (method === messages.compileQuery.method) {
-      const queryPath = parameter?.queryToCheck?.queryPath || 'unknown';
-      this.activeQueryName = `query-${path.basename(queryPath)}-${this.nextProgress}.log`;
+      this.activeQueryLogFile = findQueryLogFile(path.dirname(parameter.resultPath));
     }
   }
 }
+
+export function findQueryLogFile(resultPath: string): string {
+  return path.join(resultPath, 'query.log');
+}

extensions/ql-vscode/src/remote-queries/remote-query-history-item.ts

@@ -10,6 +10,6 @@ export interface RemoteQueryHistoryItem {
   status: QueryStatus;
   completed: boolean;
   readonly queryId: string,
-  label: string, // TODO, the query label should have interpolation like local queries
-  remoteQuery: RemoteQuery,
+  label: string; // TODO, the query label should have interpolation like local queries
+  remoteQuery: RemoteQuery;
 }