metrics: Allows filtering by dynamic labels #1461
Conversation
The metrics relabel API allows the creation of new labels during run time. It would be helpful to add those labels to the set of all existing labels so that Prometheus will be able to filter by those labels. This patch adds the labels to the label collection after performing metric relabeling. Signed-off-by: Amnon Heiman <[email protected]>
| @@ -447,6 +444,9 @@ void impl::add_registration(const metric_id& id, const metric_type& type, metric | |||
| _value_map[name].info().aggregate_labels = aggregate_labels; | |||
| _value_map[name][rm->info().id.labels()] = rm; | |||
| } | |||
| for (auto&& i : rm->info().id.labels()) { | |||
| _labels.insert(i.first); | |||
| } | |||
There was a problem hiding this comment.
Yikes. I don't follow this change and to be honest -- I don't even follow the code before this change :(
When registering new metrics, the if (_value_map.find(name) != _value_map.end()) { from above would evaluate to false so we get into the else branch and skip the _labels.insert() before the patch, but make insertions after it. What's going on here?
There was a problem hiding this comment.
@xemul Let me try to explain, the metrics layer holds a set of all available labels.
This way, if some queries for a specific label value (i.e. Prometheus API), it can check if this label exists.
Recently, a new API was added to the metrics layer that would allow dynamically add or change label values.
Following that change it's possible that someone register a metrics with specific labels, but because of relabel_config more labels were added.
The series address this situations, it always tries to update the set of labels.
Overall there's little impact on performance, metrics are registered once, and most of them during startup.
There was a problem hiding this comment.
The series address this situations, it always tries to update the set of labels.
That's clear, thanks. However, I still don't get why the
for (auto&& i : rm->info().id.labels()) {
_labels.insert(i.first);
}appeared in the impl::add_registration's "else" branch only now. Before this change, if a new metrics is added, its labels were not added to the _labels set?
Overall there's little impact on performance, metrics are registered once, and most of them during startup.
Yes, of course. Performance is not of interest here.
There was a problem hiding this comment.
Up until now, metrics were static, so it was enough to look for labels the first time a metric was added. However, maybe this assumption is wrong to begin with, and this is a bug (i.e., it shouldn't have been conditional in the first place).
I hit that limitation when I tested adding labels dynamically and managed to get to a situation where I could add a label, but I couldn't filter by it.
The metrics relabel API allows the creation of new labels during run time. It would be helpful to add those labels to the set of all existing labels so that Prometheus will be able to filter by those labels.
This patch adds the labels to the label collection after performing metric relabeling.
Signed-off-by: Amnon Heiman [email protected]
Fixes #2527