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