Notebook(Alerts): have dataset schema, be correctly utilized in the alert thresholds.

wiedld commented 2 years ago

About the bug

Steps to reproduce: List the minimal actions needed to reproduce the behavior.

Use the query builder, or raw flux editor, to generate a query using fields.
Run notebook --> see that the output graph contains data.
Build an alert using the fields.
create a threshold using non-numeric data: trigger = (r) => r["co"] > 20 and (r["water_level"] > 0 and r["water_level"] < 100) and r["types"] == "earthquake"
Run task get an error --> because used a string field (not a numeric measurement).

.

Expected behavior: I should not be able to create threshold alerts, based on my dataset, which fail when I run the alert task.

failures due to non-numeric fields (as the example above)
failures of "never triggered", because user choose a field which exists on another measurement

.

Proposed solutions:

Either we can: A. decide to only allow numeric fields to be used in the notebook alerts B. permit string and numeric fields in threshold checks

Option A: only allow numeric fields

implementation details:

data elements:
- https://docs.influxdata.com/influxdb/cloud/reference/key-concepts/data-elements/
- a measurement is a numeric only data element.
- a field has many more types.
we currently read available fields for thresholds, from the returned dataset in the table view.
- The challenge here is that the "field" are actually just table columns --> and do not represent which are measurements or field.
- I believe we have this information already (see that the Error Thresholds have this awareness).

Option B: permit multiple field types?

implementation details:

we could start by scoping small (strings and numerics only)
- so make sure we don't have other field types to be aware of

we would need to stop using the monitor.check() method

this method is already problematic for us. see comment here: https://github.com/influxdata/ui/issues/3707#issuecomment-1064714862

then use flux like:

trigger = (r) => r["co"] > 20 and (r["water_level"] > 0 and r["water_level"] < 100) and r["types"] == "earthquake"

task_data =
  from(bucket: "sample_data")
      |> range(start: v.timeRangeStart, stop: v.timeRangeStop)
      |> filter(fn: (r) => r["_field"] == "co" or r["_field"] == "types" or r["_field"] == "water_level")
      |> filter(fn: trigger)
      // then send a notification...

wiedld commented 2 years ago

Assigned this to myself for now. Since this issue is intertwined with another issue I am currently working upon. So not sure yet if (or how much) of this will be resolved as I am in progress of fixing the other ticket.

wiedld commented 2 years ago

we decided to not pull out the monitor.check() yet, for reasons here: https://github.com/influxdata/ui/issues/4081#issuecomment-1074584517

But we are utilizing more schema awareness in the notebooks alerts. As in this PR: https://github.com/influxdata/ui/pull/4067

is not perfect. But is a patch until a broader product decision (updates) are made.

wiedld commented 2 years ago

Could not reach team consensus on resolution. Then we decided to not do more fixes to notebooks.

influxdata / ui