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

Use parenthesis to surround the selected tags for influxdb queries #9131

Merged
merged 1 commit into from Aug 31, 2017

Conversation

Projects
None yet
3 participants
@jsternberg
Contributor

jsternberg commented Aug 30, 2017

The generated queries when selecting multiple tags are incorrect. In
InfluxQL, AND has a higher precedence than OR so the condition:

WHERE "hostname" = 'server1' OR "hostname" = 'server2' AND time > now() - 5m

This is parsed as if it were:

WHERE "hostname" = 'server1' OR ("hostname" = 'server2' AND time > now() - 5m)

But the intention is to write a query like this:

WHERE ("hostname" = 'server1' OR "hostname" = 'server2') AND time > now() - 5m

This change modifies the generated query so it surrounds a query with
multiple conditions in parenthesis so it doesn't conflict with the time
expression in an unexpected way.

This is currently not an issue because InfluxDB doesn't actually
evaluate the condition for the time expression correctly. It just looks
through the AST for anything that looks like a time expression and then
assumes the proper format of AND was used rather than validating that
it was used correctly.

Use parenthesis to surround the selected tags for influxdb queries
The generated queries when selecting multiple tags are incorrect. In
InfluxQL, `AND` has a higher precedence than `OR` so the condition:

    WHERE "hostname" = 'server1' OR "hostname" = 'server2' AND time > now() - 5m

This is parsed as if it were:

    WHERE "hostname" = 'server1' OR ("hostname" = 'server2' AND time > now() - 5m)

But the intention is to write a query like this:

    WHERE ("hostname" = 'server1' OR "hostname" = 'server2') AND time > now() - 5m

This change modifies the generated query so it surrounds a query with
multiple conditions in parenthesis so it doesn't conflict with the time
expression in an unexpected way.

This is currently not an issue because InfluxDB doesn't actually
evaluate the condition for the time expression correctly. It just looks
through the AST for anything that looks like a time expression and then
assumes the proper format of `AND` was used rather than validating that
it was used correctly.
@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Aug 30, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Aug 30, 2017

CLA assistant check
All committers have signed the CLA.

@jsternberg

This comment has been minimized.

Show comment
Hide comment
@jsternberg

jsternberg Aug 30, 2017

Contributor

The previously generated queries breaks with the current version of InfluxDB master as I'm writing this because of influxdata/influxdb#8712. We are considering it a regression so we'll be making sure that the previous improper behavior is restored so the types of queries generated by Grafana remain working (so this PR isn't technically needed for it to work), but I'm also going to try and have it send back a warning message that the query is improper.

Thanks. If you have any issues open for people seeing Grafana break with InfluxDB master, the issue linked above is the culprit.

Contributor

jsternberg commented Aug 30, 2017

The previously generated queries breaks with the current version of InfluxDB master as I'm writing this because of influxdata/influxdb#8712. We are considering it a regression so we'll be making sure that the previous improper behavior is restored so the types of queries generated by Grafana remain working (so this PR isn't technically needed for it to work), but I'm also going to try and have it send back a warning message that the query is improper.

Thanks. If you have any issues open for people seeing Grafana break with InfluxDB master, the issue linked above is the culprit.

@torkelo torkelo added this to the 4.5.0 milestone Aug 31, 2017

@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Aug 31, 2017

Member

thanks!

Member

torkelo commented Aug 31, 2017

thanks!

@torkelo torkelo merged commit bdfbc24 into grafana:master Aug 31, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

torkelo added a commit that referenced this pull request Aug 31, 2017

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