Add auto group by functionality using :interval: template variable #1609

timraymond · 2017-06-14T15:08:22Z

CHANGELOG.md updated with a link to the PR (not the Issue)
Rebased/mergable
Tests pass
Sign CLA (if not already signed)

Connect #1471

The problem

It's far too easy to select more data than your monitor can actually display.

The Solution

This introduces a new template variable, :autoGroupBy:, which calculates a group by time based off of the available resolution for rendering a graph (per @goller 's math in #829). This is supplied by Dygraphs along with the query.

This introduces another "type" of template variable, a GroupByVar, which handles this calculation and is instantiated by a custom UnmarshalJSON, capable of determining Template Variable types. Also, Template Variables now have varying precedence, as GroupByVar must be applied after all template variables so that it can see the selected time range in the query.

Caveats / Known Grossness

This currently works only with data reporting at the Telegraf default of 10s. There is some follow-on work to determine the underlying reporting interval, and @goller and I have discussed some strategies for doing this.
Template Variables that analyze and modify queries, like :autoGroupBy: need to parse the query before they can do their job. Unfortunately, the InfluxQL parsing functions need a legal query to parse, which InfluxQL + Template Variables is not. We ran into this circular dependency with :dashboardTime: as well. To break this dependency, I instead manually parsed out the duration, which limits the types of where expressions that can be understood by :autoGroupBy:. Ideally, template variables and template macros (which :autoGroupBy: and :dashboardTime: really are) would be supported directly by InfluxQL.

This adds support for dynamic template variables that compute something about themselves given some additional context.

In order for automatic group by to be remotely useful, we need to parse out the selected duration of time from the query itself. The problem with doing this is that using the existing machinery for parsing InfluxQL requires having valid InfluxQL, which InfluxQL+Template Variables is not. To break this chicken-and-egg problem, the duration is directly extracted from the query using regular string processing.

In order for :autoGroupBy: and :dashboardTime: to co-exist in a query, it's necessary to introduce template variable precedence to the backend. This is done by adding a `Precedence()` method to the TemplateVariable interface that returns an ordinal indicating the precedence level of the template variable. Precedence starts from 0 (highest) proceeding to the maximum that a `uint` can represent. A template variable at a given precedence level can expect that all template variables with higher precedence will have already been replaced in the query that is passed to its `Exec` call. For example, :autoGroupBy: has lower precedence than :dashboardTime: because it needs to know the selected time range for the query. When the `Exec` method of `GroupByVar` is invoked, it will see the query after :dashboardTime: has already been replaced, allowing it to extract the duration successfully.

In order to properly parse time ranges including `days` and `weeks` like `time > now() - 180d`, we need to use the `influxql.ParseDuration` function

This adds a bounds check to make sure that we don't overrun the end of the string when searching for a where clause.

There's some follow-on work to be done here to determine an appropriate value for the reporting interval, but for now this lets the client supply it.

Where clauses generated by the query builder have "WHERE" capitalized, and supply template variables with the ":" bracing.

lukevmorris

@nhaugo wants :autoGroupBy: to be renamed :interval:. Other than that, only minor comments.

We'll wait to merge this until it's gone through a round of testing.

lukevmorris · 2017-06-14T20:22:22Z

ui/src/shared/components/AutoRefresh.js

+            if (resolution) {
+              return {...temp, resolution}
+            }
+            return {...temp, resolution: 1000}


Instead of if (resolution) {, let's set a default value on line 108:

const {resolution = 1000} = this.state

Though I just noticed DashboardPage.js L266 -- can't we assume that it will exist?

lukevmorris · 2017-06-14T20:26:09Z

chronograf.go

+	Range        *Range       `json:"range,omitempty"` // Range is the default Y-Axis range for the data
+}
+
+// TemplateVars are a hetergenous collection of different TemplateVariables


heterogenous?

lukevmorris · 2017-06-14T20:27:00Z

chronograf.go

+	TemplateVars TemplateVars `json:"tempVars,omitempty"` // TemplateVars are template variables to replace within an InfluxQL query
+	Wheres       []string     `json:"wheres,omitempty"`   // Wheres restricts the query to certain attributes
+	GroupBys     []string     `json:"groupbys,omitempty"` // GroupBys collate the query by these tags
+	Resolution   uint         `json:"resolution,omitempty"`


Should mention in a comment that this is basically maximum resolution for the chart

lukevmorris · 2017-06-14T21:00:35Z

chronograf.go

@@ -149,17 +174,80 @@ func (t TemplateVar) String() string {
 		return `'` + t.Values[0].Value + `'`
 	case "csv", "constant":
 		return t.Values[0].Value
+	case "autoGroupBy":
+		return "group by time(1555s)"


Do we need this anymore now that GroupByVar is its own type?

Nope, that was mocked out before I had GroupByVar. Good catch.

This adds an "auto" option to the group by dropdown which interpolates "GROUP BY :autoGroupBy:" to the query.

This was present for testing in development and is no longer needed.

This naming is more consistent with user's expectations from other similar visualization tools. Also, the usage of the variable now requires the words "GROUP BY" to be present. e.g. `GROUP BY :interval:`

This perpetuates the hacks that we added for :dashboardTime: so that they will also work for :interval:. We should really find a better way to do this.

This reverts commit 5a37e8f, reversing changes made to 8bb869a.

The Data Explorer doesn't work with the :interval: template variable, so it makes no sense to have an 'auto' option in the group by time dropdown.

This makes sure that a group by time is applied whenever a function is applied to a field within the query builder.

For the Data Explorer, the appropriate default group by interval is 10s, where in the CEO it should be :interval:, since template variables are supported there.

Improve spellingz and document what "resolution" actually means in the context of a query

timraymond · 2017-06-16T00:51:18Z

We've changed :autoGroupBy: to :interval: and reverted the reversion (in #1585) of the PR (in #1560) that ensures that functions are automatically applied to fields.

The Query Builder is now able to appropriately apply the correct default group by time to both the Data Explorer (which does not support :interval:) and the Dashboard Page.

goller

Would you also add tests for the changes to query.go's Convert function?

Also, would you update server/swagger.json?

goller · 2017-06-16T20:54:11Z

chronograf.go

 // TemplateValue is a value use to replace a template in an InfluxQL query
-type TemplateValue struct {
+type BasicTemplateValue struct {


@timraymond Is it possible to move these interface implementations into the influx package?

goller · 2017-06-16T20:55:58Z

chronograf.go

+}
+
+// Exec is responsible for extracting the Duration from the query
+func (g *GroupByVar) Exec(query string) {


Would you move the functions into the influx package? That way you can change them to functions.

goller · 2017-06-16T20:56:51Z

chronograf.go

+	TemplateVars TemplateVars `json:"tempVars,omitempty"`   // TemplateVars are template variables to replace within an InfluxQL query
+	Wheres       []string     `json:"wheres,omitempty"`     // Wheres restricts the query to certain attributes
+	GroupBys     []string     `json:"groupbys,omitempty"`   // GroupBys collate the query by these tags
+	Resolution   uint         `json:"resolution,omitempty"` // Resolution is the available screen resolution to render query results


Would you specify the units of the resolution in the comments? Probably width in pixels?

goller · 2017-06-16T20:59:48Z

chronograf.go

+// TemplateVars are a heterogeneous collection of different TemplateVariables
+// with the capability to decode arbitrary JSON into the appropriate template
+// variable type
+type TemplateVars []TemplateVariable


Would you place TemplateVars into the server package? Only the server packages "cares" about unmarshaling.

goller · 2017-06-16T21:01:38Z

chronograf.go

+// variable type
+type TemplateVars []TemplateVariable
+
+func (t *TemplateVars) UnmarshalJSON(text []byte) error {


Would you describe why there is a custom unmarshal function? Perhaps a comment. This must be some custom format, and, I'd like to understand the reasoning.

goller · 2017-06-16T21:02:44Z

influx/query.go

@@ -11,6 +11,12 @@ import (
 // Convert changes an InfluxQL query to a QueryConfig
 func Convert(influxQL string) (chronograf.QueryConfig, error) {
 	itsDashboardTime := false
+	intervalTime := false
+	if strings.Contains(influxQL, ":interval:") {
+		influxQL = strings.Replace(influxQL, ":interval:", "time(1234s)", 1)


Is this left over from a test?

goller · 2017-06-16T21:03:39Z

influx/query.go

@@ -25,6 +31,10 @@ func Convert(influxQL string) (chronograf.QueryConfig, error) {
 		influxQL = strings.Replace(influxQL, "now() - 15m", ":dashboardTime:", 1)
 	}

+	if intervalTime {
+		influxQL = strings.Replace(influxQL, "time(1234s)", ":interval:", 1)


Is this extra as well?

goller · 2017-06-16T21:04:20Z

influx/query.go

@@ -104,7 +114,11 @@ func Convert(influxQL string) (chronograf.QueryConfig, error) {
 			if !ok {
 				return raw, nil
 			}
-			qc.GroupBy.Time = lit.String()
+			if intervalTime {
+				qc.GroupBy.Time = "auto"


Why auto rather than ":interval:"?

goller · 2017-06-16T21:09:55Z

influx/templates.go

+	}
+
+	replaced := query
+	for prc := uint(0); prc <= maxPrecedence; prc++ {


This algorithm is pretty specific. Would you comment it? Mostly, I'm interested in you commenting why you are checking for an empty newVal, and why you are handling ExecutableVar differently.

goller · 2017-06-16T21:13:25Z

server/queries.go

-	TemplateVars []chronograf.TemplateVar `json:"tempVars,omitempty"`
+	ID           string                  `json:"id"`
+	Query        string                  `json:"query"`
+	TemplateVars chronograf.TemplateVars `json:"tempVars,omitempty"`


This could be your unmarshal-able TemplateVars type

goller

I'm worried about the parsing for autoGroupby mentioned in your comments. That limits the influxql parsing significantly.

What other options did you think about for this? Could requiring a default value work?

lukevmorris · 2017-06-16T23:29:27Z

I'm merging this so I can test on the HEAD of master. Does not invalidate Chris' latest review: @goller @timraymond

jaredscheib · 2017-06-17T01:18:15Z

Changelog updated in master retroactively.

timraymond added 11 commits June 13, 2017 14:59

Add support for :autoGroupBy: template variable

45402f4

This adds support for dynamic template variables that compute something about themselves given some additional context.

Fetch resolution from dygraphs for autoGroupBy

ca9f93c

correct TemplateVar collection type in Queries

c5872bb

Working dynamic json Unmarshaling

6a672cf

Add hacks to make AGB work with the frontend

4f37949

Swap time.ParseDuration for influxql.ParseDuration

5cd5963

In order to properly parse time ranges including `days` and `weeks` like `time > now() - 180d`, we need to use the `influxql.ParseDuration` function

Prevent panic when where clause not found

7c5b7ab

This adds a bounds check to make sure that we don't overrun the end of the string when searching for a where clause.

Move hardcoded reporting interval to client side

d3ac237

There's some follow-on work to be done here to determine an appropriate value for the reporting interval, but for now this lets the client supply it.

Adjust templates_tests to reflect reality

f8ebb13

Where clauses generated by the query builder have "WHERE" capitalized, and supply template variables with the ":" bracing.

timraymond added the ready for review label Jun 14, 2017

timraymond changed the title ~~Feature/tr auto group by~~ Introduce :autoGroupBy: template variable Jun 14, 2017

lukevmorris suggested changes Jun 14, 2017

View reviewed changes

timraymond and others added 5 commits June 15, 2017 17:41

Update Group By Dropdown with Auto Option

597f080

This adds an "auto" option to the group by dropdown which interpolates "GROUP BY :autoGroupBy:" to the query.

Remove stubbed-out auto group by option

6c23fd1

This was present for testing in development and is no longer needed.

Rename :autoGroupBy: to :interval:

95862a9

This naming is more consistent with user's expectations from other similar visualization tools. Also, the usage of the variable now requires the words "GROUP BY" to be present. e.g. `GROUP BY :interval:`

Add Horrible Hacks for Group By Queries endpoint

4e7ec0b

This perpetuates the hacks that we added for :dashboardTime: so that they will also work for :interval:. We should really find a better way to do this.

Revert "Merge pull request #1585 from influxdata/revert-auto-func"

6a91690

This reverts commit 5a37e8f, reversing changes made to 8bb869a.

timraymond force-pushed the feature/tr-auto-group-by branch from cbbdd86 to 6a91690 Compare June 15, 2017 21:44

jaredscheib and others added 3 commits June 15, 2017 19:17

Fix Auto Group By time with Data Explorer

7c63f5f

The Data Explorer doesn't work with the :interval: template variable, so it makes no sense to have an 'auto' option in the group by time dropdown.

Ensure group by time when toggling fields

c9d5dd9

This makes sure that a group by time is applied whenever a function is applied to a field within the query builder.

Use appropriate default group by for CEO and DE

44723cf

For the Data Explorer, the appropriate default group by interval is 10s, where in the CEO it should be :interval:, since template variables are supported there.

timraymond force-pushed the feature/tr-auto-group-by branch from 022a2da to 44723cf Compare June 16, 2017 00:39

timraymond added 2 commits June 15, 2017 20:42

Documentation++

193811e

Improve spellingz and document what "resolution" actually means in the context of a query

Merge branch 'master' into feature/tr-auto-group-by

a396bd1

timraymond changed the title ~~Introduce :autoGroupBy: template variable~~ Add auto group by functionality using :interval: template variable Jun 16, 2017

lukevmorris approved these changes Jun 16, 2017

View reviewed changes

goller suggested changes Jun 16, 2017

View reviewed changes

goller reviewed Jun 16, 2017

View reviewed changes

lukevmorris added 2 commits June 16, 2017 15:54

Tested + fixed bug in GROUP BY interval calculation

5aeb32b

Change resolution on graph resize

4263a0d

lukevmorris merged commit d8c465b into master Jun 16, 2017

lukevmorris deleted the feature/tr-auto-group-by branch June 16, 2017 23:29

lukevmorris removed the ready for review label Jun 16, 2017

goller mentioned this pull request Jul 17, 2017

Chronograf process suspended by Chrome sometimes #1538

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auto group by functionality using :interval: template variable #1609

Add auto group by functionality using :interval: template variable #1609

timraymond commented Jun 14, 2017 •

edited

Loading

lukevmorris left a comment

lukevmorris Jun 14, 2017

lukevmorris Jun 14, 2017

lukevmorris Jun 14, 2017

lukevmorris Jun 14, 2017

lukevmorris Jun 14, 2017

timraymond Jun 15, 2017

timraymond commented Jun 16, 2017 •

edited by jaredscheib

Loading

goller left a comment

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller Jun 16, 2017

goller left a comment

lukevmorris commented Jun 16, 2017

jaredscheib commented Jun 17, 2017

Add auto group by functionality using :interval: template variable #1609

Add auto group by functionality using :interval: template variable #1609

Conversation

timraymond commented Jun 14, 2017 • edited Loading

The problem

The Solution

Caveats / Known Grossness

lukevmorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timraymond commented Jun 16, 2017 • edited by jaredscheib Loading

goller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goller left a comment

Choose a reason for hiding this comment

lukevmorris commented Jun 16, 2017

jaredscheib commented Jun 17, 2017

timraymond commented Jun 14, 2017 •

edited

Loading

timraymond commented Jun 16, 2017 •

edited by jaredscheib

Loading