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

SQL: Fixes issues with showing value column name prefix in legends #35839

Merged
merged 7 commits into from
Jun 18, 2021

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Jun 16, 2021

Fixes #35390 (partially)

This is a SQL specific and back end only alternative to #35392

image

http://localhost:3000/d/JYola5qzz/datasource-tests-postgres?orgId=1&editPanel=4

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

This looks good to me

@torkelo torkelo requested a review from marefr June 16, 2021 19:37
ryantxu and others added 2 commits June 16, 2021 12:42
Co-authored-by: Torkel Ödegaard <torkel@grafana.org>
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Works in the case where you only have two fields, but not when you have more than one value fields with label metric. There's other cases when there's not a value and metric field as can be seen in documentation:

The gdev dashboard has one such example for postgres unittest dashboard:
image

SELECT
  $__timeGroup("time_date_time",'5m'),
  min("value_double") as "min_value",
  max("value_double") as "max_value"
FROM test_data
WHERE $__timeFilter("time_date_time")
GROUP BY time
ORDER BY time

This gdev dashboard shows that it resolves one panel, but not the other two:

image

For example below query generates the following frame

SELECT
  $__timeGroup("createdAt",'$summarize'),
  max(value) as "value",
  hostname as "metric"
FROM 
  grafana_metric
WHERE
  $__timeFilter("createdAt") AND
  measurement = 'cpu' AND
  hostname IN($host)
GROUP BY time, metric
ORDER BY time
{
  "response": {
    "results": {
      "A": {
        "frames": [
          {
            "schema": {
              "name": "results",
              "meta": {
                "executedQueryString": "SELECT\n  floor(extract(epoch from \"createdAt\")/60)*60 AS \"time\",\n  max(value) as \"value\",\n  hostname as \"metric\"\nFROM \n  grafana_metric\nWHERE\n  \"createdAt\" BETWEEN '2021-06-17T07:47:59.888Z' AND '2021-06-17T07:52:59.888Z' AND\n  measurement = 'cpu' AND\n  hostname IN('10.1.100.1','10.1.100.10','server1','server2')\nGROUP BY time, metric\nORDER BY time"
              },
              "fields": [
                {
                  "name": "time",
                  "type": "time",
                  "typeInfo": {
                    "frame": "time.Time"
                  }
                },
                {
                  "name": "value",
                  "type": "number",
                  "typeInfo": {
                    "frame": "float64",
                    "nullable": true
                  },
                  "labels": {
                    "metric": "server1"
                  }
                },
                {
                  "name": "value",
                  "type": "number",
                  "typeInfo": {
                    "frame": "float64",
                    "nullable": true
                  },
                  "labels": {
                    "metric": "server2"
                  }
                }
              ]
            },
            "data": {
              "values": [
                [
                  1623916080000,
                  1623916140000,
                  1623916200000,
                  1623916260000,
                  1623916320000
                ],
                [
                  101.94567116740645,
                  101.32983803008855,
                  98.24485134174023,
                  99.11082242003671,
                  98.20762076760217
                ],
                [
                  102.17495640876385,
                  101.49139611423894,
                  98.5697798472507,
                  99.34979998130595,
                  98.5910734483771
                ]
              ]
            }
          }
        ],
        "refId": "A"
      }
    }
  }
}

We need to decide whether wide data frames for SQL is the way forward. To me it is because of unified alerting and multi dimensions plus v8 is a good opportunity to do breaking change as long as we have a good workaround story. Let us know if you need help from backend platform figuring out this.

@hugohaggmark
Copy link
Contributor

This solves this issue for me #35599

@torkelo
Copy link
Member

torkelo commented Jun 17, 2021

@marefr

SELECT
$__timeGroup("time_date_time",'5m'),
min("value_double") as "min_value",
max("value_double") as "max_value"
FROM test_data
WHERE $__timeFilter("time_date_time")
GROUP BY time
ORDER BY time

This query did not work in v7.5 for me, what format have selected time series or table? The query is missing a metric field as well so the above code should not have any effect ?

We need to figure out something out really soon as v8 breaks almost all time series queries

@torkelo
Copy link
Member

torkelo commented Jun 17, 2021

@marefr but good catch that it does not solve all those graphs on that dashboard, a small update should fix that.

@torkelo
Copy link
Member

torkelo commented Jun 17, 2021

@marefr pushed an update that fixes it so that it works on all panels on that test dashboard, was a wrong condition that made the fix logic only apply to data frames with 2 fields

@marefr
Copy link
Member

marefr commented Jun 17, 2021

@torkelo

This query did not work in v7.5 for me, what format have selected time series or table? The query is missing a metric field as well so the above code should not have any effect ?

The old logic is, if there's no metric column selected, use the names of value columns as metric :)

@torkelo
Copy link
Member

torkelo commented Jun 17, 2021

@marefr it's working now, the query below works well for me

SELECT
$__timeGroup("time_date_time",'5m'),
min("value_double") as "min_value",
max("value_double") as "max_value"
FROM test_data
WHERE $__timeFilter("time_date_time")
GROUP BY time
ORDER BY time

@torkelo torkelo marked this pull request as ready for review June 17, 2021 14:05
@torkelo torkelo requested a review from a team as a code owner June 17, 2021 14:05
@torkelo torkelo requested review from wbrowne and removed request for a team June 17, 2021 14:05
@torkelo
Copy link
Member

torkelo commented Jun 17, 2021

pushed another update that refines the logic to only be applied when the orignal query returns 3 columns (so if you have more than one value field the fixing logic wont happen).
Updated a integration test as well

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

Works great 🎉

@torkelo torkelo changed the title SQL: use metric label as field name SQL: Fixes issues with showing value column name prefix in legends Jun 18, 2021
@torkelo torkelo merged commit 1d6e99b into main Jun 18, 2021
@torkelo torkelo deleted the sql-rename-strategy branch June 18, 2021 06:05
grafanabot pushed a commit that referenced this pull request Jun 18, 2021
…35839)

* rename strategy

* Update pkg/tsdb/sqleng/sql_engine.go

Co-authored-by: Torkel Ödegaard <torkel@grafana.org>

* more strict constraints

* Fixed so that it works on multi series results

* only apply the logic when original query returns 3 fields

* removed part of comment

* Update mysql test

Co-authored-by: Torkel Ödegaard <torkel@grafana.org>
Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
(cherry picked from commit 1d6e99b)
torkelo pushed a commit that referenced this pull request Jun 18, 2021
…35839) (#35922)

* rename strategy

* Update pkg/tsdb/sqleng/sql_engine.go

Co-authored-by: Torkel Ödegaard <torkel@grafana.org>

* more strict constraints

* Fixed so that it works on multi series results

* only apply the logic when original query returns 3 fields

* removed part of comment

* Update mysql test

Co-authored-by: Torkel Ödegaard <torkel@grafana.org>
Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
(cherry picked from commit 1d6e99b)

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>
ying-jeanne added a commit that referenced this pull request Aug 3, 2021
bryanuribe pushed a commit to open-o11y/grafana that referenced this pull request Aug 6, 2021
…rafana#35839)

* rename strategy

* Update pkg/tsdb/sqleng/sql_engine.go

Co-authored-by: Torkel Ödegaard <torkel@grafana.org>

* more strict constraints

* Fixed so that it works on multi series results

* only apply the logic when original query returns 3 fields

* removed part of comment

* Update mysql test

Co-authored-by: Torkel Ödegaard <torkel@grafana.org>
Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
ying-jeanne added a commit that referenced this pull request Sep 7, 2021
* convert SQLs to use sdk contracts

* make draft

* postgres

* intermedia

* get datasourceinfo filled at the beginning of the service

* move the interval into package because of cyclict  import and fix all postgres tests

* fix mysql test

* fix mssql

* fix the test for pr #35839

* fix some issue about intervalv2 package

* update sql test

* wire migration for SQLs

* add sqls to the background process

* make it register instead of register and start

* revert formatting

* fix tests

* fix linter

* remove integration test

* Postgres test fix

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grafana 8.0 incorrectly adding prefix label to grouped data
5 participants