[PATCH] D92181: [clangd] NFC: Add client-side logging for remote index requests
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 8 04:03:22 PST 2020
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:45
Context.AddMetadata("version", clang::getClangToolFullVersion("clangd"));
- std::chrono::system_clock::time_point Deadline =
- std::chrono::system_clock::now() + DeadlineWaitingTime;
+ const std::chrono::system_clock::time_point StartTime =
+ std::chrono::system_clock::now();
----------------
(we don't ussually use `const` for locals)
================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:50
auto Reader = (Stub.get()->*RPCCall)(&Context, RPCRequest);
+ vlog("Sending RPC Request {0}: {1}", RequestT::descriptor()->name(),
+ RPCRequest.DebugString());
----------------
Including the request payload is IMO too much even at --log=verbose, we do this for LSP messages but those are more useful/central. Vlog the request and dlog the payload? (Similar to log/vlog for LSP)
================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:74
+ .count();
+ vlog("RPC Request {0} => OK: {1} results in {2}ms",
+ RequestT::descriptor()->name(), Successful, Millis);
----------------
"RPC Request" may be a bit jargony for these messages. And "Request" is part of the RequestT name.
It'd be nice to include the server name too, there may be several.
What about `Remote index: LookupRequest => 123 results in 18ms. [llvm.clangd-index-staging.org:1234]`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92181/new/
https://reviews.llvm.org/D92181
More information about the cfe-commits
mailing list