kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:46
+// Tracks end-to-end latency of high level lsp calls.
+constexpr trace::Metric LSPLatencies("lsp_latencies",
----------------
sammccall wrote:
> sammccall wrote:
> > comment should mention the label schema if it's not configured
> > programmatically
> >
> > (You could also do a trick of having a StringLiteral parameter for the
> > label, even if all you store is `bool WantsLabel` or nothing at all)
> units. We should decide whether units go in the metric name or just a comment.
> (You could also do a trick of having a StringLiteral parameter for the label,
> even if all you store is bool WantsLabel or nothing at all)
did that and put an assertion to ensure labels are provided when needed and
vice versa.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:366
+// Tracks number of renamed symbols per invocation.
+constexpr trace::Metric RenamedSymbolCounts("renamed_symbol_counts",
+ trace::Metric::Distribution);
----------------
sammccall wrote:
> sammccall wrote:
> > We're not counting symbols, we're counting occurrences.
> > rename_occurrences?
> static? and below
why? i thought constexpr already enforced internal linkage(at least for top
level decls).
are you suggesting moving it into the function body and marking static
constexpr?
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:475
if (!Selections)
return CB(Selections.takeError());
llvm::Optional<llvm::Expected<Tweak::Effect>> Effect;
----------------
sammccall wrote:
> we return early here, skipping the metric increment, but not for
> error-conditions below.
>
> We should be either measuring attempts or successes, consistently.
> (For attempts, can do this at the top of applyTweak)
yes i was trying to record only the successful executions. but attempts sounds
better.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:148
/// return a null unique_ptr wrapped into an optional.
llvm::Optional<std::unique_ptr<ParsedAST>> take(Key K) {
+ // Record metric after unlocking the mutex.
----------------
sammccall wrote:
> you could consider passing the metric in here so we can distinguish between
> retrieval for diagnostics and retrieval for actions
did that. apart from tracking read/diag access it is also important to not
track accesses when we are trying to evict the cache.
================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:222
+ llvm::Optional<WithContextValue> WithLatency;
+ if (LatencyMetric) {
+ using Clock = std::chrono::high_resolution_clock;
----------------
sammccall wrote:
> don't we want a catch-all metric so we get the existing span latencies in
> some form?
i put a metric to track all spans that wasn't created with an explicit metric.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78429/new/
https://reviews.llvm.org/D78429
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
