Skip to content

Commit

Permalink
Improve logging system using logLevel, avoid trace value (#15232)
Browse files Browse the repository at this point in the history
## Summary

Refer to the VS Code PR
(astral-sh/ruff-vscode#659) for details on the
change.

This PR changes the following:

1. Add tracing span for both request (request id and method name) and
notification (method name) handler
2. Remove the `RUFF_TRACE` environment variable. This was being used to
turn on / off logging for the server
3. Similarly, remove reading the `trace` value from the initialization
options
4. Remove handling the `$/setTrace` notification
5. Remove the specialized `TraceLogWriter` used for Zed and VS Code
(#12564)

Regarding the (5) for the Zed editor, the reason that was implemented
was because there was no way of looking at the stderr messages in the
editor which has been changed. Now, it captures the stderr as part of
the "Server Logs".
(https://github.com/zed-industries/zed/blob/82492d74a8d0350cba66671c27e282a928fd4c85/crates/language_tools/src/lsp_log.rs#L548-L552)

### Question

Regarding (1), I think having just a simple trace level message should
be good for now as the spans are not hierarchical. This could be tackled
with #12744. The difference between the two:

<details><summary>Using <code>tracing::trace</code></summary>
<p>

```
   0.019243416s DEBUG ThreadId(08) ruff_server::session::index::ruff_settings: Ignored path via `exclude`: /Users/dhruv/playground/ruff/.vscode
   0.026398750s  INFO main ruff_server::session::index: Registering workspace: /Users/dhruv/playground/ruff
   0.026802125s TRACE ruff:main ruff_server::server::api: Received notification "textDocument/didOpen"
   0.026930666s TRACE ruff:main ruff_server::server::api: Received notification "textDocument/didOpen"
   0.026962333s TRACE ruff:main ruff_server::server::api: Received request "textDocument/diagnostic" (1)
   0.027042875s TRACE ruff:main ruff_server::server::api: Received request "textDocument/diagnostic" (2)
   0.027097500s TRACE ruff:main ruff_server::server::api: Received request "textDocument/codeAction" (3)
   0.027107458s DEBUG ruff:worker:0 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
   0.027123541s DEBUG ruff:worker:3 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/organize_imports.py
   0.027514875s  INFO     ruff:main ruff_server::server: Configuration file watcher successfully registered
   0.285689833s TRACE     ruff:main ruff_server::server::api: Received request "textDocument/codeAction" (4)
  45.741101666s TRACE     ruff:main ruff_server::server::api: Received notification "textDocument/didClose"
  47.108745500s TRACE     ruff:main ruff_server::server::api: Received notification "textDocument/didOpen"
  47.109802041s TRACE     ruff:main ruff_server::server::api: Received request "textDocument/diagnostic" (5)
  47.109926958s TRACE     ruff:main ruff_server::server::api: Received request "textDocument/codeAction" (6)
  47.110027791s DEBUG ruff:worker:6 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
  51.863679125s TRACE     ruff:main ruff_server::server::api: Received request "textDocument/hover" (7)
```

</p>
</details> 

<details><summary>Using <code>tracing::trace_span</code></summary>
<p>

Only logging the enter event:

```
   0.018638750s DEBUG ThreadId(11) ruff_server::session::index::ruff_settings: Ignored path via `exclude`: /Users/dhruv/playground/ruff/.vscode
   0.025895791s  INFO main ruff_server::session::index: Registering workspace: /Users/dhruv/playground/ruff
   0.026378791s TRACE ruff:main notification{method="textDocument/didOpen"}: ruff_server::server::api: enter
   0.026531208s TRACE ruff:main notification{method="textDocument/didOpen"}: ruff_server::server::api: enter
   0.026567583s TRACE ruff:main request{id=1 method="textDocument/diagnostic"}: ruff_server::server::api: enter
   0.026652541s TRACE ruff:main request{id=2 method="textDocument/diagnostic"}: ruff_server::server::api: enter
   0.026711041s DEBUG ruff:worker:2 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/organize_imports.py
   0.026729166s DEBUG ruff:worker:1 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
   0.027023083s  INFO     ruff:main ruff_server::server: Configuration file watcher successfully registered
   5.197554750s TRACE     ruff:main notification{method="textDocument/didClose"}: ruff_server::server::api: enter
   6.534458000s TRACE     ruff:main notification{method="textDocument/didOpen"}: ruff_server::server::api: enter
   6.535027958s TRACE     ruff:main request{id=3 method="textDocument/diagnostic"}: ruff_server::server::api: enter
   6.535271166s DEBUG ruff:worker:3 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/organize_imports.py
   6.544240583s TRACE     ruff:main request{id=4 method="textDocument/codeAction"}: ruff_server::server::api: enter
   7.049692458s TRACE     ruff:main request{id=5 method="textDocument/codeAction"}: ruff_server::server::api: enter
   7.508142541s TRACE     ruff:main request{id=6 method="textDocument/hover"}: ruff_server::server::api: enter
   7.872421958s TRACE     ruff:main request{id=7 method="textDocument/hover"}: ruff_server::server::api: enter
   8.024498583s TRACE     ruff:main request{id=8 method="textDocument/codeAction"}: ruff_server::server::api: enter
  13.895063666s TRACE     ruff:main request{id=9 method="textDocument/codeAction"}: ruff_server::server::api: enter
  14.774706083s TRACE     ruff:main request{id=10 method="textDocument/hover"}: ruff_server::server::api: enter
  16.058918958s TRACE     ruff:main notification{method="textDocument/didChange"}: ruff_server::server::api: enter
  16.060562208s TRACE     ruff:main request{id=11 method="textDocument/diagnostic"}: ruff_server::server::api: enter
  16.061109083s DEBUG ruff:worker:8 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
  21.561742875s TRACE     ruff:main notification{method="textDocument/didChange"}: ruff_server::server::api: enter
  21.563573791s TRACE     ruff:main request{id=12 method="textDocument/diagnostic"}: ruff_server::server::api: enter
  21.564206750s DEBUG ruff:worker:4 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
  21.826691375s TRACE     ruff:main request{id=13 method="textDocument/codeAction"}: ruff_server::server::api: enter
  22.091080125s TRACE     ruff:main request{id=14 method="textDocument/codeAction"}: ruff_server::server::api: enter
```

</p>
</details> 


**Todo**

- [x] Update documentation (I'll be adding a troubleshooting section
under "Editors" as a follow-up which is for all editors)
- [x] Check for backwards compatibility. I don't think this should break
backwards compatibility as it's mainly targeted towards improving the
debugging experience.

~**Before I go on to updating the documentation, I'd appreciate initial
review on the chosen approach.**~

resolves: #14959 

## Test Plan

Refer to the test plan in
astral-sh/ruff-vscode#659.

Example logs at `debug` level:

```
   0.010770083s DEBUG ThreadId(15) ruff_server::session::index::ruff_settings: Ignored path via `exclude`: /Users/dhruv/playground/ruff/.vscode
   0.018101916s  INFO main ruff_server::session::index: Registering workspace: /Users/dhruv/playground/ruff
   0.018559916s DEBUG ruff:worker:4 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
   0.018992375s  INFO     ruff:main ruff_server::server: Configuration file watcher successfully registered
  23.408802375s DEBUG ruff:worker:11 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
  24.329127416s DEBUG  ruff:worker:6 ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
```

Example logs at `trace` level:

```
   0.010296375s DEBUG ThreadId(13) ruff_server::session::index::ruff_settings: Ignored path via `exclude`: /Users/dhruv/playground/ruff/.vscode
   0.017422583s  INFO main ruff_server::session::index: Registering workspace: /Users/dhruv/playground/ruff
   0.018034458s TRACE ruff:main notification{method="textDocument/didOpen"}: ruff_server::server::api: enter
   0.018199708s TRACE ruff:worker:0 request{id=1 method="textDocument/diagnostic"}: ruff_server::server::api: enter
   0.018251167s DEBUG ruff:worker:0 request{id=1 method="textDocument/diagnostic"}: ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
   0.018528708s  INFO     ruff:main ruff_server::server: Configuration file watcher successfully registered
   1.611798417s TRACE ruff:worker:1 request{id=2 method="textDocument/codeAction"}: ruff_server::server::api: enter
   1.861757542s TRACE ruff:worker:4 request{id=3 method="textDocument/codeAction"}: ruff_server::server::api: enter
   7.027361792s TRACE ruff:worker:2 request{id=4 method="textDocument/codeAction"}: ruff_server::server::api: enter
   7.851361500s TRACE ruff:worker:5 request{id=5 method="textDocument/codeAction"}: ruff_server::server::api: enter
   7.901690875s TRACE     ruff:main notification{method="textDocument/didChange"}: ruff_server::server::api: enter
   7.903063167s TRACE ruff:worker:10 request{id=6 method="textDocument/diagnostic"}: ruff_server::server::api: enter
   7.903183500s DEBUG ruff:worker:10 request{id=6 method="textDocument/diagnostic"}: ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
   8.702385292s TRACE      ruff:main notification{method="textDocument/didChange"}: ruff_server::server::api: enter
   8.704106625s TRACE  ruff:worker:3 request{id=7 method="textDocument/diagnostic"}: ruff_server::server::api: enter
   8.704304875s DEBUG  ruff:worker:3 request{id=7 method="textDocument/diagnostic"}: ruff_server::resolve: Included path via `include`: /Users/dhruv/playground/ruff/lsp/play.py
   8.966853458s TRACE  ruff:worker:9 request{id=8 method="textDocument/codeAction"}: ruff_server::server::api: enter
   9.229622792s TRACE  ruff:worker:6 request{id=9 method="textDocument/codeAction"}: ruff_server::server::api: enter
  10.513111583s TRACE  ruff:worker:7 request{id=10 method="textDocument/codeAction"}: ruff_server::server::api: enter
```
  • Loading branch information
dhruvmanila authored Jan 8, 2025
1 parent 9a27b37 commit 1447553
Show file tree
Hide file tree
Showing 17 changed files with 257 additions and 540 deletions.
2 changes: 1 addition & 1 deletion crates/red_knot_server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ use crate::server::Server;
mod message;

mod edit;
mod logging;
mod server;
mod session;
mod system;
mod trace;

pub(crate) const SERVER_NAME: &str = "red-knot";
pub(crate) const DIAGNOSTIC_NAME: &str = "Red Knot";
Expand Down
114 changes: 114 additions & 0 deletions crates/red_knot_server/src/logging.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
//! The logging system for `red_knot server`.
//!
//! Log messages are controlled by the `logLevel` setting which defaults to `"info"`. Log messages
//! are written to `stderr` by default, which should appear in the logs for most LSP clients. A
//! `logFile` path can also be specified in the settings, and output will be directed there
//! instead.
use core::str;
use serde::Deserialize;
use std::{path::PathBuf, str::FromStr, sync::Arc};
use tracing::level_filters::LevelFilter;
use tracing_subscriber::{
fmt::{time::Uptime, writer::BoxMakeWriter},
layer::SubscriberExt,
Layer,
};

pub(crate) fn init_logging(log_level: LogLevel, log_file: Option<&std::path::Path>) {
let log_file = log_file
.map(|path| {
// this expands `logFile` so that tildes and environment variables
// are replaced with their values, if possible.
if let Some(expanded) = shellexpand::full(&path.to_string_lossy())
.ok()
.and_then(|path| PathBuf::from_str(&path).ok())
{
expanded
} else {
path.to_path_buf()
}
})
.and_then(|path| {
std::fs::OpenOptions::new()
.create(true)
.append(true)
.open(&path)
.map_err(|err| {
#[allow(clippy::print_stderr)]
{
eprintln!(
"Failed to open file at {} for logging: {err}",
path.display()
);
}
})
.ok()
});

let logger = match log_file {
Some(file) => BoxMakeWriter::new(Arc::new(file)),
None => BoxMakeWriter::new(std::io::stderr),
};
let subscriber = tracing_subscriber::Registry::default().with(
tracing_subscriber::fmt::layer()
.with_timer(Uptime::default())
.with_thread_names(true)
.with_ansi(false)
.with_writer(logger)
.with_filter(LogLevelFilter { filter: log_level }),
);

tracing::subscriber::set_global_default(subscriber)
.expect("should be able to set global default subscriber");
}

/// The log level for the server as provided by the client during initialization.
///
/// The default log level is `info`.
#[derive(Clone, Copy, Debug, Deserialize, Default, PartialEq, Eq, PartialOrd, Ord)]
#[serde(rename_all = "lowercase")]
pub(crate) enum LogLevel {
Error,
Warn,
#[default]
Info,
Debug,
Trace,
}

impl LogLevel {
fn trace_level(self) -> tracing::Level {
match self {
Self::Error => tracing::Level::ERROR,
Self::Warn => tracing::Level::WARN,
Self::Info => tracing::Level::INFO,
Self::Debug => tracing::Level::DEBUG,
Self::Trace => tracing::Level::TRACE,
}
}
}

/// Filters out traces which have a log level lower than the `logLevel` set by the client.
struct LogLevelFilter {
filter: LogLevel,
}

impl<S> tracing_subscriber::layer::Filter<S> for LogLevelFilter {
fn enabled(
&self,
meta: &tracing::Metadata<'_>,
_: &tracing_subscriber::layer::Context<'_, S>,
) -> bool {
let filter = if meta.target().starts_with("red_knot") {
self.filter.trace_level()
} else {
tracing::Level::INFO
};

meta.level() <= &filter
}

fn max_level_hint(&self) -> Option<tracing::level_filters::LevelFilter> {
Some(LevelFilter::from_level(self.filter.trace_level()))
}
}
13 changes: 2 additions & 11 deletions crates/red_knot_server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@ impl Server {
crate::version(),
)?;

if let Some(trace) = init_params.trace {
crate::trace::set_trace_value(trace);
}

crate::message::init_messenger(connection.make_sender());

let AllSettings {
Expand All @@ -66,14 +62,9 @@ impl Server {
.unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::default())),
);

crate::trace::init_tracing(
connection.make_sender(),
global_settings
.tracing
.log_level
.unwrap_or(crate::trace::LogLevel::Info),
crate::logging::init_logging(
global_settings.tracing.log_level.unwrap_or_default(),
global_settings.tracing.log_file.as_deref(),
init_params.client_info.as_ref(),
);

let mut workspace_for_url = |url: Url| {
Expand Down
3 changes: 0 additions & 3 deletions crates/red_knot_server/src/server/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ pub(super) fn notification<'a>(notif: server::Notification) -> Task<'a> {
notification::DidCloseNotebookHandler::METHOD => {
local_notification_task::<notification::DidCloseNotebookHandler>(notif)
}
notification::SetTraceHandler::METHOD => {
local_notification_task::<notification::SetTraceHandler>(notif)
}
method => {
tracing::warn!("Received notification {method} which does not have a handler.");
return Task::nothing();
Expand Down
2 changes: 0 additions & 2 deletions crates/red_knot_server/src/server/api/notifications.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ mod did_close;
mod did_close_notebook;
mod did_open;
mod did_open_notebook;
mod set_trace;

pub(super) use did_change::DidChangeTextDocumentHandler;
pub(super) use did_close::DidCloseTextDocumentHandler;
pub(super) use did_close_notebook::DidCloseNotebookHandler;
pub(super) use did_open::DidOpenTextDocumentHandler;
pub(super) use did_open_notebook::DidOpenNotebookHandler;
pub(super) use set_trace::SetTraceHandler;
25 changes: 0 additions & 25 deletions crates/red_knot_server/src/server/api/notifications/set_trace.rs

This file was deleted.

2 changes: 1 addition & 1 deletion crates/red_knot_server/src/session/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub struct ClientSettings {
#[cfg_attr(test, derive(PartialEq, Eq))]
#[serde(rename_all = "camelCase")]
pub(crate) struct TracingSettings {
pub(crate) log_level: Option<crate::trace::LogLevel>,
pub(crate) log_level: Option<crate::logging::LogLevel>,
/// Path to the log file - tildes and environment variables are supported.
pub(crate) log_file: Option<PathBuf>,
}
Expand Down
Loading

0 comments on commit 1447553

Please sign in to comment.