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.

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

ruff server: Tracing system now respects log level and trace level, with options to log to a file #11747

Merged
merged 18 commits into from
Jun 11, 2024

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented Jun 5, 2024

Summary

Fixes #10968.
Fixes #11545.

The server's tracing system has been rewritten from the ground up. The server now has trace level and log level settings which restrict the tracing events and spans that get logged.

  • A logLevel setting has been added, which lets a user set the log level. By default, it is set to "info".
  • A logFile setting has also been added, which lets the user supply an optional file to send tracing output (it does not have to exist as a file yet). By default, if this is unset, tracing output will be sent to stderr.
  • A $/setTrace handler has also been added, and we also set the trace level from the initialization options. For editors without direct support for tracing, the environment variable RUFF_TRACE can override the trace level.
  • Small changes have been made to how we display tracing output. We no longer use tracing-tree, and instead use tracing_subscriber::fmt::Layer to format output. Thread names are now included in traces, and I've made some adjustment to thread worker names to be more useful.

Test Plan

In VS Code, with ruff.trace.server set to its default value, no logs from Ruff should appear.

After changing ruff.trace.server to either messages or verbose, you should see log messages at info level or higher appear in Ruff's output:
Screenshot 2024-06-10 at 10 35 04 AM

In Helix, by default, no logs from Ruff should appear.

To set the trace level in Helix, you'll need to modify your language configuration as follows:

[language-server.ruff]
command = "/Users/jane/astral/ruff/target/debug/ruff"
args = ["server", "--preview"]
environment = { "RUFF_TRACE" = "messages" }

After doing this, logs of info level or higher should be visible in Helix:
Screenshot 2024-06-10 at 10 39 26 AM

You can use :log-open to quickly open the Helix log file.

In Neovim, by default, no logs from Ruff should appear.

To set the trace level in Neovim, you'll need to modify your configuration as follows:

require('lspconfig').ruff.setup {
  cmd = {"/path/to/debug/executable", "server", "--preview"},
  cmd_env = { RUFF_TRACE = "messages" }
}

You should see logs appear in :LspLog that look like the following:
Screenshot 2024-06-11 at 11 24 01 AM

You can adjust logLevel and logFile in settings:

require('lspconfig').ruff.setup {
  cmd = {"/path/to/debug/executable", "server", "--preview"},
  cmd_env = { RUFF_TRACE = "messages" },
  settings = {
    logLevel = "debug",
    logFile = "your/log/file/path/log.txt"
  }
}

The logLevel and logFile can also be set in Helix like so:

[language-server.ruff.config.settings]
logLevel = "debug"
logFile = "your/log/file/path/log.txt"

Even if this log file does not exist, it should now be created and written to after running the server:

Screenshot 2024-06-10 at 10 43 44 AM

@snowsignal snowsignal added the server Related to the LSP server label Jun 5, 2024
Copy link
Contributor

github-actions bot commented Jun 5, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Looks good. I think we should address the TODO before merging this and extend the test plan with a case that shows that changing the verbosity in the extension settings changes the output.

crates/ruff_server/src/trace.rs Show resolved Hide resolved
crates/ruff_server/src/trace.rs Outdated Show resolved Hide resolved
Copy link

codspeed-hq bot commented Jun 5, 2024

CodSpeed Performance Report

Merging #11747 will improve performances by 13.17%

Comparing jane/server/logging-v2 (f5d35dd) with main (521a358)

Summary

⚡ 1 improvements
✅ 29 untouched benchmarks

Benchmarks breakdown

Benchmark main jane/server/logging-v2 Change
linter/all-with-preview-rules[numpy/globals.py] 917.3 µs 810.5 µs +13.17%

@snowsignal snowsignal changed the title ruff server: Use window/logMessage for tracing events ruff server: Tracing system now respects log level and trace level, with options to log to a file Jun 9, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

The code changes look good to me. I left a few questions and are waiting for a test plan that shows a few traces.

I think some documentation on how the tracing setup would be helpful, especially because it now supports environment variables, tracing level, logLevel, log file etc.

crates/ruff_server/src/trace.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/trace.rs Show resolved Hide resolved
crates/ruff_server/src/trace.rs Show resolved Hide resolved
crates/ruff_server/src/trace.rs Show resolved Hide resolved
crates/ruff_server/src/trace.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/message.rs Show resolved Hide resolved
crates/ruff_server/src/session/settings.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server.rs Outdated Show resolved Hide resolved
@MichaReiser
Copy link
Member

This looks good to me. Happy to approve tomorrow morning when the Test plan's filled out. Thanks for doing another iteration on this!

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice! Thank's for taking the time to iterate on the design

@dhruvmanila
Copy link
Member

This isn't blocking the PR but can you provide similar examples for Neovim. I'm able to set the RUFF_TRACE environment variable via the cmd_env key but not able to figure out where the logLevel / logFile should go and how should they affect the messages displayed.

@snowsignal
Copy link
Contributor Author

@dhruvmanila I've updated the PR description with a Neovim testing guide 😄

@snowsignal snowsignal merged commit 507f5c1 into main Jun 11, 2024
20 checks passed
@snowsignal snowsignal deleted the jane/server/logging-v2 branch June 11, 2024 18:29
snowsignal added a commit that referenced this pull request Jun 18, 2024
snowsignal added a commit that referenced this pull request Jun 18, 2024
A follow-up to [this
suggestion](#11747 (comment))
on the tracing PR.

---------

Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruff server not closing with neovim instances - vol 2 Server log messages should use window/logMessage
3 participants