Specify custom directory for storing evaluation logs #863

shati-patel · 2021-05-14T17:09:28Z

Will fix #820.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.
- Docs-wise, we should probably mention this in https://codeql.github.com/docs/codeql-for-visual-studio-code/troubleshooting-codeql-for-visual-studio-code/. I'll open a PR for this: Docs: Describe custom log directory setting in VS Code extension codeql#5946

aeisenberg · 2021-05-20T14:33:47Z

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

+      if (fs.existsSync(this.config.customLogDirectory) && fs.statSync(this.config.customLogDirectory).isDirectory()) {
+        storagePath = this.config.customLogDirectory;
+        isCustomLogDirectory = true;
+      } else if (this.config.customLogDirectory) {
+        helpers.showAndLogErrorMessage(`${this.config.customLogDirectory} is not a valid directory. Logs will be stored in a temporary workspace directory instead.`);
+      }


If the custom path doesn't exist, maybe you can create it (with an info message). I think it's only an error if it already exists, but isn't a file.

I made an attempt in b7897b0, which seems to do the right thing, but any tips on syntax/fixes are very welcome 😄

adityasharad · 2021-05-20T21:28:29Z

Looks good so far. Next step is to look at the tests for logging: you'll need to update the tests that use loggers to pass in the new boolean flags you introduced, and check their behaviour in each case.

shati-patel · 2021-05-21T15:04:15Z

extensions/ql-vscode/test/pure-tests/logging.test.ts

+  it('should delete an existing folder when setting the log storage path', async () => {
    fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.init(tempFolders.storagePath.name);
+    logger.setLogStoragePath(tempFolders.storagePath.name, false);
    // should be empty dir

+    const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');
+    // TODO: Why does this test pass? I'd expect the length to be 0 if it's correctly deleted the existing folder.
+    expect(fs.readdirSync(testLoggerFolder).length).to.equal(1);
+  });


I couldn't work out why this test passes with "length equal to 1". Can someone explain? I might be misunderstanding what it's doing... 👀

Is it possible fs.remove is not working because the directory is not empty? Try setting a breakpoint on line 129 and step through to see what happens and what is deleted.

Also, I wonder if that method isn't completely synchronous. You can try addinga a waitABit call in between and see what happens.

Oh the sync/async might be it. I was also baffled by why the existing test worked.

extensions/ql-vscode/src/logging.ts

aeisenberg · 2021-05-21T15:46:40Z

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

+    let isCustomLogDirectory = false;
+    if (this.config.customLogDirectory) {
+      try {
+        fs.mkdirSync(this.config.customLogDirectory);


This method needs to be async

Suggested change

fs.mkdirSync(this.config.customLogDirectory);

await fs.mkdir(this.config.customLogDirectory);

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

aeisenberg · 2021-05-21T16:01:15Z

extensions/ql-vscode/test/pure-tests/logging.test.ts

+  it('should delete an existing folder when setting the log storage path', async () => {
    fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.init(tempFolders.storagePath.name);
+    logger.setLogStoragePath(tempFolders.storagePath.name, false);
    // should be empty dir

+    const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');
+    // TODO: Why does this test pass? I'd expect the length to be 0 if it's correctly deleted the existing folder.
+    expect(fs.readdirSync(testLoggerFolder).length).to.equal(1);
+  });


Is it possible fs.remove is not working because the directory is not empty? Try setting a breakpoint on line 129 and step through to see what happens and what is deleted.

aeisenberg · 2021-05-21T16:02:26Z

extensions/ql-vscode/test/pure-tests/logging.test.ts

+  it('should delete an existing folder when setting the log storage path', async () => {
    fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.init(tempFolders.storagePath.name);
+    logger.setLogStoragePath(tempFolders.storagePath.name, false);
    // should be empty dir

+    const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');
+    // TODO: Why does this test pass? I'd expect the length to be 0 if it's correctly deleted the existing folder.
+    expect(fs.readdirSync(testLoggerFolder).length).to.equal(1);
+  });


Also, I wonder if that method isn't completely synchronous. You can try addinga a waitABit call in between and see what happens.

aeisenberg · 2021-05-21T16:09:17Z

extensions/ql-vscode/src/logging.ts

-    fs.remove(this.additionalLogLocationPath);
+    this.isCustomLogDirectory = isCustomLogDirectory;
+
+    if (!this.isCustomLogDirectory) {


So, what this means is whenever someone changes the query server logs from the default to a custom location, then the old logs are deleted. And if they switch back, there won't be any logs at all. Maybe that's ok.

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

aeisenberg

One last change and then .

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

(I haven't got the error handling to work asynchronously, so I stuck with `mkdirSync` for now)

aeisenberg

Great job!

aeisenberg reviewed May 20, 2021

View reviewed changes

shati-patel changed the title ~~[WIP] Specify custom directory for storing evaluation logs~~ Specify custom directory for storing evaluation logs May 21, 2021

shati-patel force-pushed the shati-patel/store-logs branch 2 times, most recently from 892a125 to cd39ede Compare May 21, 2021 15:01

shati-patel marked this pull request as ready for review May 21, 2021 15:03

shati-patel requested review from aeisenberg and adityasharad May 21, 2021 15:03

shati-patel commented May 21, 2021

View reviewed changes

aeisenberg reviewed May 21, 2021

View reviewed changes

shati-patel commented May 21, 2021

View reviewed changes

extensions/ql-vscode/src/queryserver-client.ts Outdated Show resolved Hide resolved

shati-patel force-pushed the shati-patel/store-logs branch from 8d711c6 to 8e4930c Compare May 21, 2021 19:55

aeisenberg reviewed May 21, 2021

View reviewed changes

extensions/ql-vscode/src/queryserver-client.ts Outdated Show resolved Hide resolved

shati-patel added 5 commits May 21, 2021 21:32

Specify custom directory for storing query server logs

18fc98f

Create custom log directory, if possible

25a6965

(I haven't got the error handling to work asynchronously, so I stuck with `mkdirSync` for now)

Update tests

ad0fb75

Add changelog note

0c03195

Make functions async + other review comments

43fba35

shati-patel force-pushed the shati-patel/store-logs branch from 1637a22 to 43fba35 Compare May 21, 2021 20:34

aeisenberg approved these changes May 21, 2021

View reviewed changes

shati-patel merged commit 2f3be92 into main May 21, 2021

shati-patel deleted the shati-patel/store-logs branch May 21, 2021 20:41

shati-patel mentioned this pull request May 21, 2021

Docs: Describe custom log directory setting in VS Code extension github/codeql#5946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify custom directory for storing evaluation logs #863

Specify custom directory for storing evaluation logs #863

shati-patel commented May 14, 2021 •

edited

Loading

aeisenberg May 20, 2021

shati-patel May 20, 2021 •

edited

Loading

adityasharad commented May 20, 2021

shati-patel May 21, 2021

aeisenberg May 21, 2021

aeisenberg May 21, 2021

adityasharad May 21, 2021

aeisenberg May 21, 2021

aeisenberg May 21, 2021

aeisenberg May 21, 2021

aeisenberg May 21, 2021

aeisenberg left a comment

aeisenberg left a comment

	fs.mkdirSync(this.config.customLogDirectory);
	await fs.mkdir(this.config.customLogDirectory);

Specify custom directory for storing evaluation logs #863

Specify custom directory for storing evaluation logs #863

Conversation

shati-patel commented May 14, 2021 • edited Loading

Checklist

Choose a reason for hiding this comment

shati-patel May 20, 2021 • edited Loading

Choose a reason for hiding this comment

adityasharad commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg left a comment

Choose a reason for hiding this comment

aeisenberg left a comment

Choose a reason for hiding this comment

shati-patel commented May 14, 2021 •

edited

Loading

shati-patel May 20, 2021 •

edited

Loading