[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:412
+    llvm::raw_string_ostream OS(Message);
+    OS << "method not found (" << Method << ")";
+    replyError(ErrorCode::MethodNotFound, OS.str());
----------------
simark wrote:
> ilya-biryukov wrote:
> > malaperle wrote:
> > > simark wrote:
> > > > ilya-biryukov wrote:
> > > > > Could we also `log` (i.e. not `vlog`) names of the methods that were handled successfully? To have some context when something crashes and we only have non-verbose logs.
> > > > That would be against the purpose of this patch, which is to output nothing if everything goes right (so it's easier to notice when something goes wrong).  Just outputting the names of the methods that are handled successfully would still be very verbose I think.
> > > Wouldn't that mean pretty much logging everything coming in? The idea of this patch it to make it that by default Clangd is not talkative unless there is an error and optionally make it verbose, for troubleshooting.
> > Logs are also useful to diagnose problems in case something crashes or works incorrectly.
> > Clang will probably log something when processing any request anyway, logging the method name first will merely give some more context on which request other log messages correspond to.
> > 
> > > The idea of this patch it to make it that by default Clangd is not talkative unless there is an error.
> > I don't think we use logging this way in clangd. Logs give us a way to make sense of what's happening inside clangd even when there's no error. `vlog` lets us turn off some extremely noisy output that is not useful most of the time.
> > We have a whole bunch of log statements that don't correspond to errors (e.g. "reusing preamble", "building file with compile command").
> > Logs are also useful to diagnose problems in case something crashes or works incorrectly.
> Clang will probably log something when processing any request anyway, logging the method name first will merely give some more context on which request other log messages correspond to.
> 
> I think it's fine if clangd logs warning/errors by default, that a user might want to look at and address.  But logging things that happen recurrently and are part of clangd's normal operation just flood the terminal (and makes it harder to spot actual errors).
> 
> I agree that having some context helps to make sense of an error.  A reasonnable way would be to include the method name only when printing an error.  For example, `While handling method 'workspace/symbol': Could not open file foo.`.  That would require us to carry around some more context information, but I think it would be better than printing all the handled methods. 
> 
> > I don't think we use logging this way in clangd. Logs give us a way to make sense of what's happening inside clangd even when there's no error. 
> 
> In that case you turn on a more verbose log level.
> 
> > vlog lets us turn off some extremely noisy output that is not useful most of the time. We have a whole bunch of log statements that don't correspond to errors (e.g. "reusing preamble", "building file with compile command").
> 
> I would consider everything that happens at each json-rpc request to be "noisy".  When using clangd from an IDE, there's quite a lot of requests being done.  I was also thinking of modyfing the patch to also use vlog for the "reusing preamble" and "building file with compile command" messages.
One more thing: when using multiple workers and the asynchronous model, the error may not be related to the last printed method name:

```
Handling json-rpc method A
Handling json-rpc method B
Some error related to the handling of request A
```

If you just see the error like this, you would think it's cause by request B, even though it's when handling request A.  Passing some context to the callback would allow us to print the request which is really at the origin of the error, avoiding any confusion.  And it would allow us to not print it if everything goes fine.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226