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

16

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

29

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)

29

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

332

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)

16

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.

29

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

16

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

32

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

37

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.

38

i think this fixme belongs to the service implementation.

42

i smell some c/p

46

what's the point of this exactly?

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

338

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

[kbobyrev]

Resolve review comments.

[kadircet]

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

17 ↗

(On Diff #330681)

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)

18 ↗

(On Diff #330681)

what happened to 2 ?

18 ↗

(On Diff #330681)

nit: maybe rename this to index_age

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

307

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

321

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)

325

nit: i'd log after this one.

330

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

437

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

321

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.

437

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

321

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.

437

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/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 "google/protobuf/empty.proto";
 import "Index.proto";
 
+message MonitoringInfoReply { optional string info = 1; }
+
 // 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) {}
+
+  rpc MonitoringInfo(google.protobuf.Empty) returns (MonitoringInfoReply) {}
 }
 

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

 target_link_libraries(clangd-index-server
   PRIVATE
   clangDaemon
   clangdSupport
 
   RemoteIndexProto
   RemoteIndexServiceProto
   clangdRemoteMarshalling
+
+  grpc++_reflection
   )

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 "Index.pb.h"
 #include "Service.grpc.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"
 #include "support/Trace.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Chrono.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/VirtualFileSystem.h"
 
 #include <chrono>
+#include <grpc++/ext/proto_server_reflection_plugin.h>
 #include <grpc++/grpc++.h>
 #include <grpc++/health_check_service_interface.h>
 #include <memory>
 #include <thread>
 
 namespace clang {
 namespace clangd {
 namespace remote {
llvm::cl::opt<size_t> IdleTimeoutSeconds(
     llvm::cl::desc("Maximum time a channel may stay idle until server closes "
                    "the connection, in seconds. Defaults to 480."));
 
 static Key<grpc::ServerContext *> CurrentRequest;
 
 class RemoteIndexServer final : public v1::SymbolIndex::Service {
 public:
   RemoteIndexServer(clangd::SymbolIndex &Index, llvm::StringRef IndexRoot)
-      : Index(Index) {
+      : Index(Index), ServiceStartTime(std::chrono::system_clock::now()) {
     llvm::SmallString<256> NativePath = IndexRoot;
     llvm::sys::path::native(NativePath);
     ProtobufMarshaller = std::unique_ptr<Marshaller>(new Marshaller(
         /*RemoteIndexRoot=*/llvm::StringRef(NativePath),
         /*LocalIndexRoot=*/""));
   }
 
 private:
void logRequestSummary(llvm::StringLiteral RequestName, unsigned Sent,
                          stopwatch::time_point StartTime) {
     auto Duration = stopwatch::now() - StartTime;
     auto Millis =
         std::chrono::duration_cast<std::chrono::milliseconds>(Duration).count();
     log("[public] request {0} => OK: {1} results in {2}ms", RequestName, Sent,
         Millis);
   }
 
+  grpc::Status MonitoringInfo(grpc::ServerContext *Context,
+                              const google::protobuf::Empty *Request,
+                              v1::MonitoringInfoReply *Reply) override {
+    auto Uptime = std::chrono::duration_cast<std::chrono::minutes>(
+                      std::chrono::system_clock::now() - ServiceStartTime)
+                      .count();
+    Reply->set_info(llvm::formatv(
+        "Service started at {0:%Y-%m-%d %H:%M:%S}. Uptime: {1} h {2} m.",
+        ServiceStartTime, Uptime / 60, Uptime % 60));
+    logRequest(*Request);
+    logResponse(*Reply);
+    return grpc::Status::OK;
+  }
+
   std::unique_ptr<Marshaller> ProtobufMarshaller;
   clangd::SymbolIndex &Index;
+  llvm::sys::TimePoint<> ServiceStartTime;
 };
 
 // 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) {
   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));
   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) {
   RemoteIndexServer Service(Index, IndexRoot);
 
   grpc::EnableDefaultHealthCheckService(true);
+  grpc::reflection::InitProtoReflectionServerBuilderPlugin();
   grpc::ServerBuilder Builder;
   Builder.AddListeningPort(ServerAddress.str(),
                            grpc::InsecureServerCredentials());
   Builder.AddChannelArgument(GRPC_ARG_MAX_CONNECTION_IDLE_MS,
                              IdleTimeoutSeconds * 1000);
   Builder.RegisterService(&Service);
   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]() {
     llvm::vfs::Status LastStatus = *Status;
     static constexpr auto RefreshFrequency = std::chrono::seconds(30);
     while (!clang::clangd::shutdownRequested()) {
       hotReload(Index, llvm::StringRef(IndexPath), LastStatus, FS);
       std::this_thread::sleep_for(RefreshFrequency);
     }
   });
 
   runServerAndWait(Index, ServerAddress, IndexPath);
 
   HotReloadThread.join();
 }

llvm/cmake/modules/FindGRPC.cmake

if (GRPC_INSTALL_PATH)
   include_directories(${Protobuf_INCLUDE_DIRS})
 
   # gRPC CMake CONFIG gives the libraries slightly odd names, make them match
   # the conventional system-installed names.
   set_target_properties(protobuf::libprotobuf PROPERTIES IMPORTED_GLOBAL TRUE)
   add_library(protobuf ALIAS protobuf::libprotobuf)
   set_target_properties(gRPC::grpc++ PROPERTIES IMPORTED_GLOBAL TRUE)
   add_library(grpc++ ALIAS gRPC::grpc++)
+  set_target_properties(gRPC::grpc++_reflection PROPERTIES IMPORTED_GLOBAL TRUE)
+  add_library(grpc++_reflection ALIAS gRPC::grpc++_reflection)
 
   set(GRPC_CPP_PLUGIN $<TARGET_FILE:gRPC::grpc_cpp_plugin>)
   set(PROTOC ${Protobuf_PROTOC_EXECUTABLE})
 else()
   # This setup requires system-installed gRPC and Protobuf.
   # We always link dynamically in this mode. While the static libraries are
   # usually installed, the CMake files telling us *which* static libraries to
   # link are not.
if (HOMEBREW)
         list(APPEND PROTOBUF_OPTS PATHS ${PROTOBUF_HOMEBREW_PATH}/lib NO_DEFAULT_PATH)
       endif()
     endif()
   endif()
   find_library(GRPC_LIBRARY grpc++ $GRPC_OPTS REQUIRED)
   add_library(grpc++ UNKNOWN IMPORTED GLOBAL)
   message(STATUS "Using grpc++: " ${GRPC_LIBRARY})
   set_target_properties(grpc++ PROPERTIES IMPORTED_LOCATION ${GRPC_LIBRARY})
+  find_library(GRPC_REFLECTION_LIBRARY grpc++_reflection $GRPC_OPTS REQUIRED)
+  add_library(grpc++_reflection UNKNOWN IMPORTED GLOBAL)
+  set_target_properties(grpc++_reflection PROPERTIES IMPORTED_LOCATION ${GRPC_REFLECTION_LIBRARY})
   find_library(PROTOBUF_LIBRARY protobuf $PROTOBUF_OPTS REQUIRED)
   message(STATUS "Using protobuf: " ${PROTOBUF_LIBRARY})
   add_library(protobuf UNKNOWN IMPORTED GLOBAL)
   set_target_properties(protobuf PROPERTIES IMPORTED_LOCATION ${PROTOBUF_LIBRARY})
 endif()
 
 # Proto headers are generated in ${CMAKE_CURRENT_BINARY_DIR}.
 # Libraries that use these headers should adjust the include path.