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

Add support to render values of multiple queries in the same table #10050

Merged
merged 8 commits into from Dec 14, 2017

Conversation

@davkal
Copy link
Contributor

davkal commented Dec 1, 2017

The current table transform renders only the first query.
This PR adds a new transform to render all query results in a JOIN-ish
semantic.

  • new table transform: Multi-Query table
  • columns is the union of all non-value fields
  • one value column per query is added
  • rows that share all the same label values are merged into one

How this PR relates to existing issues and PRs:

  • Fixes #9170
  • Extends the solution of #9340 by returning the union of all columns and adding merging.

Screenshots:

2 queries render 2 value columns if the other fields match:
screen shot 2017-12-01 at 14 39 47

2 queries when fields dont match:
screen shot 2017-12-01 at 14 43 14

Relabeling of value fields:
screen shot 2017-12-01 at 14 45 13

The current table transform renders only the first query.
This PR adds a new transform to render all query results in a JOIN-ish
semantic.

* new table transform: Multi-Query table
* columns is the union of all non-value fields
* one value column per query is added
* rows that share all the same label values are merged into one
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Dec 1, 2017

CLA assistant check
All committers have signed the CLA.

@bergquist

This comment has been minimized.

Copy link
Contributor

bergquist commented Dec 4, 2017

Neat!

Not 100% sure about adding a new transform. I think this should be the expected behavior when having multiple queries. Any specific reason for introducing a new transform? Perhaps I've missed a discussion somewhere else.

@davkal

This comment has been minimized.

Copy link
Contributor Author

davkal commented Dec 4, 2017

Any specific reason for introducing a new transform?

@bergquist no other than making it less controversial. 😄
IMO this should be the default behavior for table transforms in general. I'm happy to work this into the standard table transform.

@bergquist

This comment has been minimized.

Copy link
Contributor

bergquist commented Dec 4, 2017

@davkal I cannot come up with a good reason for not making it the standard table transform.

If anyone else wants to test this. Make sure uncomment this line https://github.com/grafana/grafana/blob/master/public/app/plugins/panel/table/module.ts#L105 for now. Otherwise, you will go postal :)

When referring to other queries in Grafana we usually prefix the query letter with # I think that makes sense in this case as well. Would be great if the letter was the real query refId as well.

I wonder if it would make sense to sort the order of the rows based on timestamp, refId.
Right now the order seems to be based timestamp then query the returns first

@davkal

This comment has been minimized.

Copy link
Contributor Author

davkal commented Dec 4, 2017

I combined the logic of both into the table transform and added a non-table type test.
Also, the value columns are now named Value #A, ...
I could not find any refId logic, nor a multi-sort support in the TableModel. I'd leave that for a future PR.

@bergquist

This comment has been minimized.

Copy link
Contributor

bergquist commented Dec 5, 2017

Fair enough :)

var columns = transformers[transform].getColumns(singleQueryData);
expect(columns[0].text).toBe('Time');
expect(columns[1].text).toBe('Label Key 1');
expect(columns[2].text).toBe('Value #A');

This comment has been minimized.

Copy link
@bergquist

bergquist Dec 5, 2017

Contributor

If there is only one result. Value is enough.

This would a breaking change for those of rename Value

* treat single-query table panels like they were before
* adjusted test cases
@davkal

This comment has been minimized.

Copy link
Contributor Author

davkal commented Dec 5, 2017

@bergquist I'm now treating the single query like it was done previously to stay 100% backwards-compatible. PTAL

}, []);

// Append one value column per data set
data.forEach((_, i) => {

This comment has been minimized.

Copy link
@bergquist

bergquist Dec 5, 2017

Contributor

Sorry for the scattered feedback... too many things going on today.

This assumes that the table data have a value field. Which is true for Prometheus but not for InfluxDb / Elastic

This comment has been minimized.

Copy link
@davkal

davkal Dec 5, 2017

Author Contributor

No worries. It's a complex software.
Let's revisit the essence of the PR: producing a union of columns and merging rows that have the same non-metric field values.
I see a couple of ways forward:

a) probe the first rows of each series for which types are metric fields and adjust the merge semantic accordingly (I do realize that relying on 'Value' labels was brittle to begin with)
b) keep ignoring the additional queries for data sources other than prometheus.
c) accept the undefined behavior for non-prometheus sources and merge as is

@bergquist or do you see other options? Let me know what you prefer...

This comment has been minimized.

Copy link
@bergquist

bergquist Dec 6, 2017

Contributor

B) Feels unconsistent. Really want to avoid such things when we have so many data sources.
C) This would be fine if the code change would be contained in the Prometheus plugin. But since it's in a panel I don't like the idea.
A) I wonder how good this would workout. How would be decide if the should merge or create a new column? strings vs numbers would not be good enough for elastic/influxdb.

Another approach would be to ask the Prometheus data source to give columns a unique name when returning multiple datasets as a table. ex: bergquist@962709a

The code for converting data into table/timeseries in the Prometheus data source is a bit messy.

This comment has been minimized.

Copy link
@davkal

davkal Dec 6, 2017

Author Contributor

I like the renaming of the value column in the prom datasource, that simplifies the column union.
For the merge/join logic, if we cant do probing based on data types, we could simply merge on missing indexes in the shifted rows, e.g., row 1 can merge with a row that has missing values on index(V2) while other values must match unless they are undefined:

   C1 | V1 | V2 | TS
1. foo| 123| -  | 1
2. foo|  - | 454| 1
3. bar|  - | 890| 1

I'll have a think.

davkal added 2 commits Dec 6, 2017
* after a match has been found the merger should keep looking for more
* moved unique value naming to datasource (credit: @bergquist)
* merge rows based on same column-values and empty values
* expanded tests
@grafana grafana deleted a comment from codecov-io Dec 11, 2017

// Merge rows that have same values for columns
const mergedRows = {};
rows = rows.reduce((acc, row, rowIndex) => {

This comment has been minimized.

Copy link
@bergquist

bergquist Dec 11, 2017

Contributor

I would love if this could be split up a bit more. It feels like I'm staring down the abyss :)

Looks good otherwise! This will be awesome

This comment has been minimized.

Copy link
@davkal

davkal Dec 11, 2017

Author Contributor

Cheers! Will split this up a bit more and make it less nested.

@davkal

This comment has been minimized.

Copy link
Contributor Author

davkal commented Dec 12, 2017

@bergquist hope that makes it easier to read.

@bergquist

This comment has been minimized.

Copy link
Contributor

bergquist commented Dec 14, 2017

Great!

It's still fairly complex but I don't have a better solution. Merge time!

@bergquist bergquist merged commit f5d26bf into grafana:master Dec 14, 2017
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
@paddynewman

This comment has been minimized.

Copy link

paddynewman commented Feb 15, 2018

Hello @bergquist,

This is a great feature! Are there any plans to release another 4.6 version that will include it? TIA.

@bergquist

This comment has been minimized.

Copy link
Contributor

bergquist commented Feb 15, 2018

@paddynewman No. But 5.0 stable is just a few weeks away.

@ahrtr

This comment has been minimized.

Copy link

ahrtr commented Mar 15, 2018

It seems that this fix isn't included in 5.x releases, not even 5.0.2, which was released yesterday.
What did I miss?

@davkal

This comment has been minimized.

Copy link
Contributor Author

davkal commented Mar 16, 2018

@ahrtr it's included from 5.0.0 onwards, here is an example:
http://play.grafana.org/d/000000065/prometheus-console-tables
Check out the configuration of the second table: two instant queries with the same label breakdown. This makes merging the results possible.

@yellowpattern

This comment has been minimized.

Copy link

yellowpattern commented May 14, 2018

What's the magic to get the merge tables part up?
With influxdb as my data source, this is what I see...

Even if I add "GROUP BY tag(instance),tag(host)", it does not join the two queries.

screen shot 2018-05-14 at 9 24 38 pm

@yellowpattern

This comment has been minimized.

Copy link

yellowpattern commented May 14, 2018

Is this feature currently prometheus-only?

@daniellee

This comment has been minimized.

Copy link
Member

daniellee commented May 14, 2018

@davkal and @bergquist can we document this please. Do you want me to create a new docs issue for it?

@davkal

This comment has been minimized.

Copy link
Contributor Author

davkal commented May 14, 2018

Is this feature currently prometheus-only?

It's implemented in the table panel's transformers, so in theory it should be possible to use this with all datasources. I'll take a look at influxdb next week.

Do you want me to create a new docs issue for it?

@daniellee yes pleeeease.

@yellowpattern

This comment has been minimized.

Copy link

yellowpattern commented May 14, 2018

It mostly seems to work but not always?

screen shot 2018-05-14 at 10 17 29 pm

@yellowpattern

This comment has been minimized.

Copy link

yellowpattern commented May 14, 2018

Another example of a failed merge.

screen shot 2018-05-14 at 10 24 09 pm

@marefr

This comment has been minimized.

Copy link
Member

marefr commented May 14, 2018

@yellowpattern please verify the timestamps using the query inspector when you get a failed merged. The epoch timestamps in response as seen in query inspector need to match exactly to be able to merge rows.

@yellowpattern

This comment has been minimized.

Copy link

yellowpattern commented May 14, 2018

I found out that influxdb (version 1.5.2) is the problem. For some queries, influxdb is returning what can only be described as a "corrupt" value for time.

@marefr

This comment has been minimized.

Copy link
Member

marefr commented May 15, 2018

@yellowpattern Thanks for the follow up. That's interesting!

@yellowpattern

This comment has been minimized.

Copy link

yellowpattern commented May 16, 2018

Ok, there are a couple of problems when using influxdb.
#1 is that when multiple queries are sent that use "now()", each will have a different value of 'now'.
#2 data generated by collectd is supplied with second values. the now() in influxdb is millisecond accurate.
#3 Doing now() - 3d for a complex query (such as "max() - min()") will return the result of "now() - 3d" as the time of the query and each row having a different time stamp even if all of the rows have data stored with the same time stamp.

@davkal

This comment has been minimized.

Copy link
Contributor Author

davkal commented May 24, 2018

Cross posting this for posterity:

When using multiple queries, Grafana sends them in the same request, delimited by a ;. I reckon now() gets evaluated when it's parsed. I would expect this time to be the same within the same query, but not across queries even if they arrive in the same request. If you need to rely on some form of bucketing, a date round function could be helpful, but I dont think influx has that.

I'm curious if you need the timestamp column to be displayed. If not, you can mark it as Type: Hidden in the column Styles of Grafana, then they wont be considered for row merges. If you still need to mention some time aspect, you can put it in the panel title as e.g., "Last 3h".

@davkal davkal deleted the davkal:davkal/multi-query-table branch May 24, 2018
@tophei

This comment has been minimized.

Copy link

tophei commented May 27, 2018

@davkal There seems to be a issue when merging 2 plain metrics in a table. The result returned from Prometheus query would contain a builtin label called __name__ representing the metric name, which I believe is added by Grafana somewhere. Thus 2 plain queries will never match in label values since the value of __name__ is different at least. By plain query, I mean simply a plain call to query metric, no aggregation or other functions involved. Is that a known issue? The testing env is Grafana with v5.1.2 and Prometheus with v2.2.1

@propertone

This comment has been minimized.

Copy link

propertone commented May 30, 2018

A simple workaround for this is to add +0 to the query, which causes prometheus to drop the __name__ label (https://www.robustperception.io/whats-in-a-__name__/).

@rcjsuen

This comment has been minimized.

Copy link

rcjsuen commented Jul 15, 2018

Thank you so much for the workaround, @propertone!

What does this imply though? Is this an issue with Prometheus including "redundant" information or should Grafana be stripping it out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.