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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:13
 
+import "google/protobuf/empty.proto";
 import "Index.proto";
----------------
sammccall wrote:
> 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.
+1 (i also had a comment about it but forgot to hit submit)


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:16
 
+message MonitoringInfoReply { optional string info = 1; }
+
----------------
i am not sure if having more structure here immediately vs. incrementally makes much difference, we should at least settle on the proto initially, and can fill in those fields in incremental steps if need be (but i don't expect those changes to be huge, so it should be fine to just cram them into this patch).

i think we need more structure here, rather than just string. for starters:
- an unsigned for uptime (probably in seconds)
- another unsgined (or timepoint object) for indicating the build time of currently used index

i am not sure if we got anything else we can dispatch immediately, but can probably incorporate things like QPS and more details about the loaded index if turns out to be useful/needed.


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:29
+
+  rpc MonitoringInfo(google.protobuf.Empty) returns (MonitoringInfoReply) {}
 }
----------------
sammccall wrote:
> 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.
actually having this available as a separate service makes sense to me, it is not directly related to symbol index facilities and i don't think will be useful to clangd as is. in the future we might try to expose some meta information to the clients about the index being used on the server to enable different workflows (e.g. branches/staleness/incremental updates), but that's probably a different endpoint.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98246