-
Notifications
You must be signed in to change notification settings - Fork 525
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
Make __meta_tenant_id available in metric_relabel_configs #4725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think approach by passing builder to relabel.ProcessBuilder
is smart, and will not be as expensive as @bboreham mentioned in #4692 (comment).
pkg/distributor/distributor.go
Outdated
@@ -818,12 +819,19 @@ func (d *Distributor) prePushRelabelMiddleware(next push.Func) push.Func { | |||
ts := req.Timeseries[tsIdx] | |||
|
|||
if mrc := d.limits.MetricRelabelConfigs(userID); len(mrc) > 0 { | |||
l, keep := relabel.Process(mimirpb.FromLabelAdaptersToLabels(ts.Labels), mrc...) | |||
lb := labels.NewBuilder(mimirpb.FromLabelAdaptersToLabels(ts.Labels)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could reuse the builder between timeseries, to avoid creating new one in each iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if I understand you right)
The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Looks like a nice addition to me.
About the performance impact, I'm not too much worried because it should just apply to tenants for which the relabelling config has been set. However, you could add a test case with relabelling to BenchmarkDistributor_Push
(without the tenant ID label), then run it both on main
and this PR branch with go test -count=3 ...
and the compare the benchmarks with benchstat
, please? I would like to see the actual different from a benchmark.
pkg/distributor/distributor.go
Outdated
if !keep { | ||
removeTsIndexes = append(removeTsIndexes, tsIdx) | ||
continue | ||
} | ||
ts.Labels = mimirpb.FromLabelsToLabelAdapters(l) | ||
lb.Del(metaLabelTenantID) | ||
ts.Labels = mimirpb.FromLabelsToLabelAdapters(lb.Labels(labels.EmptyLabels())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should make it slightly more performant. Since we want to overwrite ts.Labels
anyway then we can pass it:
ts.Labels = mimirpb.FromLabelsToLabelAdapters(lb.Labels(labels.EmptyLabels())) | |
ts.Labels = mimirpb.FromLabelsToLabelAdapters(lb.Labels(ts.Labels)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But ts.Labels
is []LabelAdapter
and lb.Labels()
wants Labels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about this too much. New stringlabels
version of labels code that we will use soon doesn't even take this parameter anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about this too much. New
stringlabels
version of labels code that we will use soon doesn't even take this parameter anymore.
Sorry, it's not "new stringlabels
version" that doesn't have this method with labels.Labels
parameter anymore, but also latest Prometheus main (after merging prometheus/prometheus#12173) which will be in Mimir after #4759 gets merged.
f206381
to
80dd2d3
Compare
Ok tested on
Should I leave the bench test code here? |
Simplified this, now results for allocs are much better:
|
This reverts commit fb42dda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
Yes, feel free to include this benchmark in the PR. Note that there's a conflict in CHANGELOG that needs to resolved before we can merge this. |
After #4759 was merged, can you please rebase this PR on top of Mimir |
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
What this PR does
This PR adds meta label with
tenant_id
to distributormetric_relabel_configs
phaseWhich issue(s) this PR fixes or relates to
Fixes #4692
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]