Page MenuHomePhabricator

Introduce clangd-server-monitor tool
ClosedPublic

Authored by kbobyrev on Thu, Apr 29, 2:40 AM.

Diff Detail

Event Timeline

kbobyrev created this revision.Thu, Apr 29, 2:40 AM
kbobyrev requested review of this revision.Thu, Apr 29, 2:40 AM

thanks for doing this! can we also have a lit test? we already have some tests spinning up an index-server, i suppose we can make an extra rpc to those and ensure we get a success response?

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

s/server/index-server/

being more specific would be nice, just in case we have more servers (i.e. review) in the future.

clang-tools-extra/clangd/index/remote/monitor/Monitor.cpp
55

nit: elog

60
kbobyrev updated this revision to Diff 341922.Fri, Apr 30, 8:25 AM
kbobyrev marked 3 inline comments as done.

Address review comments.

kbobyrev updated this revision to Diff 341925.Fri, Apr 30, 8:36 AM

Inline gRPC deadline.

kadircet added inline comments.Mon, May 3, 1:32 AM
clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
76

logs for index-server and monitor gets intertwined (as we put logs from index-server until initialization above). so can we output monitoring logs after we've dumped all the logs from index-server?

94

can we just dump the stdout to main process (as we do with other subprocesses) and check for the output in the lit tests instead? that way we can check for particular values without complicating the helper instead

kbobyrev updated this revision to Diff 342688.Tue, May 4, 3:16 AM
kbobyrev marked 2 inline comments as done.

Address review comments

kbobyrev updated this revision to Diff 342689.Tue, May 4, 3:17 AM

Remove (now) unused Python JSON import.

kadircet accepted this revision.Tue, May 4, 3:33 AM

thanks, let's ship it!

clang-tools-extra/clangd/index/remote/monitor/Monitor.cpp
16

nit: this is probably not needed now as we dropped formatv

73

nit: i am not sure if this really compiles (was testing it for status-updater patch on gcp). as this is a stringpiece, so you might wanna call ToString/as_string on it.

clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
33

nit: as discussed offline this is only to save some runtime on tests that don't care about it. I'd probably drop this and add later if we feel the need, but up to you.

This revision is now accepted and ready to land.Tue, May 4, 3:33 AM
kbobyrev updated this revision to Diff 342692.Tue, May 4, 3:40 AM
kbobyrev marked 3 inline comments as done.

Resolve post-LGTM comments.

kbobyrev updated this revision to Diff 342693.Tue, May 4, 3:47 AM

Remove leftover --with-monitor from the lit test.

This revision was landed with ongoing or failed builds.Tue, May 4, 3:48 AM
This revision was automatically updated to reflect the committed changes.