client_rust icon indicating copy to clipboard operation
client_rust copied to clipboard

feat(metrics): Enhance Family's flexibility

Open ethercflow opened this issue 1 year ago • 3 comments

This PR introduces two new interfaces to Family: get and with_metrics. These additions cater to the following use cases:

  1. In crate A (e.g., an embedded database), the get_or_create interface is used to create metric M1. In crate B, which depends on crate A, there's a need to check if metric A exists (with dynamic labels that may not have been created yet, eg., no set_kv happened yet). If it exists, additional business-specific labels from crate B can be added to this metric. For example:
for (xxx, family) in families { // Family is defined in the foreign crate A
    for method in Method::iter() { // Method is also defined in the foreign  crate A
        if let Some(metrics) = family.get(&Labels { method }) { // Only when the event happend in crate A, we add more labels to it in crate B
            let labels = [("XXX", xxx.to_string()), ("method", method.to_string())];
            let e = metric_encoder.encode_family(&labels)?;
            metrics.encode(e)?;
        }
    }
}
  1. While Family's definition allows for custom constructors, there are instances where constructor parameters are required. The standard get_or_create implementation and MetricConstructor falls short in such scenarios. The new with_metrics interface enables users to implement a CustomFamily that wraps Family as an inner field. By implementing Deref and DerefMut, CustomFamily can reuse most of Family's interfaces while defining its own constructor trait and get_or_create implementation. This significantly enhances Family's flexibility. For example:
pub trait CustomMetricCreator<S: FamilyLabels, M: FamilyMetric>: Send + Sync + 'static {
    fn create(&self, labels: S) -> M;
}

#[derive(Clone)]
pub struct CustomFamily<S: FamilyLabels, M: FamilyMetric, C: FamilyMetricCreator<S, M>> {
    inner: Family<S, M>,
    custom_metric_creator: C,
}


impl<S: FamilyLabels, M: FamilyMetric, C: FamilyMetricCreator<S, M>> CustomFamily<S, M, C> {
    pub fn create(creator: C) -> Self {
        Self {
            inner: Family::new_with_constructor(|| unreachable!()),
            custom_metric_creator: creator,
        }
    }

 pub fn get_or_create(&self, label_set: &S) -> Arc<M> {
        if let Some(metric) = self.inner.get(label_set) {
            return metric.clone();
        }

        self.inner.with_metrics(|metrics| {
            let mut write_guard = metrics.write();
            write_guard
                .entry(label_set.clone())
                .or_insert_with(|| Arc::new(self.custome_metric_creator.create(label_set.clone())))
                .clone()
        })
    }
}

ethercflow avatar Sep 26 '24 03:09 ethercflow

@mxinden PTAL, thanks!

ethercflow avatar Sep 26 '24 03:09 ethercflow

For the sake of getting Family::get merged, I suggest splitting this pull request by the two new functions.

Note that I am not sure with_metrics is a good idea.

  • It adds complexity to the crate that we will have to maintain long in the future.
  • It does not enable a new use-case, as power-users can already provide their own fork of a Family metric type.

mxinden avatar Oct 27 '24 19:10 mxinden

For the sake of getting Family::get merged, I suggest splitting this pull request by the two new functions.

Note that I am not sure with_metrics is a good idea.

  • It adds complexity to the crate that we will have to maintain long in the future.
  • It does not enable a new use-case, as power-users can already provide their own fork of a Family metric type.

Get it, will split this pr and submit Family::get first soon. Thanks!

ethercflow avatar Oct 28 '24 08:10 ethercflow