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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I like this a lot!



================
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",
----------------
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)


================
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:
> 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.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:47
+// Tracks end-to-end latency of high level lsp calls.
+constexpr trace::Metric LSPLatencies("lsp_latencies",
+                                     trace::Metric::Distribution);
----------------
nit: I'm not sure pluralizing metric names makes sense/is helpful, seems  likely to make them less regular


================
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);
----------------
We're not counting symbols, we're counting occurrences.
rename_occurrences?


================
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:
> We're not counting symbols, we're counting occurrences.
> rename_occurrences?
static? and below


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:430
+// id.
+constexpr trace::Metric ShownCodeactions("shown_codeaction_count",
+                                         trace::Metric::Counter);
----------------
this is an internal name, mixing terminology seems unneccesary
tweak_available ?

I also like noun first then verb, so we get related metrics grouped alphabetically


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:464
+// id.
+constexpr trace::Metric ExecutedTweaks("executed_tweaks",
+                                       trace::Metric::Counter);
----------------
suggest "tweak_success" or "tweak_attempt" depending on what we're measuring


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:475
         if (!Selections)
           return CB(Selections.takeError());
         llvm::Optional<llvm::Expected<Tweak::Effect>> Effect;
----------------
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)


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:497
           }
+          ExecutedTweaks.record(1, TweakID);
         }
----------------
nit: this should be outside the if


================
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.
----------------
you could consider passing the metric in here so we can distinguish between retrieval for diagnostics and retrieval for actions


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:222
+  llvm::Optional<WithContextValue> WithLatency;
+  if (LatencyMetric) {
+    using Clock = std::chrono::high_resolution_clock;
----------------
don't we want a catch-all metric so we get the existing span latencies in some form?


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:227
+          LatencyMetric->record(
+              std::chrono::duration_cast<std::chrono::milliseconds>(
+                  Clock::now() - StartTime)
----------------
if we're recording floating point numbers, seconds seems less arbitrary than ms, and mayxe less prone to confusion


================
Comment at: clang-tools-extra/clangd/support/Trace.h:53
+
+  /// Records a measurement for this metric to active tracer.
+  void record(double Value, llvm::StringRef Label = "") const;
----------------
define label here or as part of the class definition
(in particular, the idea that metrics are recorded per-label and also aggregated across all labels)


================
Comment at: clang-tools-extra/clangd/support/Trace.h:57
+  /// Uniquely identifies the metric. Should use snake_case identifiers, can use
+  /// slashes for hierarchy if needed. e.g. method_latency, foo.bar.
+  const llvm::StringLiteral Name;
----------------
text says slashes, example has dots


================
Comment at: clang-tools-extra/clangd/support/Trace.h:62
+
 /// A consumer of trace events. The events are produced by Spans and trace::log.
 /// Implementations of this interface must be thread-safe.
----------------
trace events and metric measurements....
, the measurements are produced by Metric::record().


================
Comment at: clang-tools-extra/clangd/support/Trace.h:73
   /// The args are *Args, only complete when the event ends.
   virtual Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args) = 0;
   // Called when a Span is destroyed (it may still be active on other threads).
----------------
now we have several logically-independent functions here, consider providing a no-op implementation (return Context::current().clone()) for this and the other methods (and remove them from the test)


================
Comment at: clang-tools-extra/clangd/support/Trace.h:84
+
+  /// Called whenever an event exports a measurement.
+  virtual void record(const Metric &Metric, double Value,
----------------
event -> metric
exports -> records


================
Comment at: clang-tools-extra/clangd/support/Trace.h:121
+  /// If \p LatencyMetric is non-null, it will receive a measurement reflecting
+  /// the spans lifetime. Label of measurement will be \p Name.
+  Span(llvm::Twine Name, const Metric *LatencyMetric = nullptr);
----------------
units (suggest seconds)


================
Comment at: clang-tools-extra/clangd/support/Trace.h:122
+  /// the spans lifetime. Label of measurement will be \p Name.
+  Span(llvm::Twine Name, const Metric *LatencyMetric = nullptr);
   ~Span();
----------------
I'd consider making these overloads so we don't have to use a pointer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78429