[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/MonitoringService.proto:17
+  optional uint64 uptime = 1;
+  // Time since the index was built on the indexing machine.
+  optional uint64 freshness = 3;
----------------
mention the units in here too (e.g. seconds)

(it might be even better to just put that in the name actually, e.g. uptime_seconds, as documentation for generated files might not show up in tools like clangd :P, up to you though)


================
Comment at: clang-tools-extra/clangd/index/remote/MonitoringService.proto:18
+  // Time since the index was built on the indexing machine.
+  optional uint64 freshness = 3;
+  // ID of the indexed commit in Version Control System.
----------------
what happened to `2` ?


================
Comment at: clang-tools-extra/clangd/index/remote/MonitoringService.proto:18
+  // Time since the index was built on the indexing machine.
+  optional uint64 freshness = 3;
+  // ID of the indexed commit in Version Control System.
----------------
kadircet wrote:
> what happened to `2` ?
nit: maybe rename this to `index_age`


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:294
+
+  void updateIndex(const llvm::sys::TimePoint<> UpdateTime) {
+    IndexLastReload = UpdateTime;
----------------
nit: drop the `const`, as that's the style in clangd in general if we are going to copy the parameters anyway.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:308
+                          .count());
+    // FIXME(kirillbobyrev): This is not really "freshness", this is just the
+    // time since last index reload.
----------------
what's the issue to be fixed here exactly? do we want to display the index build time instead of age? (if so what's the blocker)


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:317
+  const llvm::sys::TimePoint<> StartTime;
+  llvm::sys::TimePoint<> IndexLastReload;
+};
----------------
we should either use an atomic here or use a mutex. as the `updateIndex` and serving RPCs are happening on different threads.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:344
       Status->getLastModificationTime(), Status->getSize());
+  Monitor.updateIndex(Status->getLastModificationTime());
 }
----------------
nit: i'd log after this one.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:457
 
-  std::thread HotReloadThread([&Index, &Status, &FS]() {
+  Monitor Monitor(std::chrono::system_clock::now());
+
----------------
unless we call updateIndex here too, monitoring service won't know about the one being served until the first reload right?

(another option would be to just not load the index at startup immediately, and handle the initial load in HotReloadThread, but that can be a separate change.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98246