OmniSharp / csharp-language-server-protocol

Language Server Protocol in C#
MIT License
530 stars 104 forks source link

Add Logging support #27

Closed david-driscoll closed 4 years ago

david-driscoll commented 7 years ago

We should probably add a story for logging. I'm a huge user of Serilog however I don't want to enforce library choice on anyone because they could use .

Since we'll have to couple to something, it would be easiest to just add a reference to Microsoft.Extensions.Logging.Abstractions. And just hand off ILoggerFactory to LanguageServer.

Related #24

tintoy commented 7 years ago

Nice! I also use Serilog, but apart from Microsoft.Extensions.Logging there's also LibLog if you want to stay framework-agnostic. Apparently LibLog's selling point is that it automatically detects your logging framework at runtime and uses it. It also supports named format strings for structured logging such as "Customer name is {CustomerName}".

tintoy commented 7 years ago

Then again, Microsoft.Extensions.Logging.Abstractions also handles that format and seems a lot simpler. Gets my vote :)

david-driscoll commented 7 years ago

Familiarity pushes me towards Microsoft.Extensions.Logging, but that's a biased view point. 😏

david-driscoll commented 7 years ago

We could allow a logging factory in, or we could simply just internalize it all and just have ILogger use the logging extension methods.

tintoy commented 7 years ago

Yeah, I use MEL all the time, too (any time I'm doing ASP.NET Core MVC, for example) - personally I think it's sufficiently generic and widely-recognised that most consumers could adapt to it.

I think the logger factory is a good option; really, much of the ASP.NET app / pipeline model is not a bad way to model a simple server with middleware and / or optional services such as logging ;-)

tintoy commented 7 years ago

(and IMHO exposing the factory to clients is also a simple way to give them just enough control of where things are logged to cover the most common scenarios)

david-driscoll commented 7 years ago

Yeah allowing them to pass a factory gives the user some control on where the language server logs go. They may not care about our verbose logging once they have solved the problem they've run into.

tintoy commented 7 years ago

For sure - and it's not just about verbose logging; being able to filter by source context makes a big difference too. 99% of the time, nobody wants to see log messages for every single step of the receive loop. The other 1%, though? I'd just about kill for it :)

tintoy commented 7 years ago

BTW, examples of send, receive, and dispatch logging:

https://github.com/tintoy/dotnet-language-client/blob/a8b04586a8433fd59d14dbd7cca5ffb96070bf11/src/LSP.Client/Protocol/LspConnection.cs#L650-L660

https://github.com/tintoy/dotnet-language-client/blob/a8b04586a8433fd59d14dbd7cca5ffb96070bf11/src/LSP.Client/Protocol/LspConnection.cs#L720

https://github.com/tintoy/dotnet-language-client/blob/a8b04586a8433fd59d14dbd7cca5ffb96070bf11/src/LSP.Client/Protocol/LspConnection.cs#L840

tintoy commented 7 years ago

(that XXXing/XXXed logging is great for troubleshooting deadlocks)