-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adding Metrics family config #2121
Changes from all commits
d2929c2
352c8ce
9c148f3
9a80dce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,13 @@ public: | |
_regex = std::regex(_regex_str); | ||
return *this; | ||
} | ||
bool empty() const noexcept { | ||
return _regex_str.empty(); | ||
} | ||
|
||
bool match(const std::string& str) const noexcept { | ||
return !empty() && std::regex_match(str, _regex); | ||
} | ||
}; | ||
|
||
/*! | ||
|
@@ -103,6 +110,25 @@ struct relabel_config { | |
*/ | ||
relabel_config::relabel_action relabel_config_action(const std::string& action); | ||
|
||
/*! | ||
* \brief metric_family_config allow changing metrics family configuration | ||
* | ||
* Allow changing metrics family configuration | ||
* Supports changing the aggregation labels. | ||
* The metrics family can be identified by a name or by regex; name-matching is | ||
* more efficient and should be used for a single metrics family. | ||
* | ||
* name - optional exact metric name | ||
* regex_name - if set, all the metrics name that match the regular expression | ||
* aggregate_labels - The labels to aggregate the metrics by. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't understand (see my comment on the commit message too) what aggregate_labels="x","y" means: Please explain this more clearly, because I don't have any intuitive grasp of what the concepts of "metric families" or "label aggregation" means. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading your test in the last patch, I think I understand better: It turns out labels have a name and a value - it's not that we have a metric with a label x - we can have one metric with the label "x=dog" and another one with "x=cat". So apparently aggregation will sum these two metrics and create one metric without (?) the label x at all. I still don't understand, though, what happens with other labels: What if we have the following four metrics:
How do we aggregate it over "x"? Do we sum all of those metrics and drop all labels? Or will we have three metrics for the three different "z" label values (i.e., just the first two metrics are summed)? This makes sense, but is it documented somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First, just for context, we already support aggregate by label, but currently, you have to configure it when you register the metrics, and it cannot be changed. Aggregation works by dropping one or more labels and summing over all metrics that now have the same name and label. Using your example, let's drop
Now we sum and get:
This is not new, just it's run-time configuration There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I must have been sleeping under a rock when you added it. That being said, all of this should be documented somewhere. I also found the names of the fields and their explanations to be confusing. It says "aggregate_labels - The labels to aggregate the metrics by.". In the above example, we are not aggregating "by x" - in fact in some sense it's the opposite - we're dropping x, and aggregating the metrics by the other labels. |
||
* | ||
*/ | ||
struct metric_family_config { | ||
std::string name; | ||
relabel_config_regex regex_name = ""; | ||
std::vector<std::string> aggregate_labels; | ||
}; | ||
|
||
SEASTAR_MODULE_EXPORT_END | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,7 +130,20 @@ future<metric_relabeling_result> set_relabel_configs(const std::vector<relabel_c | |
const std::vector<relabel_config>& get_relabel_configs() { | ||
return impl::get_local_impl()->get_relabel_configs(); | ||
} | ||
void impl::impl::update_aggregate(metric_family_info& mf) const noexcept { | ||
for (const auto& fc : _metric_family_configs) { | ||
if (fc.name == mf.name || fc.regex_name.match(mf.name)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you need to check regex_name.empty() here? Or match() is correct (and efficient enough) in the empty case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed that it was efficient enough and the update_aggregate is a rare operation |
||
mf.aggregate_labels = fc.aggregate_labels; | ||
} | ||
} | ||
} | ||
void set_metric_family_configs(const std::vector<metric_family_config>& family_config) { | ||
impl::get_local_impl()->set_metric_family_configs(family_config); | ||
} | ||
|
||
const std::vector<metric_family_config>& get_metric_family_configs() { | ||
return impl::get_local_impl()->get_metric_family_configs(); | ||
} | ||
|
||
static bool apply_relabeling(const relabel_config& rc, impl::metric_info& info) { | ||
std::stringstream s; | ||
|
@@ -454,6 +467,7 @@ void impl::add_registration(const metric_id& id, const metric_type& type, metric | |
_value_map[name].info().inherit_type = type.type_name; | ||
_value_map[name].info().name = id.full_name(); | ||
_value_map[name].info().aggregate_labels = aggregate_labels; | ||
impl::update_aggregate(_value_map[name].info()); | ||
_value_map[name][rm->info().id.labels()] = rm; | ||
} | ||
dirty(); | ||
|
@@ -503,6 +517,26 @@ future<metric_relabeling_result> impl::set_relabel_configs(const std::vector<rel | |
} | ||
return make_ready_future<metric_relabeling_result>(conflicts); | ||
} | ||
|
||
void impl::set_metric_family_configs(const std::vector<metric_family_config>& family_config) { | ||
_metric_family_configs = family_config; | ||
bool has_regex = false; | ||
for (const auto& fc : family_config) { | ||
has_regex |= !fc.regex_name.empty(); | ||
if (fc.name != "" && _value_map.find(fc.name) != _value_map.end()) { | ||
_value_map[fc.name].info().aggregate_labels = fc.aggregate_labels; | ||
} | ||
} | ||
if (has_regex) { | ||
for (auto& [name, family] : _value_map) { | ||
for (const auto& fc : family_config) { | ||
if (fc.regex_name.match(name)) { | ||
family.info().aggregate_labels = fc.aggregate_labels; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
const bool metric_disabled = false; | ||
|
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 understand what this sentence is saying - "changing the aggregation labels"? Are you changing labels? Or the aggregation of values with different labels?