Page MenuHomePhabricator

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

Authored by kbobyrev on Mar 9 2021, 3:26 AM.

Details

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

Diff Detail

Event Timeline

kbobyrev created this revision.Mar 9 2021, 3:26 AM
kbobyrev requested review of this revision.Mar 9 2021, 3:26 AM
kbobyrev updated this revision to Diff 329312.Mar 9 2021, 6:06 AM

Use reflection, improve message format.

Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 6:06 AM
kbobyrev edited the summary of this revision. (Show Details)Mar 9 2021, 6:10 AM
kbobyrev edited the summary of this revision. (Show Details)
sammccall added inline comments.Mar 9 2021, 2:05 PM
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)

kadircet added inline comments.Mar 10 2021, 12:39 AM
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 added inline comments.Mar 10 2021, 12:51 AM
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 updated this revision to Diff 329865.Mar 11 2021, 12:57 AM
kbobyrev marked 8 inline comments as done.

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.

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 updated this revision to Diff 330681.Mar 15 2021, 9:03 AM
kbobyrev marked 6 inline comments as done.

Resolve review comments.

Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 9:03 AM
kadircet added inline comments.Mar 15 2021, 12:55 PM
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 updated this revision to Diff 330791.Mar 15 2021, 1:26 PM
kbobyrev marked 7 inline comments as done.

Address comments, rebase on top of main.

kbobyrev added inline comments.Mar 15 2021, 1:26 PM
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 added inline comments.Mar 16 2021, 3:28 AM
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 updated this revision to Diff 330937.Mar 16 2021, 4:32 AM
kbobyrev marked 3 inline comments as done.

Resolve review comments.

kadircet accepted this revision.Mar 16 2021, 4:47 AM

thanks! let's ship it!

This revision is now accepted and ready to land.Mar 16 2021, 4:47 AM
kbobyrev edited the summary of this revision. (Show Details)Mar 16 2021, 5:36 AM
This revision was landed with ongoing or failed builds.Mar 16 2021, 5:38 AM
This revision was automatically updated to reflect the committed changes.