Print end-of-query summary logs to Query Server Console#1264
Print end-of-query summary logs to Query Server Console#1264angelapwen merged 9 commits intogithub:mainfrom
Conversation
| } | ||
|
|
||
| export function findQueryEvalLogSummaryFile(resultPath: string): string { | ||
| return path.join(resultPath, 'evaluator-log.jsonl.summary'); |
There was a problem hiding this comment.
Is this a valid json or jsonl file? If so, it should probably use the proper file extension. Maybe:
| return path.join(resultPath, 'evaluator-log.jsonl.summary'); | |
| return path.join(resultPath, 'evaluator-log.summary.jsonl'); |
There was a problem hiding this comment.
It's not — it's a generated text file summarizing the contents of the jsonl file.
There was a problem hiding this comment.
OK...then maybe avoid the jsonl component entirely?
There was a problem hiding this comment.
That seems agreeable to me, I'll hold to see if @edoardopirovano who named the summary file originally has objections 😸
There was a problem hiding this comment.
Just calling it .summary seems perfectly reasonable to me, yep!
| } | ||
|
|
||
| export function findQueryEvalLogEndSummaryFile(resultPath: string): string { | ||
| return path.join(resultPath, 'eoq-evaluator-log.jsonl.summary'); |
There was a problem hiding this comment.
Also here:
| return path.join(resultPath, 'eoq-evaluator-log.jsonl.summary'); | |
| return path.join(resultPath, 'eoq-evaluator-log.summary.jsonl'); |
There was a problem hiding this comment.
Same as above, it's not a JSONL file but rather a condensed version of the summary text file. I didn't know how to name it so appended eoq to the beginning of the file rather than at the end, but
evaluator-log.jsonl.summary.eoq might also work?
There was a problem hiding this comment.
Maybe evaluator-log-end.summary? As I understand it, this is only used internally so I don't suppose it matters too much.
There was a problem hiding this comment.
I like this one now that the jsonl part is gone! Very succinct
| if (err) { | ||
| throw new Error(`Could not read structured evaluator log end of summary file at ${this.evalLogEndSummaryPath}.`); | ||
| } | ||
| void qs.logger.log(' --- Evaluator Log Summary --- ', { additionalLogLocation: this.logPath }); |
There was a problem hiding this comment.
I think this will add the ' --- Evaluator Log Summary --- ' to the file. Is this what we want? Is it better/possible to ensure the output is proper json or jsonl?
There was a problem hiding this comment.
This adds the ' --- Evaluator Log Summary --- ' line to the Query Server Console, which I believe is what we want. The file itself (the end summary file) should never be read by a user; it's generated on the CLI side specifically for this use case.
There was a problem hiding this comment.
The additionalLogLocation will add the contents to this.logPath. If it's not meant to be read by a user and it is not jsonl syntax then adding this line to it as well is fine.
There was a problem hiding this comment.
Hm, the user is meant to read this line (and the following lines) only from the Query Server Console. I can remove the additionalLogLocation part of the logging so that the user will not see traces of the evaluator structured logs in the additional log where they may not expect it. If they want the full summary of the structured log, they can view the actual summary file.
There was a problem hiding this comment.
You are much more familiar with this feature than I am. I am fine with it if you decide to leave this in.
There was a problem hiding this comment.
I think dropping the additionalLogLocation so it still shows in the Query Server Console but doesn't get logged elsewhere sounds reasonable to me. I see you've already done this in your new commit 👍🏼
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
…odeql into log-end-summary-file
edoardopirovano
left a comment
There was a problem hiding this comment.
Looking good, thanks! I'm happy with this at it is, modulo you may want some simpler names for the files as discussed above 🙂
This change passes the
--end-summary=$eoq-evaluator-log.jsonl.summaryoption to the CLI so that a separate file in the given location is written, with only the end-of-query summary information (most expensive predicate and timings). It then directly logs the contents of this file to the query server console.It also changes the CLI version we gate all evaluator logs behind to 2.9.0 for simplicity. Otherwise, we would need to pass in one set of arguments to the CLI for 2.8.4 and another for 2.9.0. If in the future a user must be on a 2.8.x release and would like evaluator logs, we will be able to modify this for them easily.
This has been tested locally on VSCode (by modifying the CLI version expected to 2.8.3 and using the main checkout of the CLI) and works as we expect:
Checklist
ready-for-doc-reviewlabel there.