Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor: Store EvaluatorLogPaths object on LocalQueryInfo #3803

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Refactor: Store EvaluatorLogPaths object on LocalQueryInfo #3803

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e146e7a
Refactor: Store EvaluatorLogPaths object on LocalQueryInfo
asgerf Nov 12, 2024
fa3e3ff
Initialise object even if evaluator log paths were omitted
asgerf Nov 13, 2024
4e32a10
Make 'log' field non-nullable again.
asgerf Nov 19, 2024
eee5939
Fix typo in 'evaluatorLogPaths'
asgerf Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions extensions/ql-vscode/src/log-insights/log-scanner-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,19 @@ export class LogScannerService extends DisposableObject {
public async scanEvalLog(query: QueryHistoryInfo | undefined): Promise<void> {
this.diagnosticCollection.clear();

if (
query?.t !== "local" ||
query.evalLogSummaryLocation === undefined ||
query.jsonEvalLogSummaryLocation === undefined
) {
if (query?.t !== "local" || query.evalutorLogPaths === undefined) {
return;
}

const diagnostics = await this.scanLog(
query.jsonEvalLogSummaryLocation,
query.evalLogSummarySymbolsLocation,
);
const uri = Uri.file(query.evalLogSummaryLocation);
const { summarySymbols, jsonSummary, humanReadableSummary } =
query.evalutorLogPaths;

if (jsonSummary === undefined || humanReadableSummary === undefined) {
return;
}

const diagnostics = await this.scanLog(jsonSummary, summarySymbols);
const uri = Uri.file(humanReadableSummary);
this.diagnosticCollection.set(uri, diagnostics);
}

Expand Down
17 changes: 10 additions & 7 deletions extensions/ql-vscode/src/query-history/query-history-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ export class QueryHistoryManager extends DisposableObject {

private async warnNoEvalLogSummary(item: LocalQueryInfo) {
const evalLogLocation =
item.evalLogLocation ?? item.initialInfo.outputDir?.evalLogPath;
item.evalutorLogPaths?.log ?? item.initialInfo.outputDir?.evalLogPath;

// Summary log file doesn't exist.
if (evalLogLocation && (await pathExists(evalLogLocation))) {
Expand All @@ -801,7 +801,7 @@ export class QueryHistoryManager extends DisposableObject {
}

const evalLogLocation =
item.evalLogLocation ?? item.initialInfo.outputDir?.evalLogPath;
item.evalutorLogPaths?.log ?? item.initialInfo.outputDir?.evalLogPath;

if (evalLogLocation && (await pathExists(evalLogLocation))) {
await tryOpenExternalFile(this.app.commands, evalLogLocation);
Expand All @@ -816,12 +816,15 @@ export class QueryHistoryManager extends DisposableObject {
}

// If the summary file location wasn't saved, display error
if (!item.evalLogSummaryLocation) {
if (!item.evalutorLogPaths?.humanReadableSummary) {
await this.warnNoEvalLogSummary(item);
return;
}

await tryOpenExternalFile(this.app.commands, item.evalLogSummaryLocation);
await tryOpenExternalFile(
this.app.commands,
item.evalutorLogPaths.humanReadableSummary,
);
}

async handleShowEvalLogViewer(item: QueryHistoryInfo) {
Expand All @@ -830,15 +833,15 @@ export class QueryHistoryManager extends DisposableObject {
}

// If the JSON summary file location wasn't saved, display error
if (item.jsonEvalLogSummaryLocation === undefined) {
if (item.evalutorLogPaths?.jsonSummary === undefined) {
await this.warnNoEvalLogSummary(item);
return;
}

// TODO(angelapwen): Stream the file in.
try {
const evalLogData: EvalLogData[] = await parseViewerData(
item.jsonEvalLogSummaryLocation,
item.evalutorLogPaths.jsonSummary,
);
const evalLogTreeBuilder = new EvalLogTreeBuilder(
item.getQueryName(),
Expand All @@ -847,7 +850,7 @@ export class QueryHistoryManager extends DisposableObject {
this.evalLogViewer.updateRoots(await evalLogTreeBuilder.getRoots());
} catch {
throw new Error(
`Could not read evaluator log summary JSON file to generate viewer data at ${item.jsonEvalLogSummaryLocation}.`,
`Could not read evaluator log summary JSON file to generate viewer data at ${item.evalutorLogPaths.jsonSummary}.`,
);
}
}
Expand Down
8 changes: 4 additions & 4 deletions extensions/ql-vscode/src/query-history/store/query-history-local-query-domain-mapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ export function mapLocalQueryInfoToDto(
return {
initialInfo: mapInitialQueryInfoToDto(query.initialInfo),
t: "local",
evalLogLocation: query.evalLogLocation,
evalLogSummaryLocation: query.evalLogSummaryLocation,
jsonEvalLogSummaryLocation: query.jsonEvalLogSummaryLocation,
evalLogSummarySymbolsLocation: query.evalLogSummarySymbolsLocation,
evalLogLocation: query.evalutorLogPaths?.log,
evalLogSummaryLocation: query.evalutorLogPaths?.humanReadableSummary,
jsonEvalLogSummaryLocation: query.evalutorLogPaths?.jsonSummary,
evalLogSummarySymbolsLocation: query.evalutorLogPaths?.summarySymbols,
failureReason: query.failureReason,
completedQuery:
query.completedQuery && mapCompletedQueryToDto(query.completedQuery),
Expand Down
11 changes: 7 additions & 4 deletions extensions/ql-vscode/src/query-history/store/query-history-local-query-dto-mapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@ export function mapLocalQueryItemToDomainModel(
localQuery.failureReason,
localQuery.completedQuery &&
mapCompletedQueryInfoToDomainModel(localQuery.completedQuery),
localQuery.evalLogLocation,
localQuery.evalLogSummaryLocation,
localQuery.jsonEvalLogSummaryLocation,
localQuery.evalLogSummarySymbolsLocation,
{
log: localQuery.evalLogLocation,
humanReadableSummary: localQuery.evalLogSummaryLocation,
jsonSummary: localQuery.jsonEvalLogSummaryLocation,
summarySymbols: localQuery.evalLogSummarySymbolsLocation,
endSummary: undefined,
},
Copy link
Member

Choose a reason for hiding this comment

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

Can we set this to undefined instead when the evalLogLocation is undefined?

);
}

Expand Down
16 changes: 8 additions & 8 deletions extensions/ql-vscode/src/query-results.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,13 @@ export class LocalQueryInfo {
private cancellationSource?: CancellationTokenSource, // used to cancel in progress queries
public failureReason?: string,
public completedQuery?: CompletedQueryInfo,
public evalLogLocation?: string,
public evalLogSummaryLocation?: string,
public jsonEvalLogSummaryLocation?: string,
public evalLogSummarySymbolsLocation?: string,
public evalutorLogPaths: EvaluatorLogPaths = {
log: undefined,
humanReadableSummary: undefined,
endSummary: undefined,
jsonSummary: undefined,
summarySymbols: undefined,
},
Copy link
Member

Choose a reason for hiding this comment

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

Could this be undefined by default instead? That would make the change for making log | undefined unnecessary as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense 👍

) {
/**/
}
Expand All @@ -229,10 +232,7 @@ export class LocalQueryInfo {

/** Sets the paths to the various structured evaluator logs. */
public setEvaluatorLogPaths(logPaths: EvaluatorLogPaths): void {
this.evalLogLocation = logPaths.log;
this.evalLogSummaryLocation = logPaths.humanReadableSummary;
this.jsonEvalLogSummaryLocation = logPaths.jsonSummary;
this.evalLogSummarySymbolsLocation = logPaths.summarySymbols;
this.evalutorLogPaths = logPaths;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.evalutorLogPaths = logPaths;
this.evaluatorLogPaths = logPaths;

}

/**
Expand Down
2 changes: 1 addition & 1 deletion extensions/ql-vscode/src/run-queries-shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import type { ProgressCallback } from "./common/vscode/progress";
* Holds the paths to the various structured log summary files generated for a query evaluation.
*/
export interface EvaluatorLogPaths {
log: string;
log: string | undefined;
humanReadableSummary: string | undefined;
endSummary: string | undefined;
jsonSummary: string | undefined;
Expand Down
Loading