Closed zohnannor closed 1 year ago
In case this is a concern, we can avoid breaking changes by adding a method with a default impl to LabelFilter
trait.
pub trait LabelFilter {
fn should_include_label(&self, name: &KeyName, label: &Label) -> bool;
fn should_include_label_by_name(&self, name: &KeyName, label: &Label) -> bool {
self.should_include_label(label)
}
}
@zohnannor
I also replaced
Vec<Label>
withSmallVec<[Label; 5]>
in theLabels
to improve performance (ran the benchmarks a couple of times, but buffer of 5 elements is not tested to be the best option).
I'd say these changes better go as a separate PR. Also, it seems that [Label; 3]
is more meaningful and much more common use-case.
@ilsiv Totally! I was thinking of writing it like that...
@tyranron Yes, it is unrelated, removed that from this PR.
Changed PR name & description to reflect new changes.
Exciting to see so much activity in a PR before I even open it. 😍
I'll try and take a look at this today.
Released as metrics-tracing-context@0.13.0
.
Thanks again for your contribution!
On the project I'm working on, we have to filter excessive metric labels added by
TracingContext
after the fact, but it could be better if we could see metrics' names in the process ofTracingContext::enhance_key
. And the inner workings ofMetricsLayer
doesn't allow us to create customLayer
+Recorder
to efficiently add only required metric labels.This PR adds
name: KeyName
argument toLabelFilter::should_include_label
method to pass fromTracingContext::enhance_key
, that way customLabelFilter
, that can filter labels by the name of the metric and label key/value, is an option. Bumpedmetrics-tracing-context
version to0.13
because it's a breaking change.