Replace tracing log functions in ruff server with LSP-compliant logging functions
#11169
Conversation
|
|
Is there an alternate design whereby we wire up |
|
@charliermarsh I think that should be possible by setting up our own subscriber that then forwards messages to the LSP. I think the problem this approach is that it only works for ruff_server, but we will use tracing in runes too and we don't have the luxury to change all tracing calls there. I think we need to explore an alternative approach that captures all tracing messages (subscriber) and forwards them correctly OR setup an alternative subscriber that writes e.g. to a file, although I would prefer seeing the tracing messages in vs code. |
|
Yeah I would prefer trying to integrate this into tracing too (for those reasons, plus it's the API I would've naturally reached for if I went to write code in the LSP as a future editor / author). @snowsignal can you give it a shot? |
|
@charliermarsh Sure, I'll see what I can do! |
snowsignal
force-pushed
the
jane/server/logging
branch
from
d1232e3
to
0ea86f9
Compare
| @@ -46,6 +46,7 @@ impl Server { | |||
| pub fn new(worker_threads: NonZeroUsize) -> crate::Result<Self> { | |||
| let (conn, threads) = lsp::Connection::stdio(); | |||
|
|
|||
| crate::trace::init_tracing(&conn.sender, crate::trace::LogLevel::Info); | |||
There was a problem hiding this comment.
We should ideally respect the log level configured in the VS Code settings or does VS code take care of filtering out log messages?
There was a problem hiding this comment.
We don't have a log level configured in the VS Code settings at the moment. All we have is ruff.trace.server, but that's more a verbosity setting than something that controls the log level.
| tracing::Level::WARN => Self::Warn, | ||
| tracing::Level::INFO => Self::Info, | ||
| tracing::Level::DEBUG => Self::Debug, | ||
| _ => return None, |
There was a problem hiding this comment.
Can we use a debug level instead of dropping these messages?
| fn is_enabled(&self, meta: &Metadata<'_>) -> bool { | ||
| let filter = if meta.target().starts_with("ruff") { | ||
| self.trace_level | ||
| } else { | ||
| Level::INFO | ||
| }; | ||
|
|
||
| meta.level() <= &filter | ||
| } |
There was a problem hiding this comment.
I think we still want to have some form of filtering of log messages to avoid that tracing logs from third party libraries (trace and debug) show up in our LSP logs (they can be very noisy).
The old implementation limited third-party logs to info level or higher for this.
| pub(crate) fn init_tracing(sender: &ClientSender, filter: LogLevel) { | ||
| LOGGING_SENDER | ||
| .set(sender.clone()) | ||
| .expect("logging sender should only be initialized once"); | ||
|
|
||
| let subscriber = tracing_subscriber::Registry::default().with(LogLayer { filter }); | ||
|
|
||
| tracing::subscriber::set_global_default(subscriber) | ||
| .expect("should be able to set global tracing subscriberr"); | ||
| } |
There was a problem hiding this comment.
I wonder what will happen now where ClientSender is a weakref. I suspect that we won't see the Main loop stopped message anymore because the logging sender is shut down by then.
I think we should unset our tracing subscriber before dropping Connection and restore the old subscriber to ensure these messages don't get lost.
There was a problem hiding this comment.
I think we should unset our tracing subscriber before dropping
Connectionand restore the old subscriber to ensure these messages don't get lost.
Good idea!
|
This PR is really outdated, especially with the major refactors we made to server initialization. I'll re-implement this feature in a new PR that's based off of the latest |
Summary
Closes #10968.
At the moment, the log level is always set to
Info. Log level configuration will be implemented in a follow-up.Test Plan
Add a series of logs, one for each level, to the definition of
Server::new, after the logger is initialized:You should see the following logs in the output of the VS Code extension:
