prometheus: revert the condition for enabling aggregation #2232
Conversation
in d7e16bb, we added the query time configuration for disabling aggregation, but the condition for disabling aggregation was wrong. before that change, we always enable the aggregation, but after that change, the aggregation is disabled when the value of query parameter of "__aggregate__" is not "false". in other words, - if this query parameter is not set, the aggregation is disabled. this changes the existing behavior. - if this query parameter's value is "false", the aggregation is enabeld. this is not the expected behavior. so, in this change - change the parameter from `disable_aggregation` to `enable_aggregation`. as the positive condition is always easier to understand. - and update the handling accodingly. Fixes d7e16bb Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
|
@scylladb/seastar-maint could you help review this change? it addresses a regression. and is required for scylladb/scylladb#18554 |
|
cc @bhalevy |
|
with this change, scylladb master is able to pass |
|
cc @nvartolomei |
These last two things don't look like "in other words" of the statement before them, but rather a failure to implement what the previous statement said.
You are overloading the word "parameter". The user-visible parameter remains with the name "aggregate", no change to that. You are just changing the name of some variable in the code, to make it easier for you to understand. I'll take a look at the actual change now :-) Also, you have in the commit message a "Fixes" line which refers to a commit hash, not to an issue. Please remove that line or fix it. |
I just now realized that the previous statement had a wrong "not" in the statement, so indeed the following statements are equivalent to that first wrong statement. This is a very confusing way to describe the bug. But I think I finally understood. |
| @@ -896,16 +896,16 @@ class metrics_handler : public httpd::handler_base { | |||
| sstring metric_family_name = req->get_query_param("__name__"); | |||
| bool prefix = trim_asterisk(metric_family_name); | |||
| bool show_help = req->get_query_param("__help__") != "false"; | |||
| bool disable_aggregation = req->get_query_param("__aggregate__") != "false"; | |||
| bool enable_aggregation = req->get_query_param("__aggregate__") != "false"; | |||
There was a problem hiding this comment.
Please note that you could have fixed this bug by a one-line change - changing != to ==, and that's it. The renaming (and flipping) the variable wasn't necessary. But I agree it looks better.
There was a problem hiding this comment.
i knew. but i was not after a single-liner fix.
|
🤦♂️ sorry for the bad PR. Appreciate the fix! |
in d7e16bb, we added the query time configuration for disabling aggregation, but the condition for disabling aggregation was wrong. before that change, we always enable the aggregation, but after that change, the aggregation is disabled when the value of query parameter of "aggregate" is not "false". in other words,
so, in this change
disable_aggregationtoenable_aggregation. as the positive condition is always easier to understand.Fixes d7e16bb
Signed-off-by: Kefu Chai kefu.chai@scylladb.com