configurable default database, improved ad-hoc filters, parse key columns from query #60
Conversation
…lters The datasource now allows default database to be configurable, this is used in two places: * In query editor, the database field is prefilled with the default database * In ad-hoc filters, the database is stripped from column names to make them less verbose (e.g. if default database is 'default', it will only show 'table.column' pairs) The datasource also allows non-fully-qualified column names in the filter. In such case the database and table will be filled for each target independently, e.g. if there are two graphs in the dashboard with 'default.table1' and 'default.table2', and ad-hoc filter is 'column1 = 5', then it will apply in both cases. If the ad-hoc filter is 'table2.column1 = 5' then it will only apply in the second graph.
This removes the need to wrap everything in subqueries with groupArray() and fixes several widget types (notably heatmap). If there's just time column, in the GROUP BY, it will use the metric alias name as before. If there's more columns, it will generate the timeseries name as a join of dimension values, e.g: `GROUP BY t, OS, Country` can generate timeseries `Linux, US`
There was a problem hiding this comment.
Hi @vavrusa
Thank you for the help with plugin! It's always great to get some new ideas and use-case scenarios.
I like the idea with default database. Come to think of it, in 99% cases all data which can be used as filter will be located in the same database. It makes adhoc options more readable, thx for this!
Datapoints grouping without subqueries is great! The only thing I'm afraid of is alerting plugin(implemented here). Because we need to move the same parsing logic from js to go implementation, which can make it more complicated or unclear. What do you think about it?
dist/datasource.ts
Outdated
| @@ -35,6 +37,7 @@ export class ClickHouseDatasource { | |||
| this.withCredentials = instanceSettings.withCredentials; | |||
| this.addCorsHeader = instanceSettings.jsonData.addCorsHeader; | |||
| this.usePOST = instanceSettings.jsonData.usePOST; | |||
| this.defaultDatabase = instanceSettings.jsonData.defaultDatabase; | |||
There was a problem hiding this comment.
It will turn to undefined if user didn't set anything at Settings page and produce an error when we will try to check its length. So I suggest to do following:
this.defaultDatabase = instanceSettings.jsonData.defaultDatabase || '';
dist/adhoc.ts
Outdated
| return datasource.metricFindQuery(columnsQuery) | ||
| let query = columnsQuery.replace('{filter}', queryFilter); | ||
| if (datasource.defaultDatabase.length > 0) { | ||
| query = columnsQuery.replace('{filter}', "database = '" + datasource.defaultDatabase + "' AND " + queryFilter); |
There was a problem hiding this comment.
Can be simplified to following:
let filter = queryFilter;
if (datasource.defaultDatabase.length > 0) {
filter = "database = '" + datasource.defaultDatabase "' AND " + queryFilter
}
let query = columnsQuery.replace('{filter}', queryFilter);
dist/datasource.ts
Outdated
|
|
||
| _.map(options.targets, (target) => { | ||
| if (!target.hide && target.query) { | ||
| var queryModel = new SqlQuery(target, this.templateSrv, options); | ||
| q = queryModel.replace(options, adhocFilters); | ||
| queries.push(q); | ||
| let queryAST = new Scanner(q).toAST(); |
There was a problem hiding this comment.
The problem here is with my Scanner. I'm not quite sure about it, that's why parsing inside of SqlQuery is wrapped with try-catch, which will process the query even if Scanner fails. Probably, we need to wrap it here too.
| <div class="gf-form-inline"> | ||
| <div class="gf-form max-width-30"> | ||
| <span class="gf-form-label width-10">Default database</span> | ||
| <input type="text" class="gf-form-input max-width-20" ng-model='ctrl.current.jsonData.defaultDatabase' placeholder="default"></input> |
There was a problem hiding this comment.
Could you please add tooltip description?
| @@ -2,6 +2,7 @@ import _ from 'lodash'; | |||
|
|
|||
| export default class SqlSeries { | |||
| series: any; | |||
| keys: any; | |||
|
@hagen1778 Thanks for reviewing. I'm not sure how alerting is implemented, but it will need some sort of introspection anyway. I've fixed the remarks from this PR and added a short doc to README on how to build the project and run tests. |
|
Thanks for contributing! |
|
@hagen1778 any chance of tagging this so we could install the module? |
|
@hagen1778 thanks! |
|
@hagen1778 I think you also need to bump the package version here https://github.com/Vertamedia/clickhouse-grafana/blob/master/package.json#L3 from 1.4.3 before submitting it to grafana plugin repo |
|
Oh, you are totally right. That's my bad |
|
Updated, thx again! |
Hi, there are three improvements in this PR:
Here's some rationale:
The datasource now allows default database to be configurable, this is used in two places:
less verbose (e.g. if default database is 'default', it will only show 'table.column' pairs)
The datasource also allows non-fully-qualified column names in the filter. In such case the database and table will be filled for each target independently, e.g. if there are two graphs in the dashboard with 'default.table1' and 'default.table2', and ad-hoc filter is 'column1 = 5', then it will apply in both cases. If the ad-hoc filter is 'table2.column1 = 5' then it will only apply in the second graph.
The columns representing dimensions are parsed from GROUP BY, which removes the need to wrap everything in subqueries with
groupArray()and fixes several widget types (notably heatmap, table). If there's just time column, in the GROUP BY, it will use the metric alias name as before. If there's morecolumns, it will generate the timeseries name as a join of dimension values, e.g:
GROUP BY t, OS, Countrycan generate timeseriesLinux, US