conradludgate / measured

A better metrics crate
26 stars 3 forks source link

Add a `prefix` setting to metric sets #2

Closed clux closed 3 months ago

clux commented 3 months ago

Currently metric names that typically share a common prefix need to have them all set individually via:

#[derive(MetricGroup)]
#[metric(new())]
struct ReconcilerMetrics {
    /// reconciliations
    #[metric(rename = "doc_controller_reconciler")]
    reconciliations: Counter,
    /// reconciliation errors
    #[metric(rename = "doc_controller_reconciler_errors")]
    errors: CounterVec<ErrorLabelSet>,
    ...

whereas with a prefix we could avoid the rename attribute and lean on variable names (which are typically a lot shorter than metric names).

Proposed Solution 1

as a container attribute; #[metric(new(), prefix = "...")]

#[derive(MetricGroup)]
#[metric(new(), prefix = "doc_controller")]
struct ReconcilerMetrics {
    /// reconciliations
    reconciliations: Counter,
    /// reconciliation errors
    errors: CounterVec<ErrorLabelSet>,
    ...

Proposed Solution 2

A different implementation is present in prometheus client's with_prefix, but measured does not have a registry and instead bundles the stand-in encoder so maybe it could be provided on an encoder-type thing instead so that it's up to the user of libraries to decide what to call the metrics:

encoder.with_prefix("doc_controller" &metrics_struct);

... and then the consumer of ReconcilerMetrics (say you got it from a library) gets to choose the prefix of the library.

conradludgate commented 3 months ago

You can technically do this with the namespace attribute. It's possible I've not documented this well enough

clux commented 3 months ago

afaikt you need to have it on each metric in the struct as a mutually exclusive option to rename:

#[derive(MetricGroup)]
#[metric(new())]
pub struct ReconcilerMetrics {
    /// reconciliations
    #[metric(namespace = "doc_controller")]
    pub reconciliations: Counter,
    /// reconciliation errors
    #[metric(namespace = "doc_controller")]
    pub failures: CounterVec<ErrorLabelSet>,
    /// duration of reconcile to complete in seconds
    #[metric(namespace = "doc_controller")]
    #[metric(metadata = Thresholds::with_buckets([0.01, 0.1, 0.25, 0.5, 1., 5., 15., 60.]))]
    pub reconcile_duration: Histogram<8>,
}

which feels more like a per-metric prefix than a group prefix, unless i am using it wrong.

i might be, the struct does seem to lose a trait impl by doing this and i can't encode:

error[E0599]: the method `collect_group_into` exists for reference `&ReconcilerMetrics`, but its trait bounds were not satisfied
   --> src/main.rs:10:16
    |
10  |     reconciler.collect_group_into(&mut *encoder).unwrap();
    |                ^^^^^^^^^^^^^^^^^^ method cannot be called on `&ReconcilerMetrics` due to unsatisfied trait bounds
    |
   ::: /home/clux/kube/controller-rs/src/metrics.rs:18:1
    |
18  | pub struct ReconcilerMetrics {
    | ---------------------------- doesn't satisfy `_: MetricGroup<_>`
    |
   ::: /home/clux/.cargo/registry/src/index.crates.io-6f17d22bba15001f/measured-0.0.21/src/metric.rs:79:1
    |
79  | pub struct Metric<M: MetricType> {
    | -------------------------------- doesn't satisfy `_: MetricGroup<WithNamespace<&mut _>>`
...
118 | pub struct MetricVec<M: MetricType, L: LabelGroupSet> {
    | ----------------------------------------------------- doesn't satisfy `_: MetricGroup<WithNamespace<&mut _>>`
    |
    = note: the following trait bounds were not satisfied:
            `Metric<CounterState>: MetricGroup<WithNamespace<&'__enc_tmp_lt mut _>>`
            which is required by `controller::metrics::ReconcilerMetrics: MetricGroup<_>`
            `MetricVec<CounterState, controller::metrics::ErrorLabelSet>: MetricGroup<WithNamespace<&'__enc_tmp_lt mut _>>`
            which is required by `controller::metrics::ReconcilerMetrics: MetricGroup<_>`
            `Metric<HistogramState<8>>: MetricGroup<WithNamespace<&'__enc_tmp_lt mut _>>`
            which is required by `controller::metrics::ReconcilerMetrics: MetricGroup<_>`
            `controller::metrics::ReconcilerMetrics: MetricGroup<_>`
            which is required by `&controller::metrics::ReconcilerMetrics: MetricGroup<_>`
conradludgate commented 3 months ago

Yeah - what I do is apply it to a nested group

https://github.com/neondatabase/neon/blob/219e78f885486698a67da6ad62ef9f6d001b118a/proxy/src/http/health_server.rs#L62

Such that it applies the namespace to all metrics contained in the group.

I'm open to having a namespace attribute on the struct as well, although I am a bit hesitant. My thinking is that a library exposing metrics should not prescribe a namespace, and instead that should be up to the application. So the libraries exposed metric group should not add a namespace, but the application would build their root metrics group and then set the namespaces as appropriate.

clux commented 3 months ago

Ah, nice that works. Basically namespace works on one level deeper than what I am expecting then.

I'm open to having a namespace attribute on the struct as well, although I am a bit hesitant. My thinking is that a library exposing metrics should not prescribe a namespace, and instead that should be up to the application. So the libraries exposed metric group should not add a namespace, but the application would build their root metrics group and then set the namespaces as appropriate.

Yeah. Agreed. I think the system here works well when libraries all expose a single group so that apps can mount individual libraries metric groups where they want.

...there is a minor edge case in this design for libraries exposing multiple groups, because then the prefixes have to be replicated on the consumer side (with modifications if they want to pre-prefix). That said, this is probably not that helpful to solve because if you are using a library exposing multiple metric groups, you probably want to hand-select the groups you care about anyway.