Skip to content

Show query results before structured evaluator log summary completes#1350

Merged
angelapwen merged 3 commits intogithub:mainfrom
angelapwen:unblock-log-summary-results
May 17, 2022
Merged

Show query results before structured evaluator log summary completes#1350
angelapwen merged 3 commits intogithub:mainfrom
angelapwen:unblock-log-summary-results

Conversation

@angelapwen
Copy link
Copy Markdown
Contributor

Previously, we were awaiting the structured evaluator log summary to be fully generated before the query result and raw structured log could be viewed, even though they were already available.

This change allows the structured evaluator log summary to execute asynchronously. If the user attempts to click the "Show Structured Evaluator Log (Summary)" command on the query history item before the summary is complete, the user receives a popup saying "The evaluator log summary is still being generated. Please try again later. The summary generation process is tracked in the "CodeQL Extension Log" view." — 

Screen Shot 2022-05-16 at 4 41 16 PM

The CodeQL Extension Log view will indicate the summary generation is in-progress and when it is complete. It will also log the same warning line as above every time the user attempts to view the summary file before it is completed (in the following screenshot the command was clicked twice):
Screen Shot 2022-05-16 at 4 45 41 PM

and at this point the user will be able to view the log summary by clicking the query history command.

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.

@angelapwen angelapwen requested a review from a team as a code owner May 16, 2022 20:55
})

.catch(err => {
void logger.log(`Failed to generate structured evaluator log summary. Reason: ${err.message}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use showAndLogWarningMessage here so that the error is displayed in a pop-up as well as being logged?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I think that makes sense. The user might not know to look at the Extension Log to see this error pop up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it potentially be a showAndLogErrorMessage rather than a warning?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think because all the other related popups in this realm are warning messages I'll keep it as a warning message.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm inclined to agree that everything in this code should be a warning since failures here aren't fatal in the sense they don't result in a failure to produce results for the query which is what most users care about.

Copy link
Copy Markdown
Contributor

@edoardopirovano edoardopirovano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, and a surprisingly simple fix! Thanks for picking this up quickly 🙂

Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also approving (on behalf of the -reviewers team 😎 )

The Windows test failure looks like a flake, so maybe try re-running and it might disappear 😅

@angelapwen angelapwen merged commit 619c485 into github:main May 17, 2022
@angelapwen angelapwen deleted the unblock-log-summary-results branch May 17, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants