[PATCH] D78429: [clangd] Metric tracking through Tracer

[Kadir Cetinkaya via Phabricator via cfe-commits]

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