Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(metrics): Enhance Family's flexibility #230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ethercflow
Copy link

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()
        })
    }
}

Signed-off-by: Wenbo Zhang <wenbo.zhang@iomesh.com>
@ethercflow
Copy link
Author

@mxinden PTAL, thanks!

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough patch.

I am fine with get. I don't think we should expose the internals of Family, e.g. via with_metrics.

/// ```
pub fn with_metrics<F, R>(&self, f: F) -> R
where
F: FnOnce(&RwLock<HashMap<S, M>>) -> R
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should expose the internals of Family on the public API.

Say we want to change self.metrics to a different datastructure. That would be a breaking change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should expose the internals of Family on the public API.

Say we want to change self.metrics to a different datastructure. That would be a breaking change.

Agree with your point of view, do you think improvements like the following are acceptable?

pub fn with_metrics<F, R>(&self, f: F) -> R
where
    F: FnOnce(&dyn MetricsView<S, M>) -> R
{
    struct MetricsViewImpl<'a, S, M>(&'a RwLock<HashMap<S, M>>);

    impl<'a, S, M> MetricsView<S, M> for MetricsViewImpl<'a, S, M> {
        fn get(&self, key: &S) -> Option<&M> {
            self.0.read().get(key)
        }
       
        // other methods
    }

    f(&MetricsViewImpl(&self.metrics))
}

pub trait MetricsView<S, M> {
    fn get(&self, key: &S) -> Option<&M>;
    // other methods
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants