[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG731043c0c494: [clangd] Add more logs and attach tracers to 
remote index server routines (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -6,10 +6,13 @@
 //
 //===--===//
 
+#include "Index.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -36,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TraceFile(
+"trace-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output in the trace"),
+llvm::cl::init(false),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -60,66 +73,90 @@
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
+trace::Span Tracer(LookupRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError());
   return grpc::Status::CANCELLED;
 }
-Index->lookup(*Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+Index->lookup(*Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer(FuzzyFindRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse FuzzyFindRequest from protobuf: {0}",
Req.takeError());
   return grpc::Status::CANCELLED;
 }
-bool HasMore = Index->fuzzyFind(*Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = Index->fuzzyFind(*Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
 grpc::ServerWriter *Reply) override {
+trace::Span Tracer(RefsRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse RefsRequest from protobuf: {0}", Req.takeError());
   return grpc::Status::CANCELLED;
 }
-bool HasMore = Index->refs(*Req, [&](const clangd::Ref ) {
-  auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference);
-  if (!SerializedRef)
+unsigned Sent = 0;
+unsigned FailedToSend = 

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280997.
kbobyrev marked an inline comment as done.
kbobyrev added a comment.

Switch from llvm::errs() to elog()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -6,10 +6,13 @@
 //
 //===--===//
 
+#include "Index.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -36,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TraceFile(
+"trace-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output in the trace"),
+llvm::cl::init(false),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -60,66 +73,90 @@
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
+trace::Span Tracer(LookupRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError());
   return grpc::Status::CANCELLED;
 }
-Index->lookup(*Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+Index->lookup(*Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer(FuzzyFindRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse FuzzyFindRequest from protobuf: {0}",
Req.takeError());
   return grpc::Status::CANCELLED;
 }
-bool HasMore = Index->fuzzyFind(*Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = Index->fuzzyFind(*Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
 grpc::ServerWriter *Reply) override {
+trace::Span Tracer(RefsRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse RefsRequest from protobuf: {0}", Req.takeError());
   return grpc::Status::CANCELLED;
 }
-bool HasMore = Index->refs(*Req, [&](const clangd::Ref ) {
-  auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference);
-  if (!SerializedRef)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = Index->refs(*Req, [&](const 

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev marked an inline comment as not done.
kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:203
+  TracerStream.reset();
+  llvm::errs() << "Error while opening trace file " << TraceFile << ": "
+   << EC.message();

kadircet wrote:
> any reasons for not using elog in here (and in other places using 
> `llvm::errs()` ?
I think we tend to use `llvm::errs()` instead of logs in CLI tools 
(ClangdMain.cpp and particularly `TraceFile` handling were the inspiration and 
it uses `llvm::errs()` although there are many `elog`s there, too) and I'm a 
little confused about when to use either of those. Could you explain how those 
are chosen in each case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280993.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Resolve all but 1 post-LGTM comment and rebase on top of master.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -6,10 +6,13 @@
 //
 //===--===//
 
+#include "Index.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -36,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TraceFile(
+"trace-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output in the trace"),
+llvm::cl::init(false),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -60,66 +73,90 @@
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
+trace::Span Tracer(LookupRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError());
   return grpc::Status::CANCELLED;
 }
-Index->lookup(*Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+Index->lookup(*Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer(FuzzyFindRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse FuzzyFindRequest from protobuf: {0}",
Req.takeError());
   return grpc::Status::CANCELLED;
 }
-bool HasMore = Index->fuzzyFind(*Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = Index->fuzzyFind(*Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
 grpc::ServerWriter *Reply) override {
+trace::Span Tracer(RefsRequest::descriptor()->name());
 auto Req = ProtobufMarshaller->fromProtobuf(Request);
 if (!Req) {
   elog("Can not parse RefsRequest from protobuf: {0}", Req.takeError());
   return grpc::Status::CANCELLED;
 }
-bool HasMore = Index->refs(*Req, [&](const clangd::Ref ) {
-  auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference);
-  if (!SerializedRef)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = 

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, LGTM apart from templated-lambdas.




Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:86
+unsigned FailedToSend = 0;
+Index->lookup(Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);

usage of `auto` in here results in a templated-lambda. I don't think we want 
that (please let me know if I am missing something) maybe go back to using 
clangd::Symbol ? (same for other lambdas)



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:203
+  TracerStream.reset();
+  llvm::errs() << "Error while opening trace file " << TraceFile << ": "
+   << EC.message();

any reasons for not using elog in here (and in other places using 
`llvm::errs()` ?



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:122
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function kbobyrev wrote:
> > kadircet wrote:
> > > all of this trickery is really nice. but i am not sure if it improves 
> > > readability at all, from the perspective of call sites the amount of code 
> > > is almost the same. moreover it was some trivial lambda before and 
> > > instead it is lots of template parameters now.
> > > 
> > > maybe just have templated function to replace the lambda body instead? 
> > > e.g.
> > > 
> > > ```
> > > template 
> > > void IndexCallback(Marshaller , StreamType ) {
> > >   trace::Span Tracer;
> > >   auto SerializedSymbol = M.toProtobuf(Item);
> > >   // log the events and much more ...
> > > }
> > > 
> > > ```
> > > 
> > > then call it in the callbacks. WDYT?
> > I think this is a good idea but I can't think of a nice way to do that, 
> > too. `IndexCallback` would have to update `Sent` and `FailedToSend` 
> > (`Tracer` should be outside of the callback, right?), and the callback 
> > signature if fixed so I wouldn't be able to pass these parameters by 
> > reference :( I could probably make those two fields of the class but this 
> > doesn't look very nice, or pass member function (templated one) to the 
> > callback in each index request, but this also doesn't look very nice.
> > 
> > I think the reason was initially to improve readability but then it's more 
> > about code deduplication: right now there are 3 pieces of code that do 
> > exactly the same, there will be a fourth one (relations request). Maybe 
> > readability even decreases but I really think that having 4 pieces of code 
> > with different types is not very nice. Also, D84525 compliments this and 
> > there will be practically only no functional code in the functions 
> > themselves.
> > 
> > I can understand your argument and I partially support it but I really 
> > think we're unfortunately choosing between bad and worse.
> FWIW, I find the template to be very hard to read, and would prefer the 
> version that is duplicated 4 times. 
> 
> I've left some suggestions for how to simplify (mostly, eliminating template 
> parameters) but I'm still not sure it's worth it.
> I think this is a good idea but I can't think of a nice way to do that, too. 
> IndexCallback would have to update Sent and FailedToSend (Tracer should be 
> outside of the callback, right?), and the callback signature if fixed so I 
> wouldn't be able to pass these parameters by reference :( I could probably 
> make those two fields of the class but this doesn't look very nice, or pass 
> member function (templated one) to the callback in each index request, but 
> this also doesn't look very nice.

right, i've missed that bit. but I was suggesting you to call `IndexCallback` 
inside the lambda, e.g. callsites would look like this:

```
Index->lookup(Req, [&](const auto ) {
IndexCallback(*ProtobufMarshaller, Item);
});
```

so in theory you could pass in `Sent` and `FailedToSend` as parameters to 
`IndexCallback` and mutate them in there.


but not really relevant anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280848.
kbobyrev added a comment.

Fix code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -6,9 +6,13 @@
 //
 //===--===//
 
+#include "Index.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -35,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TraceFile(
+"trace-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output in the trace"),
+llvm::cl::init(false),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -59,6 +73,7 @@
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
+trace::Span Tracer(LookupRequest::descriptor()->name());
 clangd::LookupRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
@@ -66,40 +81,56 @@
 return grpc::Status::CANCELLED;
   Req.IDs.insert(*SID);
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+Index->lookup(Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer(FuzzyFindRequest::descriptor()->name());
 const auto Req = ProtobufMarshaller->fromProtobuf(Request);
-bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = Index->fuzzyFind(Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
 grpc::ServerWriter *Reply) override {
+trace::Span Tracer(RefsRequest::descriptor()->name());
 clangd::RefsRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
@@ -107,17 +138,24 @@
 return grpc::Status::CANCELLED;
   Req.IDs.insert(*SID);
 }
-bool HasMore = Index->refs(Req, [&](const clangd::Ref ) {
-  auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference);
-  if (!SerializedRef)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = Index->refs(Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   RefsReply NextMessage;

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev marked 3 inline comments as done.
kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:73
 private:
+  template 

kbobyrev wrote:
> sammccall wrote:
> > you don't need both RequestT and ClangdRequest as template params:
> >  - the things you're currently doing are all available through 
> > protobuf::Message base class
> >  - but why not move the request fromProtobuf call in here so you only need 
> > to pass a single parameter?
> > but why not move the request fromProtobuf call in here so you only need to 
> > pass a single parameter?
> Yeah this is the idea; I have D84525 for extending `fromProtobuf` so that it 
> can be moved here.
Not doing this because of getting back to code duplication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280846.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.

Move back to duplicated code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -6,9 +6,13 @@
 //
 //===--===//
 
+#include "Index.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -16,6 +20,7 @@
 
 #include 
 #include 
+#include 
 
 #include "Index.grpc.pb.h"
 
@@ -35,6 +40,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TraceFile(
+"trace-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output in the trace"),
+llvm::cl::init(false),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -59,6 +74,7 @@
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
+trace::Span Tracer(LookupRequest::descriptor()->name());
 clangd::LookupRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
@@ -66,58 +82,83 @@
 return grpc::Status::CANCELLED;
   Req.IDs.insert(*SID);
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+Index->lookup(Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer(LookupRequest::descriptor()->name());
 const auto Req = ProtobufMarshaller->fromProtobuf(Request);
-bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+bool HasMore = Index->fuzzyFind(Req, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
 return;
+  }
   FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
 grpc::ServerWriter *Reply) override {
+trace::Span Tracer(LookupRequest::descriptor()->name());
 clangd::RefsRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Refs request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
-bool HasMore = Index->refs(Req, [&](const clangd::Ref ) {
-  auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference);
-  if (!SerializedRef)
+unsigned Sent = 0;
+unsigned 

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

As discussed in PM, should move back to duplicated code 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:73
 private:
+  template 

sammccall wrote:
> you don't need both RequestT and ClangdRequest as template params:
>  - the things you're currently doing are all available through 
> protobuf::Message base class
>  - but why not move the request fromProtobuf call in here so you only need to 
> pass a single parameter?
> but why not move the request fromProtobuf call in here so you only need to 
> pass a single parameter?
Yeah this is the idea; I have D84525 for extending `fromProtobuf` so that it 
can be moved here.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:75
+typename IndexCall, typename StreamType, typename CallbackT>
+  typename std::result_of::type

sammccall wrote:
> can't the return type just be `auto`?
> Then I think you don't need CallbackT as a template param.
Ah, I thought LLVM is still C++ 11, not 14. Good to know, thanks!



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:96
+} Counter(Request->DebugString());
+return std::forward(Call)(
+*Index, ClangdRequest, [&](const StreamType ) {

sammccall wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > I don't think you need forward here... just take the param by const 
> > > reference?
> > How do I do that? The problem is that some functions return `bool` and one 
> > `void`, so I can't really assign the result to variable and then return it.
> Currently you have 
> ```
> template (IndexCall&& Call) {
>   forward(Call);
> }
> ```
> but I think this would work fine here:
> ```
> template (const IndexCall& Call) {
>   IndexCall(Call);
> }
> ```
> (nothing particularly wrong with this use of forward, but "more template 
> goop" feels particularly expensive here.
Ah, you meant passing the callback by const reference. I see, thanks for the 
suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280812.
kbobyrev marked 14 inline comments as done.
kbobyrev added a comment.

Resolve most review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -8,7 +8,10 @@
 
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -16,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include "Index.grpc.pb.h"
 
@@ -35,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TraceFile(
+"trace-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output in the trace"),
+llvm::cl::init(false),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -56,24 +70,60 @@
   }
 
 private:
+  template 
+  auto streamRPC(const RequestT *Request, grpc::ServerWriter *Reply,
+ ClangdRequestT ClangdRequest, const IndexCall ) {
+// Adding Sent and FailedToSend messages to the tracer has to happen after
+// Callback but the temporary value can not be stored. Use a dummy struct
+// to hold data for tracer and then add data to the trace on destruction
+// happening after the callback.
+struct TracerCounter {
+  trace::Span Tracer;
+  unsigned Sent = 0;
+  unsigned FailedToSend = 0;
+  TracerCounter(llvm::StringRef RequestInfo)
+  : Tracer(RequestT::descriptor()->name()) {
+SPAN_ATTACH(Tracer, "Request", RequestInfo);
+  }
+  ~TracerCounter() {
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
+  }
+} Counter(Request->DebugString());
+return Call(*Index, ClangdRequest, [&](const auto ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++Counter.FailedToSend;
+return;
+  }
+  ReplyT NextMessage;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
+  Reply->Write(NextMessage);
+  ++Counter.Sent;
+});
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
 clangd::LookupRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Lookup request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::lookup;
+streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
@@ -84,14 +134,14 @@
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
 const auto Req = ProtobufMarshaller->fromProtobuf(Request);
-bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::fuzzyFind;
+bool HasMore = streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
@@ -103,18 +153,20 @@
 clangd::RefsRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Refs request 

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Need to resolve remaining comments and improve readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:96
+} Counter(Request->DebugString());
+return std::forward(Call)(
+*Index, ClangdRequest, [&](const StreamType ) {

kbobyrev wrote:
> sammccall wrote:
> > I don't think you need forward here... just take the param by const 
> > reference?
> How do I do that? The problem is that some functions return `bool` and one 
> `void`, so I can't really assign the result to variable and then return it.
Currently you have 
```
template (IndexCall&& Call) {
  forward(Call);
}
```
but I think this would work fine here:
```
template (const IndexCall& Call) {
  IndexCall(Call);
}
```
(nothing particularly wrong with this use of forward, but "more template goop" 
feels particularly expensive here.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:49
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),
+};

kbobyrev wrote:
> sammccall wrote:
> > kadircet wrote:
> > > this sounds like a debug feature, do we really want it to be true by 
> > > default?
> > This option is not worth having at all, IMO.
> Why not? It's relatively hard to read trace files otherwise. Do you think 
> they should be pretty-printed by default?
FWIW, I've never had to read them except when initially developing/debugging 
the trace feature.
(The most I have to do is fix up a broken end-of-file, and events are 
one-per-line in non-pretty anyway...)

I think either always-on or always-off is better than having a flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:96
+} Counter(Request->DebugString());
+return std::forward(Call)(
+*Index, ClangdRequest, [&](const StreamType ) {

sammccall wrote:
> I don't think you need forward here... just take the param by const reference?
How do I do that? The problem is that some functions return `bool` and one 
`void`, so I can't really assign the result to variable and then return it.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:49
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),
+};

sammccall wrote:
> kadircet wrote:
> > this sounds like a debug feature, do we really want it to be true by 
> > default?
> This option is not worth having at all, IMO.
Why not? It's relatively hard to read trace files otherwise. Do you think they 
should be pretty-printed by default?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:73
 private:
+  template 

you don't need both RequestT and ClangdRequest as template params:
 - the things you're currently doing are all available through 
protobuf::Message base class
 - but why not move the request fromProtobuf call in here so you only need to 
pass a single parameter?



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:74
+  template 
+  typename std::result_of
+  typename std::result_of::type

can't the return type just be `auto`?
Then I think you don't need CallbackT as a template param.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:96
+} Counter(Request->DebugString());
+return std::forward(Call)(
+*Index, ClangdRequest, [&](const StreamType ) {

I don't think you need forward here... just take the param by const reference?



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:42
 
+llvm::cl::opt TracerFile(
+"tracer-file",

kbobyrev wrote:
> kadircet wrote:
> > i think `TraceFile` is more appropriate here.
> `TracerFile` is what `ClangdMain.cpp` has; I'm not against this, but I think 
> it'd be better to have them the same for consistency. WDYT?
(driveby, sorry)
TracerFile is the local variable name in ClangdMain, but the public interface 
(an environment variable there rather than a flag) is CLANGD_TRACE, and it's 
more important to be consistent with that.

So my vote would be --trace or --trace-file or env CLANGD_TRACE=...



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:49
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),
+};

kadircet wrote:
> this sounds like a debug feature, do we really want it to be true by default?
This option is not worth having at all, IMO.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:122
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function kadircet wrote:
> > all of this trickery is really nice. but i am not sure if it improves 
> > readability at all, from the perspective of call sites the amount of code 
> > is almost the same. moreover it was some trivial lambda before and instead 
> > it is lots of template parameters now.
> > 
> > maybe just have templated function to replace the lambda body instead? e.g.
> > 
> > ```
> > template 
> > void IndexCallback(Marshaller , StreamType ) {
> >   trace::Span Tracer;
> >   auto SerializedSymbol = M.toProtobuf(Item);
> >   // log the events and much more ...
> > }
> > 
> > ```
> > 
> > then call it in the callbacks. WDYT?
> I think this is a good idea but I can't think of a nice way to do that, too. 
> `IndexCallback` would have to update `Sent` and `FailedToSend` (`Tracer` 
> should be outside of the callback, right?), and the callback signature if 
> fixed so I wouldn't be able to pass these parameters by reference :( I could 
> probably make those two fields of the class but this doesn't look very nice, 
> or pass member function (templated one) to the callback in each index 
> request, but this also doesn't look very nice.
> 
> I think the reason was initially to improve readability but then it's more 
> about code deduplication: right now there are 3 pieces of code that do 
> exactly the same, there will be a fourth one (relations request). Maybe 
> readability even decreases but I really think that having 4 pieces of code 
> with different types is not very nice. Also, D84525 compliments this and 
> there will be practically only no functional code in the functions themselves.
> 
> I can understand your argument and I partially support it but I really think 
> we're unfortunately choosing between bad and worse.
FWIW, I find the template to be very hard to read, and would prefer the version 
that is duplicated 4 times. 

I've left some suggestions for how to simplify (mostly, eliminating template 
parameters) but I'm still not sure it's worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:61
   Callback(*Response);
+  ++Received;
 }

kadircet wrote:
> shouldn't we increment this before the `continue` above ? (or rename this to 
> `successful` rather than `received` ?)
Makes sense, I didn't think of "received" in terms of "overall" :P



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:42
 
+llvm::cl::opt TracerFile(
+"tracer-file",

kadircet wrote:
> i think `TraceFile` is more appropriate here.
`TracerFile` is what `ClangdMain.cpp` has; I'm not against this, but I think 
it'd be better to have them the same for consistency. WDYT?



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:48
+"pretty",
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),

kadircet wrote:
> is it only for pretty printing trace files? do we expect to have any other 
> components this will interact in future? (if not maybe state that in the 
> comments)
Yeah, right now it's only for trace files. It's quite possible that there will 
be something else that could be pretty-printed in the future, but not right 
now, so I changed description (specified trace files).



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:122
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function all of this trickery is really nice. but i am not sure if it improves 
> readability at all, from the perspective of call sites the amount of code is 
> almost the same. moreover it was some trivial lambda before and instead it is 
> lots of template parameters now.
> 
> maybe just have templated function to replace the lambda body instead? e.g.
> 
> ```
> template 
> void IndexCallback(Marshaller , StreamType ) {
>   trace::Span Tracer;
>   auto SerializedSymbol = M.toProtobuf(Item);
>   // log the events and much more ...
> }
> 
> ```
> 
> then call it in the callbacks. WDYT?
I think this is a good idea but I can't think of a nice way to do that, too. 
`IndexCallback` would have to update `Sent` and `FailedToSend` (`Tracer` should 
be outside of the callback, right?), and the callback signature if fixed so I 
wouldn't be able to pass these parameters by reference :( I could probably make 
those two fields of the class but this doesn't look very nice, or pass member 
function (templated one) to the callback in each index request, but this also 
doesn't look very nice.

I think the reason was initially to improve readability but then it's more 
about code deduplication: right now there are 3 pieces of code that do exactly 
the same, there will be a fourth one (relations request). Maybe readability 
even decreases but I really think that having 4 pieces of code with different 
types is not very nice. Also, D84525 compliments this and there will be 
practically only no functional code in the functions themselves.

I can understand your argument and I partially support it but I really think 
we're unfortunately choosing between bad and worse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280587.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.

Resolve most review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -8,7 +8,10 @@
 
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -16,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include "Index.grpc.pb.h"
 
@@ -35,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TracerFile(
+"tracer-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output in the trace"),
+llvm::cl::init(false),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -56,24 +70,63 @@
   }
 
 private:
+  template 
+  typename std::result_of::type
+  streamRPC(const RequestT *Request, grpc::ServerWriter *Reply,
+ClangdRequestT ClangdRequest, IndexCall &) {
+// Adding Sent and FailedToSend messages to the tracer has to happen after
+// Callback but the temporary value can not be stored. Use a dummy struct
+// to hold data for tracer and then add data to the trace on destruction
+// happening after the callback.
+struct TracerCounter {
+  trace::Span Tracer;
+  unsigned Sent = 0;
+  unsigned FailedToSend = 0;
+  TracerCounter(llvm::StringRef RequestInfo)
+  : Tracer(RequestT::descriptor()->name()) {
+SPAN_ATTACH(Tracer, "Request", RequestInfo);
+  }
+  ~TracerCounter() {
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
+  }
+} Counter(Request->DebugString());
+return std::forward(Call)(
+*Index, ClangdRequest, [&](const StreamType ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++Counter.FailedToSend;
+return;
+  }
+  ReplyT NextMessage;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
+  Reply->Write(NextMessage);
+  ++Counter.Sent;
+});
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
 clangd::LookupRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Lookup request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::lookup;
+streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
@@ -84,14 +137,15 @@
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
 const auto Req = ProtobufMarshaller->fromProtobuf(Request);
-bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::fuzzyFind;
+bool HasMore =
+streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
@@ -103,18 +157,20 @@
 clangd::RefsRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = 

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:61
   Callback(*Response);
+  ++Received;
 }

shouldn't we increment this before the `continue` above ? (or rename this to 
`successful` rather than `received` ?)



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:42
 
+llvm::cl::opt TracerFile(
+"tracer-file",

i think `TraceFile` is more appropriate here.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:48
+"pretty",
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),

is it only for pretty printing trace files? do we expect to have any other 
components this will interact in future? (if not maybe state that in the 
comments)



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:49
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),
+};

this sounds like a debug feature, do we really want it to be true by default?



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:122
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function
void IndexCallback(Marshaller , StreamType ) {
  trace::Span Tracer;
  auto SerializedSymbol = M.toProtobuf(Item);
  // log the events and much more ...
}

```

then call it in the callbacks. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280391.
kbobyrev added a comment.

Fix (un)successful message numbers reporting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -8,7 +8,10 @@
 
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -16,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include "Index.grpc.pb.h"
 
@@ -35,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TracerFile(
+"tracer-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -56,24 +70,63 @@
   }
 
 private:
+  template 
+  typename std::result_of::type
+  streamRPC(const RequestT *Request, grpc::ServerWriter *Reply,
+ClangdRequestT ClangdRequest, IndexCall &) {
+// Adding Sent and FailedToSend messages to the tracer has to happen after
+// Callback but the temporary value can not be stored. Use a dummy struct
+// to hold data for tracer and then add data to the trace on destruction
+// happening after the callback.
+struct TracerCounter {
+  trace::Span Tracer;
+  unsigned Sent = 0;
+  unsigned FailedToSend = 0;
+  TracerCounter(llvm::StringRef RequestInfo)
+  : Tracer(RequestT::descriptor()->name()) {
+SPAN_ATTACH(Tracer, "Request", RequestInfo);
+  }
+  ~TracerCounter() {
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
+  }
+} Counter(Request->DebugString());
+return std::forward(Call)(
+*Index, ClangdRequest, [&](const StreamType ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++Counter.FailedToSend;
+return;
+  }
+  ReplyT NextMessage;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
+  Reply->Write(NextMessage);
+  ++Counter.Sent;
+});
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
 clangd::LookupRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Lookup request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::lookup;
+streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
@@ -83,15 +136,18 @@
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer("FuzzyFind");
+SPAN_ATTACH(Tracer, "Request", Request->DebugString());
 const auto Req = ProtobufMarshaller->fromProtobuf(Request);
-bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::fuzzyFind;
+bool HasMore =
+streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Tracer messages for sent and failed to parse items are not correct. Have to 
figure out how to print them _after_ the callback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280381.
kbobyrev added a comment.

Abstract out the client request into a single function call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -8,7 +8,10 @@
 
 #include "index/Index.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -16,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include "Index.grpc.pb.h"
 
@@ -35,6 +39,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TracerFile(
+"tracer-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -56,24 +70,52 @@
   }
 
 private:
+  template 
+  typename std::result_of::type
+  streamRPC(const RequestT *Request, grpc::ServerWriter *Reply,
+ClangdRequestT ClangdRequest, IndexCall &) {
+trace::Span Tracer(RequestT::descriptor()->name());
+SPAN_ATTACH(Tracer, "Request", Request->DebugString());
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
+return std::forward(Call)(
+*Index, ClangdRequest, [&](const StreamType ) {
+  auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
+  if (!SerializedItem) {
+++FailedToSend;
+return;
+  }
+  ReplyT NextMessage;
+  *NextMessage.mutable_stream_result() = *SerializedItem;
+  Reply->Write(NextMessage);
+  ++Sent;
+});
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
 clangd::LookupRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Lookup request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
-Index->lookup(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  LookupReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::lookup;
+streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
@@ -83,15 +125,18 @@
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer("FuzzyFind");
+SPAN_ATTACH(Tracer, "Request", Request->DebugString());
 const auto Req = ProtobufMarshaller->fromProtobuf(Request);
-bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol ) {
-  auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
-return;
-  FuzzyFindReply NextMessage;
-  *NextMessage.mutable_stream_result() = *SerializedSymbol;
-  Reply->Write(NextMessage);
-});
+std::function)>
+IndexCall = ::SymbolIndex::fuzzyFind;
+bool HasMore =
+streamRPC>(
+Request, Reply, Req, std::move(IndexCall));
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
@@ -103,18 +148,20 @@
 clangd::RefsRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Refs request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
-bool HasMore = Index->refs(Req, [&](const clangd::Ref ) {
-  auto SerializedRef = 

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

The first iteration is D83831  but the diffs 
are clobbered and I can't change the history there :( So, creating a new one is 
maybe the best solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84499

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

Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -9,6 +9,8 @@
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
@@ -35,6 +37,16 @@
 llvm::cl::opt IndexRoot(llvm::cl::desc(""),
  llvm::cl::Positional, llvm::cl::Required);
 
+llvm::cl::opt TracerFile(
+"tracer-file",
+llvm::cl::desc("Path to the file where tracer logs will be stored"));
+
+llvm::cl::opt PrettyPrint{
+"pretty",
+llvm::cl::desc("Pretty-print JSON output"),
+llvm::cl::init(true),
+};
+
 llvm::cl::opt ServerAddress(
 "server-address", llvm::cl::init("0.0.0.0:50051"),
 llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
@@ -59,65 +71,96 @@
   grpc::Status Lookup(grpc::ServerContext *Context,
   const LookupRequest *Request,
   grpc::ServerWriter *Reply) override {
+trace::Span Tracer("LookupRequest");
+SPAN_ATTACH(Tracer, "Request", Request->DebugString());
 clangd::LookupRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Lookup request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
 Index->lookup(Req, [&](const clangd::Symbol ) {
   auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+  if (!SerializedSymbol) {
+++FailedToSend;
 return;
+  }
   LookupReply NextMessage;
   *NextMessage.mutable_stream_result() = *SerializedSymbol;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 LookupReply LastMessage;
 LastMessage.set_final_result(true);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
  const FuzzyFindRequest *Request,
  grpc::ServerWriter *Reply) override {
+trace::Span Tracer("FuzzyFind");
+SPAN_ATTACH(Tracer, "Request", Request->DebugString());
 const auto Req = ProtobufMarshaller->fromProtobuf(Request);
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
 bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol ) {
   auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-  if (!SerializedSymbol)
+  if (!SerializedSymbol) {
+++FailedToSend;
 return;
+  }
   FuzzyFindReply NextMessage;
   *NextMessage.mutable_stream_result() = *SerializedSymbol;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 FuzzyFindReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
 return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
 grpc::ServerWriter *Reply) override {
+trace::Span Tracer("Refs");
+SPAN_ATTACH(Tracer, "Request", Request->DebugString());
 clangd::RefsRequest Req;
 for (const auto  : Request->ids()) {
   auto SID = SymbolID::fromStr(StringRef(ID));
-  if (!SID)
+  if (!SID) {
+elog("Refs request cancelled: invalid SymbolID {1}", SID.takeError());
 return grpc::Status::CANCELLED;
+  }
   Req.IDs.insert(*SID);
 }
+unsigned Sent = 0;
+unsigned FailedToSend = 0;
 bool HasMore = Index->refs(Req, [&](const clangd::Ref ) {
   auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference);
-  if (!SerializedRef)
+  if (!SerializedRef) {
+++FailedToSend;
 return;
+  }
   RefsReply NextMessage;
   *NextMessage.mutable_stream_result() = *SerializedRef;
   Reply->Write(NextMessage);
+  ++Sent;
 });
 RefsReply LastMessage;
 LastMessage.set_final_result(HasMore);
 Reply->Write(LastMessage);
+SPAN_ATTACH(Tracer, "Sent", Sent);
+