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

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added a comment.

thanks! this mostly LG, just a couple comments here and there.

> The current format is "store seconds since X event".

I think this is great for certain things like freshness, so thanks!

> Should we store UNIX timestamps instead? This is probably not portable until C++20, so maybe it isn't a great idea.

But I believe we also need to record timepoints. what about just storing seconds since unix epoch in an int64?
i don't follow why it would be unportable, as by definition it is the time elapsed since 1-1-1970, so i don't think it has anything specific to UNIX itself, apart from the name.



================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:27
 
+message MonitoringInfoRequest {}
+message MonitoringInfoReply {
----------------
nit: i'd prefer these to be on a separate file (both messages and the service definition), WDYT?


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:32
+  // Time since the last index reload (in seconds).
+  optional uint64 time_since_reload = 2;
+  // FIXME(kirillbobyrev): The following fields should be provided with the
----------------
this and freshness has very little difference. i'd either drop one (preferably this one), or change this one to be a timepoint for the latest index reload.


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:33
+  optional uint64 time_since_reload = 2;
+  // FIXME(kirillbobyrev): The following fields should be provided with the
+  // index (probably in adjacent metadata.txt file next to loaded .idx).
----------------
i think this fixme belongs to the service implementation.


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:37
+  optional uint64 freshness = 3;
+  // Time since the index was built on the indexing machine.
+  optional string index_commit_hash = 4;
----------------
i smell some c/p


================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:41
+  optional string index_link = 5;
+  optional string metadata = 9000;
+}
----------------
what's the point of this exactly?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:351
                              IdleTimeoutSeconds * 1000);
   Builder.RegisterService(&Service);
   std::unique_ptr<grpc::Server> Server(Builder.BuildAndStart());
----------------
don't we need to register monitoring service in here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98246