haskell / lsp

Haskell library for the Microsoft Language Server Protocol
366 stars 92 forks source link

Change logging to use co-log-core instead of hslogger #401

Closed michaelpj closed 2 years ago

michaelpj commented 2 years ago

This started as an attempt to bubble up errors from the VFS as actual errors and return them to the user via the LSP response. However, in fact VFS operations occur in response to notifications, which don't have responses.

So all we can do is log the error and drop the change, which is okay. However, that made me look at how the logging works. At the moment we use hslogger, which is fine, but isn't so great when it's plugging into part of a larger system. For example, we might want to have a global log handler that sends error-level logs to the client as messages, or uses the logMessage method of the LSP spec. But there's no way to intercept the messages sent by the VFS currently.

So I switched over to using co-log-core, which is also the direction that HLS is going.

co-log-core is also a lightweight dependency. It's suboptimal for lsp-types to depend on a logging library, however, but that should be fixed when we do https://github.com/haskell/lsp/issues/394.

michaelpj commented 2 years ago

Yeah, I don't know who's active at the moment and would actually be interested in this. Actually, I wonder if @eddiemundo might have some thoughts, as the author of the HLS PR?

michaelpj commented 2 years ago

There doesn't seem to any filtering on the priorities so you'll always see all messages, but if that's OK then it looks good to me.

Yeah, but only for the "easy" run method. For the "real" one you're expected to pass your own log action, so then you can filter as you like.

michaelpj commented 2 years ago

Oh, something I should really add to the changelog: there were a couple of places where the lsp server itself was sending messages to the user. I changed those to instead log errors, and then it's up to the caller whether to use a log handler that sends errors to the user. And also we could export such a handler, which would be useful.

eddiemundo commented 2 years ago

So by log handler you mean providing a LogAction IO LspServerLog that sends error logs happening in the process message loop to the client. Then an lsp implementer could pass that directly to lsp, or wrap it in their own LogAction if they want? Yeah I agree it would maintain the old client visible behaviour as well as serve as an example of how to make a LogAction.

michaelpj commented 2 years ago

Exactly. I think we've discussed in HLS having a log handler that sends all messages with error severity to the client - this is then just a special case of that.

michaelpj commented 2 years ago

Updated to use WithSeverity from co-log, and to provide some handlers for logging to the client via window/logMessage and windowShowMessage. However, you can't actually use such a handler without the server having started up properly, so this means unfortunately that running the server now needs two handlers: one for logging in IO before the server has started up fully, and one for logging in LspM that can be used after that and can send messages.

I updated the example server to use such a logger, which I think works quite nicely.

michaelpj commented 2 years ago

I think this is getting to a good state, and I think we could merge it. There's some follow-ups to sort before we can release:

I'll do some work on the latter two in parallel PRs, but they'll be a bit easier once this is merged.

michaelpj commented 2 years ago

Issue for Pretty instances: https://github.com/haskell/lsp/issues/403

michaelpj commented 2 years ago

Okay, I rebased on the VFS changes and just did local changes here to do Pretty instances for the logs. So the only todo is the co-log-core release.

Bodigrim commented 2 years ago

co-log-core-0.3.1.0 is out, I believe.

michaelpj commented 2 years ago

Okay, I'm going to merge this. I've built HLS with this and appropriate modifications, it works and logs correctly, so that seems okay.