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

[kbobyrev]

Summary

This allows requesting information about the server uptime and start time. This is the first patch in a series of monitoring changes, hence it's not immediately useful. Next step is propagating the index freshness information and then probably loading metadata into the index server.

The way to test new behaviour through command line:

$ grpc_cli call localhost:50051 Monitor/MonitoringInfo ''
connecting to localhost:50051
uptime_seconds: 42
index_age_seconds: 609568
Rpc succeeded with OK status

[kbobyrev]

Use reflection, improve message format.

[sammccall]

clang-tools-extra/clangd/index/remote/Service.proto

13

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.

15

it seems unlikely to me that string info is the right format, but I can't tell whether this is a placeholder or not

26

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)

26

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.

clang-tools-extra/clangd/index/remote/server/Server.cpp

360

This seems unrelated. Because it requires a build dep, move it to a separate patch? (That probably doesn't need review)

[kbobyrev]

Enabled server reflection in https://github.com/llvm/llvm-project/commit/8080ea4c4b8c456c72c617587cc32f174b3105c1

[kadircet]

clang-tools-extra/clangd/index/remote/Service.proto

13

+1 (i also had a comment about it but forgot to hit submit)

15

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.

26

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.

[sammccall]

clang-tools-extra/clangd/index/remote/Service.proto

15

we should at least settle on the proto initially, and can fill in those fields in incremental steps

Agree, incremental development is nice, but the scope/responsibility of RPCs is fairly important and not that easy to change later.

(Sorry, I hadn't seen this review wasn't actually assigned to me, happy to leave this with you!)

[kbobyrev]

Address review comments.

The current format is "store seconds since X event". Should we store UNIX
timestamps instead? This is probably not portable until C++20, so maybe it
isn't a great idea.

[kadircet]

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.

clang-tools-extra/clangd/index/remote/Service.proto

26

nit: i'd prefer these to be on a separate file (both messages and the service definition), WDYT?

31

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.

32

i think this fixme belongs to the service implementation.

36

i smell some c/p

40

what's the point of this exactly?

clang-tools-extra/clangd/index/remote/server/Server.cpp

368–369

don't we need to register monitoring service in here?

[kbobyrev]

Resolve review comments.

[kadircet]

clang-tools-extra/clangd/index/remote/MonitoringService.proto

18

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)

19

what happened to 2 ?

19

nit: maybe rename this to index_age

clang-tools-extra/clangd/index/remote/server/Server.cpp

299

nit: drop the const, as that's the style in clangd in general if we are going to copy the parameters anyway.

313

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)

322

we should either use an atomic here or use a mutex. as the updateIndex and serving RPCs are happening on different threads.

353

nit: i'd log after this one.

468–470

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.)

[kbobyrev]

Address comments, rebase on top of main.

[kbobyrev]

clang-tools-extra/clangd/index/remote/server/Server.cpp

313

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.

468–470

I think using Status.getLastModificationTime() should be correct, right? This is the index we actually loaded (checked above).

[kadircet]

clang-tools-extra/clangd/index/remote/server/Server.cpp

313

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.

468–470

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.

[kbobyrev]

Resolve review comments.

[kadircet]

thanks! let's ship it!

clang-tools-extra/clangd/index/remote/CMakeLists.txt

 if (CLANGD_ENABLE_REMOTE)
   generate_protos(RemoteIndexProto "Index.proto")
+  generate_protos(MonitoringServiceProto "MonitoringService.proto"
+    GRPC)
   generate_protos(RemoteIndexServiceProto "Service.proto"
     DEPENDS "Index.proto"
     GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
   # filename as a dependency, but linking requires target name.
   target_link_libraries(RemoteIndexServiceProto
     PRIVATE
     RemoteIndexProto
+    MonitoringServiceProto
     )
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 
   # FIXME(kirillbobyrev): target_compile_definitions is not working with
   # add_clang_library for some reason. Is there any way to make this
   # target-local?
   add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1)

clang-tools-extra/clangd/index/remote/MonitoringService.proto

+//===--- MonitoringService.proto - CLangd Remote index monitoring service -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+syntax = "proto2";
+
+package clang.clangd.remote.v1;
+
+message MonitoringInfoRequest {}
+message MonitoringInfoReply {
+  // Time since the server started (in seconds).
+  optional uint64 uptime_seconds = 1;
+  // Time since the index was built on the indexing machine.
+  optional uint64 index_age_seconds = 2;
+  // ID of the indexed commit in Version Control System.
+  optional string index_commit_hash = 3;
+  // URL to the index file.
+  optional string index_link = 4;
+}
+
+service Monitor {
+  rpc MonitoringInfo(MonitoringInfoRequest) returns (MonitoringInfoReply) {}
+}

clang-tools-extra/clangd/index/remote/Service.proto

 //===--- Service.proto - Remote index service definition ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 syntax = "proto2";
 
 package clang.clangd.remote.v1;
 
 import "Index.proto";
 
 // Semantics of SymbolIndex match clangd::SymbolIndex with all required
 // structures corresponding to their clangd::* counterparts.
 service SymbolIndex {
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
 
   rpc Refs(RefsRequest) returns (stream RefsReply) {}
 
   rpc Relations(RelationsRequest) returns (stream RelationsReply) {}
 }
-

clang-tools-extra/clangd/index/remote/server/Server.cpp

 //===--- Server.cpp - gRPC-based Remote Index Server  ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Features.inc"
 #include "Index.pb.h"
+#include "MonitoringService.grpc.pb.h"
+#include "MonitoringService.pb.h"
 #include "Service.grpc.pb.h"
+#include "Service.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Context.h"
 #include "support/Logger.h"
 #include "support/Shutdown.h"
 #include "support/ThreadsafeFS.h"
void logRequestSummary(llvm::StringLiteral RequestName, unsigned Sent,
     log("[public] request {0} => OK: {1} results in {2}ms", RequestName, Sent,
         Millis);
   }
 
   std::unique_ptr<Marshaller> ProtobufMarshaller;
   clangd::SymbolIndex &Index;
 };
 
+class Monitor final : public v1::Monitor::Service {
+public:
+  Monitor(llvm::sys::TimePoint<> IndexAge)
+      : StartTime(std::chrono::system_clock::now()), IndexBuildTime(IndexAge) {}
+
+  void updateIndex(llvm::sys::TimePoint<> UpdateTime) {
+    IndexBuildTime.exchange(UpdateTime);
+  }
+
+private:
+  // FIXME(kirillbobyrev): Most fields should be populated when the index
+  // reloads (probably in adjacent metadata.txt file next to loaded .idx) but
+  // they aren't right now.
+  grpc::Status MonitoringInfo(grpc::ServerContext *Context,
+                              const v1::MonitoringInfoRequest *Request,
+                              v1::MonitoringInfoReply *Reply) override {
+    Reply->set_uptime_seconds(std::chrono::duration_cast<std::chrono::seconds>(
+                                  std::chrono::system_clock::now() - StartTime)
+                                  .count());
+    // FIXME(kirillbobyrev): 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.
+    Reply->set_index_age_seconds(
+        std::chrono::duration_cast<std::chrono::seconds>(
+            std::chrono::system_clock::now() - IndexBuildTime.load())
+            .count());
+    return grpc::Status::OK;
+  }
+
+  const llvm::sys::TimePoint<> StartTime;
+  std::atomic<llvm::sys::TimePoint<>> IndexBuildTime;
+};
+
 // Detect changes in \p IndexPath file and load new versions of the index
 // whenever they become available.
 void hotReload(clangd::SwapIndex &Index, llvm::StringRef IndexPath,
                llvm::vfs::Status &LastStatus,
-               llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> &FS) {
+               llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> &FS,
+               Monitor &Monitor) {
   auto Status = FS->status(IndexPath);
   // Requested file is same as loaded index: no reload is needed.
   if (!Status || (Status->getLastModificationTime() ==
                       LastStatus.getLastModificationTime() &&
                   Status->getSize() == LastStatus.getSize()))
     return;
   vlog("Found different index version: existing index was modified at "
        "{0}, new index was modified at {1}. Attempting to reload.",
        LastStatus.getLastModificationTime(), Status->getLastModificationTime());
   LastStatus = *Status;
   std::unique_ptr<clang::clangd::SymbolIndex> NewIndex = loadIndex(IndexPath);
   if (!NewIndex) {
     elog("Failed to load new index. Old index will be served.");
     return;
   }
   Index.reset(std::move(NewIndex));
+  Monitor.updateIndex(Status->getLastModificationTime());
   log("New index version loaded. Last modification time: {0}, size: {1} bytes.",
       Status->getLastModificationTime(), Status->getSize());
 }
 
 void runServerAndWait(clangd::SymbolIndex &Index, llvm::StringRef ServerAddress,
-                      llvm::StringRef IndexPath) {
+                      llvm::StringRef IndexPath, Monitor &Monitor) {
   RemoteIndexServer Service(Index, IndexRoot);
 
   grpc::EnableDefaultHealthCheckService(true);
 #if ENABLE_GRPC_REFLECTION
   grpc::reflection::InitProtoReflectionServerBuilderPlugin();
 #endif
   grpc::ServerBuilder Builder;
   Builder.AddListeningPort(ServerAddress.str(),
                            grpc::InsecureServerCredentials());
   Builder.AddChannelArgument(GRPC_ARG_MAX_CONNECTION_IDLE_MS,
                              IdleTimeoutSeconds * 1000);
   Builder.RegisterService(&Service);
+  Builder.RegisterService(&Monitor);
   std::unique_ptr<grpc::Server> Server(Builder.BuildAndStart());
   log("Server listening on {0}", ServerAddress);
 
   std::thread ServerShutdownWatcher([&]() {
     static constexpr auto WatcherFrequency = std::chrono::seconds(5);
     while (!clang::clangd::shutdownRequested())
       std::this_thread::sleep_for(WatcherFrequency);
     Server->Shutdown();
int main(int argc, char *argv[]) {
 
   auto SymIndex = clang::clangd::loadIndex(IndexPath);
   if (!SymIndex) {
     llvm::errs() << "Failed to open the index.\n";
     return -1;
   }
   clang::clangd::SwapIndex Index(std::move(SymIndex));
 
-  std::thread HotReloadThread([&Index, &Status, &FS]() {
+  Monitor Monitor(Status->getLastModificationTime());
+
+  std::thread HotReloadThread([&Index, &Status, &FS, &Monitor]() {
     llvm::vfs::Status LastStatus = *Status;
     static constexpr auto RefreshFrequency = std::chrono::seconds(30);
     while (!clang::clangd::shutdownRequested()) {
-      hotReload(Index, llvm::StringRef(IndexPath), LastStatus, FS);
+      hotReload(Index, llvm::StringRef(IndexPath), LastStatus, FS, Monitor);
       std::this_thread::sleep_for(RefreshFrequency);
     }
   });
 
-  runServerAndWait(Index, ServerAddress, IndexPath);
+  runServerAndWait(Index, ServerAddress, IndexPath, Monitor);
 
   HotReloadThread.join();
 }