Skip to content

Use databricks CLI log-file option to capture the logs#923

Merged
ilia-db merged 7 commits intomainfrom
cli-log-to-file
Nov 3, 2023
Merged

Use databricks CLI log-file option to capture the logs#923
ilia-db merged 7 commits intomainfrom
cli-log-to-file

Conversation

@ilia-db
Copy link
Copy Markdown
Contributor

@ilia-db ilia-db commented Oct 31, 2023 •
edited
Loading

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

@ilia-db
Use databricks CLI log-file option to capture the logs
b9b39d2 
The CLI can write its logs to a file and we don't have to manage this on the
extension side
@ilia-db ilia-db temporarily deployed to azure-prod-usr October 31, 2023 10:32 — with GitHub Actions Inactive
kartikgupta-db
kartikgupta-db reviewed Oct 31, 2023
View reviewed changes
@ilia-db ilia-db temporarily deployed to azure-prod-usr October 31, 2023 13:51 — with GitHub Actions Inactive
@ilia-db ilia-db temporarily deployed to azure-prod-usr October 31, 2023 14:00 — with GitHub Actions Inactive
@ilia-db ilia-db temporarily deployed to azure-prod-usr October 31, 2023 14:10 — with GitHub Actions Inactive
@ilia-db ilia-db temporarily deployed to azure-prod-usr October 31, 2023 14:10 — with GitHub Actions Inactive
@ilia-db ilia-db temporarily deployed to azure-prod-usr October 31, 2023 15:33 — with GitHub Actions Inactive
@ilia-db ilia-db temporarily deployed to azure-prod-usr October 31, 2023 15:33 — with GitHub Actions Inactive
@ilia-db ilia-db marked this pull request as ready for review October 31, 2023 15:33
kartikgupta-db
kartikgupta-db reviewed Oct 31, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@kartikgupta-db kartikgupta-db left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! Just some nits.

We also need to inform @PaulCornellDB to get our docs updated to say we do not support this configuration going forward.

pietern
pietern reviewed Nov 1, 2023
View reviewed changes
packages/databricks-vscode/src/cli/CliWrapper.test.ts
);

const syncCommand = `${cliPath} sync . /Repos/[email protected]/project --watch --output json`;
const loggingArgs = `--log-level debug --log-file ${logFilePath} --log-format json`;
Copy link
Copy Markdown
Contributor

@pietern pietern Nov 1, 2023

Choose a reason for hiding this comment

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

You can set these via environment variables. Then you don't need to worry about paths with spaces, etc.

Here's the list: https://github.com/databricks/cli/blob/d70d7445c4f8f873959dd45ae68f8c42617d9353/cmd/root/logger.go#L17

Copy link
Copy Markdown
Contributor Author

@ilia-db ilia-db Nov 1, 2023

Choose a reason for hiding this comment

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

I went with the arguments for the consistency sake, as we use them for all other CLI options (including paths for sync destination, which can also contain spaces). As I understand paths with spaces shouldn't be a concern, as nodejs handles normalization for us, plus we don't spawn the process in a shell, so normalising shouldn't be necessary. I haven't tested this on Windows though, will try to double check

Copy link
Copy Markdown
Contributor

@pietern pietern Nov 2, 2023

Choose a reason for hiding this comment

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

Understood, I didn't see the array in loggingArguments and thought the args were built through string interpolation. Would be great to double check that everything works on Windows.

Copy link
Copy Markdown
Contributor Author

@ilia-db ilia-db Nov 3, 2023

Choose a reason for hiding this comment

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

Checked on windows, no problems for paths with spaces
Screenshot 2023-11-03 at 11 59 14

@ilia-db ilia-db merged commit 18283bb into main Nov 3, 2023
@ilia-db ilia-db mentioned this pull request Nov 6, 2023
Merged
@github-actions github-actions bot mentioned this pull request Nov 6, 2023
Merged
github-merge-queue bot pushed a commit to databricks/cli that referenced this pull request Nov 8, 2023
@ilia-db
Log process ID in each log entry (#949)
65dd9c5 
## Changes
This will help differentiate multiple cli commands that write to the
same log file.
Noticed that the root module wasn't using the common log utilities,
refactored it to avoid missing log arguments.
Relevant PR on the databricks vscode extension side:
databricks/databricks-vscode#923

## Tests
Tested manually for sdk and cli loggers
kartikgupta-db added a commit that referenced this pull request Nov 13, 2023
@github-actions @web-flow @kartikgupta-db
Release: v1.2.3 (#930)
cc61280 
## packages/databricks-vscode
## <small>1.2.3 (2023-11-06)</small>

* Make configuration wizard sticky (#925)
([7a4fb31](7a4fb31)),
closes
[#925](#925)
* Refactor `StateStore` to make keys more explicit at the point of use.
(#913)
([5b8fb23](5b8fb23)),
closes
[#913](#913)
* Use databricks CLI log-file option to capture the logs (#923)
([18283bb](18283bb)),
closes
[#923](#923)



## packages/databricks-vscode-types
## <small>1.2.3 (2023-11-06)</small>

---------

Co-authored-by: releasebot <[email protected]>
Co-authored-by: Kartik Gupta <[email protected]>
Co-authored-by: kartikgupta-db <[email protected]>
@ilia-db ilia-db deleted the cli-log-to-file branch January 23, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pietern pietern pietern left review comments

@kartikgupta-db kartikgupta-db kartikgupta-db approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ilia-db @pietern @kartikgupta-db