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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Awesome, ship it!

... though, how do you feel about testing the actual metrics we export?

Suggest a slightly generalized TestTracer that installs itself (RAII), and has a

  // Returns recorded values for a metric, and clears then.
  vector<double> TestTracer::take(StringRef Metric, StringRef Label="");

Then it should be pretty simple to add assertions to a few existing tests.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:49
+constexpr trace::Metric LSPLatency("lsp_latency", trace::Metric::Distribution,
+                                   "operation_name");
+
----------------
Nit: operation->method, as they're called in JSON-RPC


================
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);
----------------
kadircet wrote:
> 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?
You're right, I was confused about constexpr.

That said, for the ones used inside one function, restricting the scope as you suggest seems nice.

(We can also inline them entirely if only used once, but maybe some level of consistency is better)


================
Comment at: clang-tools-extra/clangd/support/Trace.h:95
+  /// Called whenever a metrics records a measurement.
+  virtual void record(const Metric &Metric, double Value,
+                      llvm::StringRef Label = "") {}
----------------
Remove the default argument here. Virtual + default is a little confusing and we're always going to pass it anyway


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78429/new/

https://reviews.llvm.org/D78429