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

Provide log messages for the native server #12523

Open
dhruvmanila opened this issue Jul 26, 2024 · 6 comments
Open

Provide log messages for the native server #12523

dhruvmanila opened this issue Jul 26, 2024 · 6 comments
Labels
server Related to the LSP server

Comments

@dhruvmanila
Copy link
Member

There aren't many log messages emitted by the server which can help in debugging. It would be quite useful to provide some more log messages at info / debug / trace levels.

Note that these log messages shouldn't include the request response messages because that's taken care by the editor. It should be specific to the server logic.

@dhruvmanila dhruvmanila added the server Related to the LSP server label Jul 26, 2024
@dhruvmanila
Copy link
Member Author

In addition to the log messages, it's also important for us to determine how to send these log messages to the client. Currently, all tracing logs are sent to stderr unless diverted by using the ruff.logFile server setting. But, as per the spec, the tracing logs are usually sent via the $/logTrace request. We should also consider using the window/logMessage request for user-facing messages.

@MichaReiser
Copy link
Member

But, as per the spec, the tracing logs are usually sent via the $/logTrace

There was some internal discussion around using $logTrace. I don't remember the reasoning to not use it.

dhruvmanila added a commit that referenced this issue Jul 30, 2024
## Summary

This pull request adds support for logging via `$/logTrace` RPC
messages. It also enables that code path for when a client is Zed editor
or VS Code (as there's no way for us to generically tell whether a client prefers
`$/logTrace` over stderr.

Related to: #12523

## Test Plan

I've built Ruff from this branch and tested it manually with Zed.

---------

Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
@dhruvmanila
Copy link
Member Author

I think this is becoming more relevant because currently the user experience around viewing log messages isn't automatic.

Currently, the default value of the ruff.trace.server (in VS Code) or RUFF_TRACE environment variable (for other editors) is off and the default value of ruff.logLevel is info. This means that any error / warning messages that the server sends to the client are not going to be logged unless the user explicitly turns on server log messages by updating the trace value. This doesn't seem like a good user experience. For example,

tracing::error!("Encountered error when routing request with ID {id}: {err}");
show_err_msg!(
"Ruff failed to handle a request from the editor. Check the logs for more details."
);

Here, the client will display the notification with the message as in show_err_msg macro but it won't log the error via the tracing::error call unless the trace value is messages or verbose. This means that the user will see the notification and when they open the logs they won't see anything.

The differentiating factor is that which messages should always be logged (window/logMessage) and which are optional ($/logTrace).

@MichaReiser
Copy link
Member

This means that any error / warning messages that the server sends to the client are not going to be logged unless the user explicitly turns on server log messages by updating the trace value. This doesn't seem like a good user experience. For example,

Yeah. This doesn't seem like a good default. I think the default should be info similar to red knot.

@dhruvmanila
Copy link
Member Author

Yeah. This doesn't seem like a good default. I think the default should be info similar to red knot.

Do you mean the default trace value should be "messages"? The default value for the log level is info but the logs themselves are off by way of having the default trace value as off.

Regardless, if we update the default trace value to be "messages", the output channel in the VS Code will be filled with the request - response cycle messages like:

2024-09-06 12:08:38.040 [info] [Trace - 12:08:38 PM] Sending request 'textDocument/codeAction - (28)'.
2024-09-06 12:08:38.041 [info] [Trace - 12:08:38 PM] Received response 'textDocument/codeAction - (28)' in 1ms.
2024-09-06 12:08:38.290 [info] [Trace - 12:08:38 PM] Sending request 'textDocument/codeAction - (29)'.
2024-09-06 12:08:38.291 [info] [Trace - 12:08:38 PM] Received response 'textDocument/codeAction - (29)' in 1ms.

We could have a separate output channel for the trace messages (similar to rust-analyzer) but that would also include the trace messages from the Ruff server. This is why we should also consider using window/logMessage in addition to $/logTrace.

@MichaReiser
Copy link
Member

Hmm. I would need to take a closer look at how our tracing setup works. Ideally, we would show info, warning, and error messages (tracing::info!) by default. We could then use those levels to e.g. log what configurations we discovered.

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

No branches or pull requests

2 participants