kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:13
+import "google/protobuf/empty.proto";
import "Index.proto";
----------------
sammccall wrote:
> 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.
+1 (i also had a comment about it but forgot to hit submit)
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:16
+message MonitoringInfoReply { optional string info = 1; }
+
----------------
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.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:29
+
+ rpc MonitoringInfo(google.protobuf.Empty) returns (MonitoringInfoReply) {}
}
----------------
sammccall wrote:
> sammccall wrote:
> > 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)
> 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.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98246/new/
https://reviews.llvm.org/D98246
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
