Skip to content
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

Number-based alerts are broken #4279

Closed
susodapop opened this issue Oct 22, 2019 · 8 comments · Fixed by #4295
Closed

Number-based alerts are broken #4279

susodapop opened this issue Oct 22, 2019 · 8 comments · Fixed by #4295

Comments

@susodapop
Copy link
Contributor

Issue Summary

It looks like the alert always sees a query result as a string and not a number. So an alert configured to trigger when field = 0 never triggers because "0" does not equal 0.

The alerts seems to evaluate fine for targets that are obviously strings. If the alert should trigger if field = boyhowdy then the alert triggers when field is "boyhowdy".

Steps to Reproduce

  1. Save a query that returns 0: SELECT 0 "value"
  2. Make an alert for the query that triggers when value = 0
  3. Run the query
  4. Observe the alert does not trigger.

The converse is also true, by the way.

  1. Same as above
  2. Make an alert for the query that triggers when value != 0
  3. Run the query
  4. Observe the alert triggers

Technical details:

  • Redash Version: 9.0.0-alpha+b348b58b (348b58b)
  • Browser/OS: Firefox on MacOS
  • How did you install Redash: SaaS
@ranbena
Copy link
Contributor

ranbena commented Oct 22, 2019

I believe this is happens cause the threshold input field has been changed to accept texts (to accommodate text comparisons) so all new alerts yield a string from the threshold value.

@rauchy @kravets-levko wdyt of handling this in the backend - casting both args to string for ==/!= and to int for the rest?

operators = {
'>': lambda v, t: v > t,
'>=': lambda v, t: v >= t,
'<': lambda v, t: v < t,
'<=': lambda v, t: v <= t,
'==': lambda v, t: v == t,
'!=': lambda v, t: v != t,
# backward compatibility
'greater than': lambda v, t: v > t,
'less than': lambda v, t: v < t,
'equals': lambda v, t: v == t,
}
should_trigger = operators.get(self.options['op'], lambda v, t: False)
value = data['rows'][0][self.options['column']]
threshold = self.options['value']
if should_trigger(value, threshold):
new_state = self.TRIGGERED_STATE
else:
new_state = self.OK_STATE

@kravets-levko
Copy link
Collaborator

kravets-levko commented Oct 22, 2019

I think the solution could be something like: if alert's threshold value contains numeric string and query result's column type is number - cast both to number; otherwise cast both to string:

Query result column type Alert threshold value Cast both to
number string containing number number
number non-numeric string string
other types string containing number string
other types non-numeric string string

@arikfr
Copy link
Member

arikfr commented Oct 22, 2019

I think that if query result is a number, then we should try to cast both to number. In other cases: use a string.

Because the operation options we show are based on the query result type, the threshold should adhere. We might want to enforce this in the UI.

@ranbena
Copy link
Contributor

ranbena commented Oct 22, 2019

Not sure I understand why casting-by-value would be preferable to casting-by-operation.

@arikfr
Copy link
Member

arikfr commented Oct 22, 2019

Not sure I understand why casting-by-value would be preferable to casting-by-operation.

Because strings and numbers have different behavior, for example:

In case of a number 0.0 == 0, but in case of strings '0.0' != '0'.

@ranbena
Copy link
Contributor

ranbena commented Oct 23, 2019

Aha. So I propose the following:

Operator Cast
== / != Threshold to result value type
Other Both to number

@ranbena
Copy link
Contributor

ranbena commented Oct 23, 2019

Not sure about this, but perhaps we can hint at string values with apostrophes.

Screen Shot 2019-10-23 at 10 35 10

@arikfr
Copy link
Member

arikfr commented Oct 25, 2019

@susodapop reopening as the fix wasn't pushed to this repo yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants