-
Notifications
You must be signed in to change notification settings - Fork 236
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
Add a prometheus label mapping component #2025
base: main
Are you sure you want to change the base?
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.
Some initial doc review comments
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
Thanks for contributing.
Could you share the use case and performance differences? In many cases users can use If the reason to add this is that prometheus.relabel it's not efficient, we will need some more data on how inefficient it is and whether it could be optimised to work faster. Any new solution we would consider will require benchmarks to prove that it's more efficient. We also generally want to avoid having many ways of doing the same thing - but there can be exceptions. |
I have to map a label holding the service ID to a label holding the tenant ID. My test case is using X services ID mapped to Y tenants. My test case is mapping 100 different services ID to a single tenant. Using Using As my worst case can be X == Y, i haven't try to summarize regex so i have a rule block for each service ID |
b72677a
to
99c869c
Compare
Thanks for sharing some performance data. Have you tried to use The issue with So, for example, in a cluster of 1k targets and each exposing 1k metrics, you would have 1 million relabels with If we find that for some reason |
Thanks for your answer. I'm not using alloy to scrape targets but I'm receiving data thru remote_write from multiple data producers. I'm unable to see how we could optimize Here is a extract of my test configuration, using
The
|
@vaxvms Did the doc topic at |
Are you talking about I've updated the way the component work: The target_label isn't fixed anymore. each value of the source label can be mapped to severals labels |
d77e275
to
c5db3a5
Compare
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/prometheus/prometheus.mapping.md
Outdated
Show resolved
Hide resolved
Thanks for review @clayton-cornell |
Yeah that topic needs some attention too. I've got an open task to review and refactor the component docs. |
SourceLabel string `alloy:"source_label,attr"` | ||
|
||
// Mapping | ||
LabelValuesMapping map[string]map[string]string `alloy:"mapping,attr"` |
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.
Is a static map the best user experience for this? Should it support reading from a file - especially using the exports of a local.file
or remote.s3
for example? This allows for polled updating of the static map without needing to reload/restart/directly edit the Alloy config.
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.
I can dynamically read the mapping using alloy components. Using this method, it avoid having parsing logic in mapping component :
...
local.file "mapping" {
filename = file.path_join(module_path, "mapping.json")
}
prometheus.mapping "tenants_mapping" {
forward_to = [prometheus.remote_write.cluster.receiver]
source_label = "uid_label_key1"
mapping = encoding.from_json(local.file.mapping.content)
}
...
Did i miss something ?
return nil | ||
} | ||
|
||
func (c *Component) mapping(val float64, lbls labels.Labels) labels.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.
If we're not looking to see if the val is a staleNaN for caching, we probably don't need it to still be in the function call.
Is there a reason this isn't using the global labelstore like prometheus.relabel? Did you test performance with/without it?
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.
As i have used prometheus.relabel as a starting point, i forgot to remove val parameter. Fixed.
The performance improvement was so high, i didn't bothered looking for increased performance. Relabeling can be a heaving process involving regex whereas mapping is just a hash lookup.
Using the labelstore, it will have to
- compute a hash based on labels
- fetch the globalId in a map
- try to fetch new labels set from cache
I might be missing something but i can't see why 2 map lookup would be faster than a single lookup
// Relabel against a copy of the labels to prevent modifying the original | ||
// slice. | ||
lb := labels.NewBuilder(lbls.Copy()) | ||
sourceValue := lb.Get(c.sourceLabel) |
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 logic will attempt to map when the sourceLabel is not present. Rather than the mapping behave the same when sourceLabel is not present && sourceLabel has explicit value "", should there be an (optional) explicit defaultMapping for when sourceValue is not present?
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.
I don't have any strong opinion about this one. Having all mapping values at the same place make more sense to mebut i don't really bother.
@dehaansa @clayton-cornell @thampiotr Is here anything I have to fix/document/explain/change ? |
hey @vaxvms fyi most of the team is on holiday break. The review might be paused for the next few weeks |
So I have to wish them to have a nice holiday ! |
PR Description
This PR add a prometheus component to create a label based on a source_label value and a static mapping table.
Which issue(s) this PR fixes
For a large mapping table, using regex with
prometheus.relabel
is inefficientNotes to the Reviewer
PR Checklist