[PATCH] D98246: [clangd] Add basic monitoring info request for remote index server

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:13
 
+import "google/protobuf/empty.proto";
 import "Index.proto";
----------------
question of style, but unless there's a concrete benefit I'd suggest just defining the request inline.
Cost is not just the import but also the irregularity of the request type *not* being called MonitoringInfoRequest or whatever.


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:16
 
+message MonitoringInfoReply { optional string info = 1; }
+
----------------
it seems unlikely to me that `string info` is the right format, but I can't tell whether this is a placeholder or not


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:29
+
+  rpc MonitoringInfo(google.protobuf.Empty) returns (MonitoringInfoReply) {}
 }
----------------
Who's going to call this? If clients will, then it probably belongs here, but should have a different name.
If only e.g. a web UI or monitoring system will, it belongs in a separate `service` IMO.

Depending on what it returns, it may be interesting for clients to call and log!
Or it may be useful to ban clients from calling it (if it has private details)


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:29
+
+  rpc MonitoringInfo(google.protobuf.Empty) returns (MonitoringInfoReply) {}
 }
----------------
sammccall wrote:
> Who's going to call this? If clients will, then it probably belongs here, but should have a different name.
> If only e.g. a web UI or monitoring system will, it belongs in a separate `service` IMO.
> 
> Depending on what it returns, it may be interesting for clients to call and log!
> Or it may be useful to ban clients from calling it (if it has private details)
I think we should try harder to find a name that describes the data requested, rather than what we expect the data to be used for.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:332
   grpc::EnableDefaultHealthCheckService(true);
+  grpc::reflection::InitProtoReflectionServerBuilderPlugin();
   grpc::ServerBuilder Builder;
----------------
This seems unrelated. Because it requires a build dep, move it to a separate patch? (That probably doesn't need review)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98246