Skip to content

Commit 18283bb

Browse files
ilia-dbilia-db
authored
Use databricks CLI log-file option to capture the logs (#923)
## Changes The CLI can write its logs to a file and we don't have to manage this on the extension side. - The format of the logs is a bit different now - In the verbose mode we were writing debug logs to the terminal task in the VSCode (as they were coming to the stderr). Right now such logs are only in the log file, and output has just the stdout (as CLI keeps stderr empty when you ask it to log to a file) - The verbose mode option is gone. The output in the sync terminal is clean and always based on stdout, and the verbose logs are always written into a file ## Tests Updated existing tests, added new unit tests
1 parent 7a4fb31 commit 18283bb

File tree

9 files changed

+109
-131
lines changed

9 files changed

+109
-131
lines changed

‎packages/databricks-vscode/DATABRICKS.quickstart.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@ This extension contributes the following settings:
7575
- `databricks.logs.maxArrayLength`: The maximum number of items to show for array fields
7676
- `databricks.logs.enabled`: Enable/disable logging. Reload window for changes to take effect
7777
- `databricks.clusters.onlyShowAccessibleClusters`: Only show clusters that the user has access to
78-
- `databricks.cli.verboseMode`: Show debug logs for the sync command
7978

8079
## <a id="commands"></a>`Databricks:` Commands
8180

‎packages/databricks-vscode/package.json‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -586,11 +586,6 @@
586586
"default": false,
587587
"description": "Enable/disable filtering for only accessible clusters (clusters on which the current user can run code)"
588588
},
589-
"databricks.cli.verboseMode": {
590-
"type": "boolean",
591-
"default": false,
592-
"description": "Enable verbose logging for databricks CLI (sync mode)."
593-
},
594589
"databricks.sync.destinationType": {
595590
"type": "string",
596591
"default": "workspace",

‎packages/databricks-vscode/src/cli/CliWrapper.test.ts‎

Lines changed: 64 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -5,62 +5,100 @@ import {
55
RemoteUri,
66
SyncDestinationMapper,
77
} from "../sync/SyncDestination";
8+
import {workspaceConfigs} from "../vscode-objs/WorkspaceConfigs";
89
import {promisify} from "node:util";
910
import {execFile as execFileCb} from "node:child_process";
1011
import {withFile} from "tmp-promise";
11-
import {writeFile} from "node:fs/promises";
12-
12+
import {writeFile, readFile} from "node:fs/promises";
13+
import {when, spy, reset} from "ts-mockito";
1314
import {CliWrapper} from "./CliWrapper";
1415
import path from "node:path";
16+
import os from "node:os";
17+
import crypto from "node:crypto";
1518
import {Context} from "@databricks/databricks-sdk/dist/context";
1619
import {logging} from "@databricks/databricks-sdk";
1720

1821
const execFile = promisify(execFileCb);
22+
const cliPath = path.join(__dirname, "../../bin/databricks");
23+
24+
function getTempLogFilePath() {
25+
return path.join(
26+
os.tmpdir(),
27+
`databricks-cli-logs-${crypto.randomUUID()}.json`
28+
);
29+
}
30+
31+
function createCliWrapper(logFilePath?: string) {
32+
return new CliWrapper(
33+
{
34+
asAbsolutePath(relativePath: string) {
35+
return path.join(__dirname, "../..", relativePath);
36+
},
37+
} as any,
38+
logFilePath
39+
);
40+
}
1941

2042
describe(__filename, () => {
2143
it("should embed a working databricks CLI", async () => {
22-
const cliPath = __dirname + "/../../bin/databricks";
2344
const result = await execFile(cliPath, ["--help"]);
2445
assert.ok(result.stdout.indexOf("databricks") > 0);
2546
});
2647

48+
let mocks: any[] = [];
49+
afterEach(() => {
50+
mocks.forEach((mock) => reset(mock));
51+
mocks = [];
52+
});
53+
54+
it("should tell CLI to log its output to a file", async () => {
55+
const logFilePath = getTempLogFilePath();
56+
const configsSpy = spy(workspaceConfigs);
57+
mocks.push(configsSpy);
58+
when(configsSpy.loggingEnabled).thenReturn(true);
59+
const cli = createCliWrapper(logFilePath);
60+
await execFile(cli.cliPath, ["version", ...cli.loggingArguments]);
61+
const file = await readFile(logFilePath);
62+
// Just checking if the file is not empty to avoid depending on internal CLI log patterns
63+
assert.ok(file.toString().length > 0);
64+
});
65+
2766
it("should create sync commands", async () => {
28-
const cli = new CliWrapper({
29-
asAbsolutePath(path: string) {
30-
return path;
31-
},
32-
} as any);
67+
const logFilePath = getTempLogFilePath();
68+
const cli = createCliWrapper(logFilePath);
3369
const mapper = new SyncDestinationMapper(
34-
new LocalUri(
35-
Uri.file("/Users/fabian.jakobs/Desktop/notebook-best-practices")
36-
),
70+
new LocalUri(Uri.file("/user/project")),
3771
new RemoteUri(
3872
Uri.from({
3973
scheme: "wsfs",
40-
path: "/Repos/fabian.jakobs@databricks.com/notebook-best-practices",
74+
path: "/Repos/user@databricks.com/project",
4175
})
4276
)
4377
);
4478

79+
const syncCommand = `${cliPath} sync . /Repos/user@databricks.com/project --watch --output json`;
80+
const loggingArgs = `--log-level debug --log-file ${logFilePath} --log-format json`;
4581
let {command, args} = cli.getSyncCommand(mapper, "incremental");
4682
assert.equal(
4783
[command, ...args].join(" "),
48-
"./bin/databricks sync . /Repos/fabian.jakobs@databricks.com/notebook-best-practices --watch --output json"
84+
[syncCommand, loggingArgs].join(" ")
4985
);
5086

5187
({command, args} = cli.getSyncCommand(mapper, "full"));
5288
assert.equal(
5389
[command, ...args].join(" "),
54-
"./bin/databricks sync . /Repos/fabian.jakobs@databricks.com/notebook-best-practices --watch --output json --full"
90+
[syncCommand, loggingArgs, "--full"].join(" ")
5591
);
92+
93+
const configsSpy = spy(workspaceConfigs);
94+
mocks.push(configsSpy);
95+
when(configsSpy.loggingEnabled).thenReturn(false);
96+
({command, args} = cli.getSyncCommand(mapper, "incremental"));
97+
assert.equal([command, ...args].join(" "), syncCommand);
5698
});
5799

58100
it("should create an 'add profile' command", () => {
59-
const cli = new CliWrapper({
60-
asAbsolutePath(path: string) {
61-
return path;
62-
},
63-
} as any);
101+
const cli = createCliWrapper();
64102

65103
const {command, args} = cli.getAddProfileCommand(
66104
"DEFAULT",
@@ -69,27 +107,20 @@ describe(__filename, () => {
69107

70108
assert.equal(
71109
[command, ...args].join(" "),
72-
"./bin/databricks configure --no-interactive --profile DEFAULT --host https://databricks.com/ --token"
110+
`${cliPath} configure --no-interactive --profile DEFAULT --host https://databricks.com/ --token`
73111
);
74112
});
75113

76114
it("should list profiles when no config file exists", async () => {
77-
const cli = new CliWrapper({
78-
asAbsolutePath(p: string) {
79-
return path.join(__dirname, "..", "..", p);
80-
},
81-
} as any);
82-
115+
const logFilePath = getTempLogFilePath();
116+
const cli = createCliWrapper(logFilePath);
83117
const profiles = await cli.listProfiles("/tmp/does-not-exist");
84118
assert.equal(profiles.length, 0);
85119
});
86120

87121
it("should list profiles", async function () {
88-
const cli = new CliWrapper({
89-
asAbsolutePath(p: string) {
90-
return path.join(__dirname, "..", "..", p);
91-
},
92-
} as any);
122+
const logFilePath = getTempLogFilePath();
123+
const cli = createCliWrapper(logFilePath);
93124

94125
await withFile(async ({path}) => {
95126
await writeFile(
@@ -121,11 +152,8 @@ describe(__filename, () => {
121152
});
122153

123154
it("should load all valid profiles and return errors for rest", async () => {
124-
const cli = new CliWrapper({
125-
asAbsolutePath(p: string) {
126-
return path.join(__dirname, "..", "..", p);
127-
},
128-
} as any);
155+
const logFilePath = getTempLogFilePath();
156+
const cli = createCliWrapper(logFilePath);
129157

130158
await withFile(async ({path}) => {
131159
await writeFile(

‎packages/databricks-vscode/src/cli/CliWrapper.ts‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,29 @@ function getValidHost(host: string) {
4141
* of the databricks CLI
4242
*/
4343
export class CliWrapper {
44-
constructor(private extensionContext: ExtensionContext) {}
44+
constructor(
45+
private extensionContext: ExtensionContext,
46+
private logFilePath?: string
47+
) {}
4548

4649
get cliPath(): string {
4750
return this.extensionContext.asAbsolutePath("./bin/databricks");
4851
}
4952

53+
get loggingArguments(): string[] {
54+
if (!workspaceConfigs.loggingEnabled) {
55+
return [];
56+
}
57+
return [
58+
"--log-level",
59+
"debug",
60+
"--log-file",
61+
this.logFilePath ?? "stderr",
62+
"--log-format",
63+
"json",
64+
];
65+
}
66+
5067
/**
5168
* Constructs the databricks sync command
5269
*/
@@ -61,20 +78,23 @@ export class CliWrapper {
6178
"--watch",
6279
"--output",
6380
"json",
81+
...this.loggingArguments,
6482
];
6583
if (syncType === "full") {
6684
args.push("--full");
6785
}
68-
if (workspaceConfigs.cliVerboseMode) {
69-
args.push("--log-level", "debug", "--log-file", "stderr");
70-
}
7186
return {command: this.cliPath, args};
7287
}
7388

7489
private getListProfilesCommand(): Command {
7590
return {
7691
command: this.cliPath,
77-
args: ["auth", "profiles", "--skip-validate"],
92+
args: [
93+
"auth",
94+
"profiles",
95+
"--skip-validate",
96+
...this.loggingArguments,
97+
],
7898
};
7999
}
80100

@@ -83,7 +103,7 @@ export class CliWrapper {
83103
configfilePath?: string,
84104
@context ctx?: Context
85105
): Promise<Array<ConfigEntry>> {
86-
const cmd = await this.getListProfilesCommand();
106+
const cmd = this.getListProfilesCommand();
87107
const res = await execFile(cmd.command, cmd.args, {
88108
env: {
89109
/* eslint-disable @typescript-eslint/naming-convention */

‎packages/databricks-vscode/src/cli/DatabricksCliSyncParser.ts‎

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,6 @@ import {EventEmitter} from "vscode";
33
import {Loggers} from "../logger";
44
import {SyncState} from "../sync";
55

6-
const databricksLogLevelToSdk = new Map<string, logging.LEVELS>([
7-
["DEBUG", logging.LEVELS.debug],
8-
["INFO", logging.LEVELS.info],
9-
["WARN", logging.LEVELS.warn],
10-
["ERROR", logging.LEVELS.error],
11-
]);
12-
136
type EventBase = {
147
timestamp: string;
158
seq: number;
@@ -96,28 +89,11 @@ export class DatabricksCliSyncParser {
9689

9790
public processStderr(data: string) {
9891
const logLines = data.split("\n");
99-
let currentLogLevel: logging.LEVELS = logging.LEVELS.info;
10092
for (let i = 0; i < logLines.length; i++) {
10193
const line = logLines[i].trim();
10294
if (line.length === 0) {
10395
continue;
10496
}
105-
106-
const typeMatch = line.match(
107-
/[0-9]+(?:\/[0-9]+)+ [0-9]+(?::[0-9]+)+ \[(.+)\]/
108-
);
109-
if (typeMatch) {
110-
currentLogLevel =
111-
databricksLogLevelToSdk.get(typeMatch[1]) ??
112-
currentLogLevel;
113-
}
114-
logging.NamedLogger.getOrCreate(Loggers.CLI).log(
115-
currentLogLevel,
116-
line,
117-
{
118-
outfile: "stderr",
119-
}
120-
);
12197
this.writeEmitter.fire(line.trim() + "\r\n");
12298
if (this.matchForErrors(line)) {
12399
return;
@@ -154,9 +130,6 @@ export class DatabricksCliSyncParser {
154130
if (line.length === 0) {
155131
continue;
156132
}
157-
logging.NamedLogger.getOrCreate(Loggers.CLI).info(line, {
158-
outfile: "stdout",
159-
});
160133

161134
try {
162135
this.processLine(line);

‎packages/databricks-vscode/src/extension.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,16 @@ export async function activate(
143143
);
144144

145145
// Configuration group
146-
const cli = new CliWrapper(context);
146+
let cliLogFilePath;
147+
try {
148+
cliLogFilePath = await loggerManager.getLogFile("databricks-cli");
149+
} catch (e) {
150+
logging.NamedLogger.getOrCreate(Loggers.Extension).error(
151+
"Failed to create a log file for the CLI",
152+
e
153+
);
154+
}
155+
const cli = new CliWrapper(context, cliLogFilePath);
147156
const connectionManager = new ConnectionManager(cli, stateStorage);
148157
context.subscriptions.push(
149158
connectionManager.onDidChangeState(async (state) => {

‎packages/databricks-vscode/src/logger/LoggerManager.test.ts‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,25 +26,19 @@ describe(__filename, function () {
2626
logging.NamedLogger.getOrCreate(Loggers.Extension).debug(
2727
"test message"
2828
);
29-
logging.NamedLogger.getOrCreate(Loggers.CLI).debug("test message");
3029

3130
await new Promise((resolve) =>
3231
setTimeout(
3332
resolve,
3433
new Time(0.5, TimeUnits.seconds).toMillSeconds().value
3534
)
3635
);
37-
["sdk-and-extension-logs.json", "databricks-cli-logs.json"].forEach(
38-
async (logfile) => {
39-
const rawLogs = await readFile(path.join(tempDir, logfile), {
40-
encoding: "utf-8",
41-
});
42-
43-
const logs = rawLogs.split("\n");
44-
assert.ok(logs.length !== 0);
45-
assert.ok(logs[0].includes("test message"));
46-
}
47-
);
36+
const logfile = path.join(tempDir, "sdk-and-extension-logs.json");
37+
const rawLogs = await readFile(logfile, {encoding: "utf-8"});
38+
39+
const logs = rawLogs.split("\n");
40+
assert.ok(logs.length !== 0);
41+
assert.ok(logs[0].includes("test message"));
4842
});
4943

5044
afterEach(async () => {

0 commit comments

Comments
 (0)