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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 5 inline comments as done.
sammccall added a comment.

Oops, took a while to get back to this.
Planning to land this with some style unaddressed (%/S, --input-file) but please do LMK if they're important and I'll fix.



================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:31
+  parser.add_argument('--server-arg', action='append')
+  parser.add_argument('--server-log', nargs='?', type=argparse.FileType('wb'), default=os.devnull)
 
----------------
kbobyrev wrote:
> Why do we need `wb` here instead of `w`? I know that `subprocess.Popen` writes binary data but since we're writing actual text to file maybe we could convert to non-binary?
we're just redirecting data from one stream to another, I don't think there's any reason to do character decoding+encoding on the way...


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:81
 if __name__ == '__main__':
   main()
----------------
kbobyrev wrote:
> this was `pipeline_helper.py` intended for `pipeline.test`, maybe we'll use it more so makes sense to rename it to just `helper.py` or somethting.
Agree - can do that as a followup.


================
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
----------------
kadircet wrote:
> nit: use `%/S` to not mix forward and backslashes.
Why do we want to avoid mixing them?
(This is copied from the other test which uses %S)


================
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
----------------
kadircet wrote:
> 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.
Yeah, there aren't a lot of these so i'd like it to be super-explicit for now. Happy to change this if we end up with lots of tests passing various server args (and few client args)


================
Comment at: clang-tools-extra/clangd/test/remote-index/public-log.test:5
+# 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
+# RUN: FileCheck --check-prefixes=LOG,LOG-PUBLIC %s < %t.public.log
+# RUN: FileCheck --check-prefixes=LOG,LOG-ALL %s < %t.log
----------------
ArcsinX wrote:
> Maybe we can use `--input-file` option of FileCheck instead of `<` (the same for the next line)
Why would this be preferred? (`<` is roughly 5x more common in llvm/test and clang/test - neither is used significantly in clang-tools-extra)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90654