Skip to content

[DECO-239] Catch login errors #186

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

Merged
kartikgupta-db merged 1 commit into from
Nov 9, 2022
Merged

[DECO-239] Catch login errors #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion packages/databricks-sdk-js/src/logging/NamedLogger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,16 @@ export class NamedLogger {
}

error(message?: string, obj?: any) {
this.log(LEVELS.error, message, obj);
if (Object(obj) === obj) {
obj = {
...Object.getOwnPropertyNames(obj).reduce((acc, i) => {
acc[i] = (obj as any)[i];
return acc;
}, {} as any),
...(obj as any),
};
}
this.log(LEVELS.error, message, {error: obj});
}

withContext<T>({
Expand Down
115 changes: 81 additions & 34 deletions packages/databricks-vscode/src/configuration/ConnectionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {selectProfile} from "./selectProfileWizard";
import {ClusterManager} from "../cluster/ClusterManager";
import {workspace} from "@databricks/databricks-sdk";
import {DatabricksWorkspace} from "./DatabricksWorkspace";
import {NamedLogger} from "@databricks/databricks-sdk/dist/logging";

const extensionVersion = require("../../package.json").version;

Expand Down Expand Up @@ -94,6 +95,18 @@ export class ConnectionManager {
}

async login(interactive: boolean = false): Promise<void> {
try {
await this._login(interactive);
} catch (e) {
NamedLogger.getOrCreate("Extension").error("Login Error", e);

@fjakobs fjakobs Nov 9, 2022

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'm generally not a fan of this global factory/singleton pattern. I'd rather have the logger be passed in and if that's to tedious use dependency injection.

It's not blocking this PR but we should address it in a separate PR.

@kartikgupta-db kartikgupta-db Nov 9, 2022

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.

Integrating DI with decorators in SDK was complicated.

The NamedLogger class is our custom wrapper around any standard logger. This factory creates a new instance of NamedLogger for but uses lookup to fetch the centrally configured internal logger. So in a way it behaves like a DI container (we need to explicitly ask it to create a new instance of internal logger).

Once context becomes more prevalent, it should be easier to address this issue.

if (interactive) {
window.showErrorMessage(`Login error ${JSON.stringify(e)}`);
}
this.updateState("DISCONNECTED");
await this.logout();
}
}
private async _login(interactive: boolean = false): Promise<void> {
await this.logout();
this.updateState("CONNECTING");

Expand Down Expand Up @@ -124,7 +137,7 @@ export class ConnectionManager {
);
} catch (e: any) {
const message = `Can't login to Databricks: ${e.message}`;
console.error(message);
NamedLogger.getOrCreate("Extension").error(message, e);
if (interactive) {
window.showErrorMessage(message);
}
Expand All @@ -146,7 +159,7 @@ export class ConnectionManager {
message =
"Files in Repos is not enabled for this workspace. Please enable it in the Databricks UI.";
}
console.error(message);
NamedLogger.getOrCreate("Extension").error(message);
if (interactive) {
let result = await window.showWarningMessage(
message,
Expand Down Expand Up @@ -230,7 +243,11 @@ export class ConnectionManager {
profile
);
} catch (e: any) {
console.error(e);
NamedLogger.getOrCreate("Extension").error(
`Connection with profile "${profile}" failed`,
e
);

const response = await window.showWarningMessage(
`Connection with profile "${profile}" failed with error: "${e.message}"."`,
"Retry",
Expand Down Expand Up @@ -277,24 +294,40 @@ export class ConnectionManager {
cluster: Cluster | string,
skipWrite = false
): Promise<void> {
if (this.cluster === cluster) {
return;
}
try {
if (this.cluster === cluster) {
return;
}

if (typeof cluster === "string") {
cluster = await Cluster.fromClusterId(this._apiClient!, cluster);
}
if (typeof cluster === "string") {
cluster = await Cluster.fromClusterId(
this._apiClient!,
cluster
);
}

if (!skipWrite) {
this._projectConfigFile!.clusterId = cluster.id;
await this._projectConfigFile!.write();
}
if (!skipWrite) {
this._projectConfigFile!.clusterId = cluster.id;
await this._projectConfigFile!.write();
}

this.updateCluster(cluster);
this.updateCluster(cluster);
} catch (e) {
NamedLogger.getOrCreate("Extension").error(
"Attach Cluster error",
e
);
window.showErrorMessage(
`Error in attaching cluster destination ${
typeof cluster === "string" ? cluster : cluster.id
}`
);
await this.detachCluster();
}
}

async detachCluster(): Promise<void> {
if (!this.cluster) {
if (!this.cluster && this._projectConfigFile?.clusterId === undefined) {
return;
}

Expand All @@ -310,32 +343,46 @@ export class ConnectionManager {
workspacePath: Uri,
skipWrite = false
): Promise<void> {
if (
!vscodeWorkspace.workspaceFolders ||
!vscodeWorkspace.workspaceFolders.length
) {
// TODO how do we handle this?
return;
}
try {
if (
!vscodeWorkspace.workspaceFolders ||
!vscodeWorkspace.workspaceFolders.length
) {
// TODO how do we handle this?
return;
}

if (!skipWrite) {
this._projectConfigFile!.workspacePath = workspacePath.path;
await this._projectConfigFile!.write();
}
if (!skipWrite) {
this._projectConfigFile!.workspacePath = workspacePath.path;
await this._projectConfigFile!.write();
}

const wsUri = vscodeWorkspace.workspaceFolders[0].uri;
if (this.apiClient === undefined) {
throw new Error(
"Can't attach a Repo when profile is not connected"
const wsUri = vscodeWorkspace.workspaceFolders[0].uri;
if (this.apiClient === undefined) {
throw new Error(
"Can't attach a Repo when profile is not connected"
);
}
this.updateSyncDestination(
await SyncDestination.from(this.apiClient, workspacePath, wsUri)
);
} catch (e) {
NamedLogger.getOrCreate("Extension").error(
"Attach Sync Destination error",
e
);
window.showErrorMessage(
`Error in attaching sync destination ${workspacePath.fsPath}`
);
await this.detachSyncDestination();
}
this.updateSyncDestination(
await SyncDestination.from(this.apiClient, workspacePath, wsUri)
);
}

async detachSyncDestination(): Promise<void> {
if (!this._syncDestination) {
if (
!this._syncDestination &&
this._projectConfigFile?.workspacePath === undefined
) {
return;
}

Expand Down
10 changes: 8 additions & 2 deletions packages/databricks-vscode/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {showQuickStartOnFirstUse} from "./quickstart/QuickStart";
import {PublicApi} from "@databricks/databricks-vscode-types";
import {initLoggers} from "./logger";
import {UtilsCommands} from "./utils/UtilsCommands";
import {NamedLogger} from "@databricks/databricks-sdk/dist/logging";

export function activate(context: ExtensionContext): PublicApi | undefined {
const a = workspace.workspaceFolders;
Expand All @@ -47,7 +48,6 @@ export function activate(context: ExtensionContext): PublicApi | undefined {
let cli = new CliWrapper(context);
// Configuration group
let connectionManager = new ConnectionManager(cli);
connectionManager.login(false);

const synchronizer = new CodeSynchronizer(connectionManager, cli);
const clusterModel = new ClusterModel(connectionManager);
Expand Down Expand Up @@ -226,7 +226,9 @@ export function activate(context: ExtensionContext): PublicApi | undefined {
)
);

showQuickStartOnFirstUse(context);
showQuickStartOnFirstUse(context).catch((e) => {
NamedLogger.getOrCreate("Extension").error("Quick Start error", e);
});

//utils
const utilCommands = new UtilsCommands();
Expand All @@ -238,6 +240,10 @@ export function activate(context: ExtensionContext): PublicApi | undefined {
)
);

connectionManager.login(false).catch((e) => {
NamedLogger.getOrCreate("Extension").error("Login error", e);
});

return {
connectionManager: connectionManager,
};
Expand Down
28 changes: 14 additions & 14 deletions packages/databricks-vscode/src/logger/outputConsoleTransport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,26 +58,26 @@ export function getOutputConsoleTransport(outputChannel: OutputChannel) {
return new transports.Stream({
format: format((info: any) => {
const stripped = Object.assign({}, info) as any;
if (stripped[LEVEL] === "error") {
return info;
}
delete stripped[LEVEL];
delete stripped[MESSAGE];
delete stripped[SPLAT];
delete stripped["level"];
delete stripped["message"];

info[MESSAGE] = inspect(
{
...recursiveTruncate(
stripped,
workspaceConfigs.truncationDepth
),
timestamp: new Date().toLocaleString(),
},
false,
workspaceConfigs.truncationDepth
);
info[MESSAGE] =
info.level === "error"
? inspect(stripped, false, 1000)
: inspect(
{
...recursiveTruncate(
stripped,
workspaceConfigs.truncationDepth
),
timestamp: new Date().toLocaleString(),
},
false,
workspaceConfigs.truncationDepth
);
return info;
})(),
stream: new OutputConsoleStream(outputChannel, {
Expand Down