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

Generate more idiomatic Flux in query builder #13800

Merged
merged 1 commit into from
May 6, 2019
Merged

Conversation

chnn
Copy link
Contributor

@chnn chnn commented May 6, 2019

Closes #13775

Updates the Flux code generated by the query builder to use the aggregateWindow helper when possible. Queries previously generated like so:

from(bucket: "telegraf")
  |> range(start: v.timeRangeStart, stop: v.timeRangeStop)
  |> filter(fn: (r) => r._measurement == "cpu" or r._measurement == "disk")
  |> filter(fn: (r) => r._field == "used_percent")
  |> window(period: v.windowPeriod)
  |> mean()
  |> group(columns: ["_value", "_time", "_start", "_stop"], mode: "except")
  |> yield(name: "mean")

Will now be generated as:

from(bucket: "telegraf")
  |> range(start: v.timeRangeStart, stop: v.timeRangeStop)
  |> filter(fn: (r) => r._measurement == "cpu" or r._measurement == "disk")
  |> filter(fn: (r) => r._field == "used_percent")
  |> aggregateWindow(every: v.windowPeriod, fn: mean)
  |> yield(name: "mean")

@chnn chnn force-pushed the update-generated-flux branch 2 times, most recently from 10581ae to d6f371a Compare May 6, 2019 19:42
@chnn chnn marked this pull request as ready for review May 6, 2019 19:42
Copy link
Contributor

@ebb-tide ebb-tide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks awesome!
any idea how we could keep track of the number type issue nathaniel highlighted? It's certainly going to come up again.

ui/src/timeMachine/constants/queryBuilder.ts Outdated Show resolved Hide resolved
@chnn chnn force-pushed the update-generated-flux branch from 9e08ae6 to 5329cdc Compare May 6, 2019 21:27
@chnn
Copy link
Contributor Author

chnn commented May 6, 2019

any idea how we could keep track of the number type issue nathaniel highlighted? It's certainly going to come up again.

Yeah not sure... maybe it makes sense to wait until the issue is reported by a user, so we have a more acute description of the problem. Or we could create an issue—my worry is that the issue would be too broad of a problem statement and unactionable.

@chnn chnn merged commit e029a95 into master May 6, 2019
@chnn chnn deleted the update-generated-flux branch May 6, 2019 21:39
@nathanielc
Copy link
Contributor

any idea how we could keep track of the number type issue nathaniel highlighted? It's certainly going to come up again.

Eventually I expect this to be something that a Flux language server could expose. Not sure exactly the details of how that will all work, but this kind of thing is a type error which in many cases should be discoverable statically.

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

Successfully merging this pull request may close these issues.

Autogenerated Flux code does not use "aggregateWindow"
3 participants