feat: Make PrometheusClientLayer Clonable

[flaneur2020]

we met an issue in datafuselabs/databend#13373 , as sometimes we'd call build_operator() on some queries, this causes duplicated metrics get registered into the registry, which finally produced millions of metrics.

the solution is to make the PrometheusClientLayer a singleton, but currently the PrometheusClientLayer have to be moved. we need allow it passing a reference before making the singleton in the layer() call.

[Xuanwo]

I don't like the idea of implementing Layer for a reference. Do we have better choices? For example, changing the PromClientLayer's API to avoid such thing happens.

[Xuanwo]

Seems We need to make PrometheusClientMetrics accessible to users, allowing them to initialize and register it as needed. Also, PrometheusClientLayer should be modified to accept a PrometheusClientMetrics.

The API will be:

#[derive(Default, Clone)]
struct PrometheusClientMetrics {..}

impl PrometheusClientMetrics {
    pub fn register(&self, registry: &mut Registry);
}

struct PrometheusClientLayer {..}

impl PrometheusClientLayer {
    pub fn new(metrics: PrometheusClientMetrics) -> Self;
}

In this way, users only need to store PrometheusClientMetrics and calling register() while they needed. For example, at the time of application start.

[flaneur2020]

Seems We need to make PrometheusClientMetrics accessible to users, allowing them to initialize and register it as needed. Also, PrometheusClientLayer should be modified to accept a PrometheusClientMetrics.

The API will be:

#[derive(Default, Clone)]
struct PrometheusClientMetrics {..}

impl PrometheusClientMetrics {
    pub fn register(&self, registry: &mut Registry);
}

struct PrometheusClientLayer {..}

impl PrometheusClientLayer {
    pub fn new(metrics: PrometheusClientMetrics) -> Self;
}

In this way, users only need to store PrometheusClientMetrics and calling register() while they needed. For example, at the time of application start.

IMHO this api is also error prone as the PrometheusClientMetrics struct is passed by moving instead of a reference. We have no way to pass a PrometheusClientMetrics singleton from adhoc build_operator() in a query basis unless using interior mutability.

any other suggestion about the API design? or just use interior mutability?

[Xuanwo]

IMHO this api is also error prone as the PrometheusClientMetrics struct is passed by moving instead of a reference. We have no way to pass a PrometheusClientMetrics singleton from adhoc build_operator() in a query basis unless using interior mutability.

PrometheusClientMetrics implements Clone, so it's ok to clone it. Family has Arc internally.

[flaneur2020]

if do not want using reference on the Layer side, I think we need an Arc over a PrometheusClientMetrics.

however this may introduce an extra Arc dereference cost on every metric modification over a direct referencing.

#[derive(Debug, Clone)]
pub struct PrometheusClientLayer {
    metrics: Arc<PrometheusClientMetrics>,
}

impl PrometheusClientLayer {
    pub fn register(&self, registry: &mut Registry) -> Self;
}

  .layer(prometheus_client_layer.clone())

[flaneur2020]

IMHO this api is also error prone as the PrometheusClientMetrics struct is passed by moving instead of a reference. We have no way to pass a PrometheusClientMetrics singleton from adhoc build_operator() in a query basis unless using interior mutability.

PrometheusClientMetrics implements Clone, so it's ok to clone it. Family has Arc internally.

no. not all metrics are Family, the simple metric like Counter is a simply atomic u64, if you clone it, you ends up with multiple metrics. it has to be placed in one single memory location.

[Xuanwo]

not all metrics are Family, the simple metric like Counter is a simply atomic u64

But all metrics in opendal will be Family? All metrics in opendal will be connectted with a OperationLabels

[flaneur2020]

not all metrics are Family, the simple metric like Counter is a simply atomic u64

But all metrics in opendal will be Family?

no, Family is used for the metrics with labels. if your labels exploded, you'd like to replace some families with simple metrics to make the metrics crapper happy.

[Xuanwo]

no, Family is used for the metrics with labels. if your labels exploded, you'd like to replace some families with simple metrics to make the metrics crapper happy.

I didn't get this. Are you talking about changing PrometheusClientMetrics too?

[flaneur2020]

oh I got it wrong, the simple metrics like Counter, Gauge are also wrapped within an Arc, so PrometheusClientMetrics is safe to clone.

[flaneur2020]

so we can consider just make PrometheusClientLayer clone-able, so it allows expose less things outside.

[Xuanwo]

so we can consider just make PrometheusClientLayer clone-able, so it allows expose less things outside.

LGTM.

[flaneur2020]

updated the PR, after when only one line of code is changed, PTAL @Xuanwo

[Xuanwo]

Mostly LGTM, please make rustfmt happy.