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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
