[PATCH] D92198: [clangd] Log remote index connectivity status

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks! Hooking into the native status stuff seems much nicer!

Sorry if it seems like we're going in circles here, I think we should be careful about adding background threads so want to look at an alternative too.



================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:29
+
+namespace llvm {
+template <> struct format_provider<grpc_connectivity_state> {
----------------
this seems pedantic, but we really shouldn't specialize templates we don't own for types we don't own (ODR concerns)

I'd just make this function (state -> StringRef) and call it


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:35
+    case GRPC_CHANNEL_IDLE:
+      Stream << "GRPC_CHANNEL_IDLE";
+      break;
----------------
any reason not to make these a bit more human-readable? connecting/ready/transient failure/shutdown?


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:114
     assert(!ProjectRoot.empty());
+    ChannelStatusWatcher = std::thread([&Channel]() {
+      grpc_connectivity_state Status =
----------------
The thread is a bit surprising here. I was assuming we'd use some callback-based API to watch rather than spawn a thread to do the "work" of blocking on a change.
Also this is going to "wake up" periodically to refresh the watch, which seems undesirable if we want to go truly idle.

Ah, so there's no callback-type connection-changed API! Sorry to mislead you :-( I hadn't read the API carefully. (I think this is coming as some point - they're talking about replacing CompletionQueue with EventManager which is a callback-based abstraction)

Seems the "async" option is to subscribe on a CompletionQueue, but then we have to poll it. We can use a zero timeout so the poll doesn't block... But then we might as well poll GetStatus directly.

Then the question is where to do that from. What if we called GetStatus before/after each request, and reported whenever it has changed? That means:

- no idle background work
- we know the connection state for each request
- we know when a request triggered a connection, and whether that connection succeeded in time for the request
- if a connection goes idle, we don't know *when*, it gets reported on the next request


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:123
+                                            DeadlineWaitingTime)) {
+          Status = Channel->GetState(/*try_to_connect=*/true);
+          log("Remote index connection status changed: {0}", Status);
----------------
This is spinning in the background keeping the channel alive (`true`)... I'm actually not sure that's advisable.

If the connection is being used, it'll stay alive in the same way. So true here affects idle (and init) behavior.

Specifically, we maintain the TCP connection for the idle period to reduce latency of the first couple of requests after waking up (reestablish connection). I'm not sure this is a good trade, idle could easily be long and I don't think we know better than the OS when to drop a connection. (Cost might be battery life, background CPU load... IDK but probably something)

I think we should ask the connection to establish on startup (*high* chance we need it soon) but otherwise only in response to requests


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:124
+          Status = Channel->GetState(/*try_to_connect=*/true);
+          log("Remote index connection status changed: {0}", Status);
+        }
----------------
Can we include the remote address and prior status?

e.g. `"Remote index ({0}): {1} => {2}"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92198