cmd/expr: fix the panic issue in dropbool function if two SeriesSets having different tagsets #2495
Conversation
…having different tagsets
The panic is "Error: interface conversion: interface {} is expr.Number,
not expr.Series".
muffix
left a comment
There was a problem hiding this comment.
Hi @harudark, thanks for the contribution. 👍
If I understand correctly, I believe your change would change the intended behaviour in Bosun. Bosun's operators are essentially working with sets, so they need to find the same groups on the left and right hand side of the operator (otherwise you'd see an unjoined group error). I think it should be the same for functions.
The docs describe dropbool() like this:
Drop datapoints where the corresponding value in the second series set is non-zero. (See Series Operations for what corresponding means).
"Corresponding values" are subject to group rules:
A corresponding datapoint is one which has the same timestamp (and normal group subset rules apply).
If we remember the group subset rules:
Various metrics can be combined by operators as long as one group is a subset of the other. A subset is when one of the groups contains all of the tag key-value pairs in the other. An empty group {} is a subset of all groups. {host=foo} is a subset of {host=foo,interface=eth0}, and neither {host=foo,interface=eth0} nor {host=foo,partition=/} are a subset of the other. Equal groups are considered subsets of each other.
So I think dropbool() should error when given series sets where the second isn't a subset of the first. It arguably shouldn't panic, but erroring is I think the correct response here.
What you can do if you really don't care about the tags in the second set (which might indicate a problem in the data being queried) is drop all the tags or not group the results in the first place. Taking your example from the test you added, the query dropbool(series("foo=bar", 0, 1, 1, 1, 2, 1, 3, 1), series("host=baz", 0, 1, 1, 2, 2, 3, 3, 2) != 2) should still error because the groups of the two series are incompatible.
However, dropbool(series("foo=bar", 0, 1, 1, 1, 2, 1, 3, 1), series("", 0, 1, 1, 2, 2, 3, 3, 2) != 2)correctly returns {"0": 1, "2": 1} in the current implementation.
Just to visualise what this query would do:
| ts | series1 |
series2 |
series2 != 2 |
|---|---|---|---|
| 0 | 1 | 1 | 1 |
| 1 | 1 | 2 | 0 |
| 2 | 1 | 3 | 1 |
| 3 | 1 | 2 | 0 |
So dropbool() drops the points from series1 where the value of series2 is != 2, if series2 is a subset of series1.
I think we shouldn't change the behaviour here. Does that make sense @johnewing1, @neilfordyce?
|
On second thought - I'm wondering if there's a mistake in the docs.
The behaviour currently seems to be that the points are kept (rather than dropped) where the corresponding value in the second series set is non-zero. 🤔 Edit: Raised #2496 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
In production, Bosun got panic with the stacktrace below (I removed the real alert name and expression):
unionis called insidedropbooland it's possibleunion.Aorunion.BhasNaN()value, which is aexpr.Number, notexpr.Series.union.Acannot do type assertion, we ignore this.union.Acan butunion.Bcannot,dropboolkeeps everything inunion.A.Type of change
How has this been tested?
See
TestDropBooladded in cmd/bosun/expr/funcs_test.goChecklist: