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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 8 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Trace.h:35
+/// itself to active tracer on destruction.
+struct Metric {
+  enum Type {
----------------
sammccall wrote:
> Conceptually there's a difference between a metric (the thing being measured, such as "latency distribution by method") and the measurement (such as "code completion took 90ms this time").
> The attributes such as name, type, permitted labels are associated with the metric - these are schema. Whereas the measurement is a tuple `(metric, actual label, value)` - it doesn't make sense to have measurements for the same metric with different types. And the metric is immutable while the measurement is transient.
> 
> For the API (to emit data) it seems natural to model this split using constexpr objects for the metrics, and methods for the measurement:
> ```
> constexpr Metric IncomingLatencyMS("/clangd/latency", Distribution);
> ...
> void handleCodeCompletion() {
>   ...
>   IncomingLatencyMS.record(90);
> }
> ```
> (IOW I think our favorite metrics framework got this right)
> 
> 
> For the SPI (to consume data), things are a bit muddier. For I think we want to avoid having a protocol to explicitly register metrics, so there's an attraction to just reporting the flat `(name, type, label, value)` tuple. I also quite like the idea of reusing the type above and reporting this as `(const Metric&, label, value)` which makes extending Metric more natural.
this looks a lot better, thanks! Changing the order of label and value to make it nicer for non-labeled metrics.


================
Comment at: clang-tools-extra/clangd/Trace.h:36
+struct Metric {
+  enum Type {
+    /// Occurence of an event, e.g. cache access.
----------------
sammccall wrote:
> sammccall wrote:
> > I think we're missing an important type: a value directly provided by the measurement, such as memory usage.
> This enum is about defining the relationship between the metric and the measurements, but also about the (related) nature of the metric itself.
> 
> The current names are measurement-centric, which is good for the former but not so much for the latter, and a bit confusing if we set this enum when initializing the *metric*. I'd consider having this be metric-centric and documenting the measurements instead. e.g.
> 
> ```
> enum MetricType {
>   /// A number whose value is meaningful, and may vary over time.
>   /// Each measurement replaces the current value.
>   Value,
> 
>   /// An aggregate number whose rate of change over time is meaningful.
>   /// Each measurement is an increment for the counter.
>   Counter,
> 
>   /// A distribution of values with a meaningful mean and count.
>   /// Each measured value is a sample for the distribution.
>   /// The distribution is assumed not to vary, samples are aggregated over time.
>   Distribution,
> };
> ```
> I think we're missing an important type: a value directly provided by the measurement, such as memory usage.

I was planning to extend with this later on when we added memory tracking metrics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78429