Expose per-query structured evaluator logs by edoardopirovano · Pull Request #1186 · github/vscode-codeql

edoardopirovano · 2022-03-04T16:08:48Z

Do not merge until the corresponding internal PR to the query server has been merged and the estimated version in this PR has been updated to match when we actually ship that.

This PR adds two new options to the Query History view:

Show Evaluator Log (Raw) displays the raw JSON from the structured logs of a given query run.
Show Evaluator Log (Summary) creates a human-readable summary of the structured logs and displays that.

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.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

aeisenberg · 2022-03-04T19:37:34Z

extensions/ql-vscode/src/pure/messages.ts

+  /**
+   * The dataset for which we want to start a new structured log
+   */
+  db: Dataset;


So, these logs are specific to a dataset, not a query?

In a sense, yes. We rotate the logs when new queries are started, but if there are multiple concurrent queries running then events from the evaluator will end up in the latest query to be started.

aeisenberg · 2022-03-04T19:39:38Z

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

+    multiSelect: QueryHistoryInfo[]
+  ) {
+    // Local queries only
+    if (!this.assertSingleQuery(multiSelect) || singleItem?.t !== 'local') {


Best if you call const { finalSingleItem, finalMultiSelect } = this.determineSelection(singleItem, multiSelect); before this so that you handle some edge cases where you right-click on an item that is not selected.

Thanks, done.

aeisenberg · 2022-03-04T19:39:48Z

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

+    multiSelect: QueryHistoryInfo[]
+  ) {
+    // Local queries only
+    if (!this.assertSingleQuery(multiSelect) || singleItem?.t !== 'local') {


See previous comment.

aeisenberg · 2022-03-04T19:41:34Z

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

+    }
+
+    if (singleItem.evalLogLocation) {
+      const summaryLocation = singleItem.evalLogLocation + '.summary';


I'd prefer to have these file names encapsulated in the LocalQueryInfo object. Can you create a getter called evalLogSummaryLocation? and return the proper path?

Good idea, done.

aeisenberg · 2022-03-04T19:42:23Z

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

 }
+
+export function findQueryStructLogFile(resultPath: string): string {
+  return path.join(resultPath, 'evaluator-log.jsonl');


Just making sure this isn't a typo and it should be evaluator-log.json.

It is not a typo - .jsonl is the format for line-delimited JSON (https://jsonlines.org/) which is what this file is.

aeisenberg · 2022-03-04T19:46:14Z

extensions/ql-vscode/src/run-queries.ts

  progress: ProgressCallback,
  token: CancellationToken,
  templates?: messages.TemplateDefinitions,
+  queryInfo?: LocalQueryInfo,


So, this value will only be available for queries directly initiated by the user? Contextual queries won't have the queryInfo? That's fine, but I just want to be sure I understand. Could you add a comment here to explain that?

Yes, that's exactly right. There's a few places where queries aren't initiated by the user (as you say, contextual queries, and also test code). In these cases we don't have a sensible place to put the structured logs, nor any reason to store them, so we don't create a log for these cases.

edoardopirovano · 2022-03-14T10:58:23Z

Thanks for the review @aeisenberg! I've believe I have addressed all your comments either with responses above or in the new commit 🙂

aeisenberg

Looks good.

angelapwen

✔️ just a few comments since we are waiting for the per-query structured logging PR to merge and be released anyway.

angelapwen · 2022-03-15T21:13:37Z

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

+    if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem || finalSingleItem.t !== 'local') {
+      return;


Should this situation also result in some kind of error message?

Hmm, good question. I followed what @aeisenberg did above for other similar drop downs. Any particular reason you went with just a return rather than a more explicit error?

angelapwen · 2022-03-15T21:13:56Z

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

+    if (!this.assertSingleQuery(finalMultiSelect) || !finalSingleItem || finalSingleItem.t !== 'local') {
+      return;


Same comment as above.

edoardopirovano · 2022-03-29T08:40:17Z

Rebased onto the latest main to check everything is still working since this PR has been open for a while and we're now at a point where we can merge it.

edoardopirovano requested a review from a team as a code owner March 4, 2022 16:08

edoardopirovano force-pushed the edoardo/per-query-struct-logs branch from 40e3944 to b31b26f Compare March 4, 2022 16:10

edoardopirovano added the DO NOT MERGE YET label Mar 4, 2022

aeisenberg reviewed Mar 4, 2022

View reviewed changes

aeisenberg approved these changes Mar 14, 2022

View reviewed changes

angelapwen reviewed Mar 15, 2022

View reviewed changes

angelapwen mentioned this pull request Mar 28, 2022

Gate show eval log and summary commands behind CLI v2.8.4 #1243

Merged

3 tasks

edoardopirovano and others added 3 commits March 29, 2022 04:39

Expose per-query structured evaluator logs

551dcbd

Address review comments from @aeisenberg

ce86ec1

Gate show eval log and summary commands behind CLI v2.8.4 (#1243)

9db40c2

edoardopirovano force-pushed the edoardo/per-query-struct-logs branch from 76cd2fa to 9db40c2 Compare March 29, 2022 08:39

edoardopirovano removed the DO NOT MERGE YET label Mar 29, 2022

edoardopirovano merged commit 3bee290 into main Mar 29, 2022

edoardopirovano deleted the edoardo/per-query-struct-logs branch March 29, 2022 09:30

		if (!this.assertSingleQuery(finalMultiSelect) \|\| !finalSingleItem \|\| finalSingleItem.t !== 'local') {
		return;

Conversation

edoardopirovano commented Mar 4, 2022

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoardopirovano commented Mar 14, 2022

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

angelapwen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoardopirovano commented Mar 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants