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

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added inline comments.


================
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.
----------------
kbobyrev wrote:
> kadircet wrote:
> > 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)
> The point here is that is the index load freshness but it's not the index load, not the index age itself. The index age is e.g. the time of the last commit being indexed. Hence, we don't actually have this information _right now_ - it should be provided separately (probably in a way of adjacent `index.metadata` file or something similar). E.g. when we download from GitHub release, we do know the time an index was uploaded, that's a better estimate of the index "age" probably. But what the comment in Proto file says is actually more like the commit time, e.g. when did the last change that made it into the index happen.
ah i see, yes you are totally right.

maybe rephrase as `We are currently making use of the last modification time of the index artifact to deduce its age. This is wrong as it doesn't account for the indexing delay. Propagate some metadata with the index artifacts to indicate time of the commit we indexed.`


================
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());
+
----------------
kbobyrev wrote:
> kadircet wrote:
> > 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.)
> I think using `Status.getLastModificationTime()` should be correct, right? This is the index we actually loaded (checked above).
right that would do, but now we are lying about the server's uptime :D

so we should explicitly call `Monitor.updateIndex(Status->getLastModificationTime());` and still initialize monitoring service with `system_clock::now`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98246