[PATCH] D90654: [clangd] Add index server request logging

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I wish there was an easy way to check redacted error logs too :/



================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:335
     return std::make_unique<StreamLogger>(OS, LogLevel);
+    llvm::errs() << "non redacted logger\n";
+  }
----------------
feels like this one, and the one below are residues?


================
Comment at: clang-tools-extra/clangd/test/remote-index/public-log.test:2
+# RUN: rm -rf %t
+# RUN: clangd-indexer %S/Inputs/Source.cpp > %t.idx
+# RUN: %python %S/pipeline_helper.py --input-file-name=%s --server-arg=--log=verbose --server-arg=-log-public --server-log=%t.public.log --project-root=%S --index-file=%t.idx > /dev/null
----------------
nit: use `%/S` to not mix forward and backslashes.


================
Comment at: clang-tools-extra/clangd/test/remote-index/public-log.test:3
+# RUN: clangd-indexer %S/Inputs/Source.cpp > %t.idx
+# RUN: %python %S/pipeline_helper.py --input-file-name=%s --server-arg=--log=verbose --server-arg=-log-public --server-log=%t.public.log --project-root=%S --index-file=%t.idx > /dev/null
+# RUN: %python %S/pipeline_helper.py --input-file-name=%s --server-arg=--log=verbose --server-log=%t.log --project-root=%S --index-file=%t.idx > /dev/null
----------------
not sure if it is any easier, but it might make sense to just pass anything after `--` to server, rather than explicitly mentioning `--server-arg` before each one. I suppose we can also rename the script to `server_request_helper` ?

This won't work if we decide to pass arbitrary client flags too though.


================
Comment at: clang-tools-extra/clangd/test/remote-index/public-log.test:6
+# RUN: FileCheck --check-prefixes=LOG,LOG-PUBLIC %s < %t.public.log
+# RUN: FileCheck --check-prefixes=LOG,LOG-ALL %s < %t.log
+# REQUIRES: clangd-remote-index
----------------
can you also add a check for TextProto printing ? I believe descriptor names should be enough.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90654/new/

https://reviews.llvm.org/D90654