From 6d53c87862e4ef67aa454c1b722282b5dd04b4a8 Mon Sep 17 00:00:00 2001 From: ismail simsek Date: Fri, 14 Apr 2023 15:10:31 +0200 Subject: [PATCH] InfluxDB: Fix querying with hardcoded retention policy (#66466) * Use default retention policy instead of hardcoded retention policy * Load retention policies for the editor * Fix the typo * Add more comment line * Update comment * Better error message * Put back getTagKeys and getTagValues * Fix unit test --- .../VisualInfluxQLEditor/Editor.test.tsx | 1 + .../plugins/datasource/influxdb/datasource.ts | 45 ++++++++++++++++++- .../influxdb/influxQLMetadataQuery.ts | 3 +- .../datasource/influxdb/queryUtils.test.ts | 18 +++++++- .../plugins/datasource/influxdb/queryUtils.ts | 16 +++++++ .../influxdb/specs/datasource.test.ts | 13 ++++++ 6 files changed, 92 insertions(+), 4 deletions(-) diff --git a/public/app/plugins/datasource/influxdb/components/VisualInfluxQLEditor/Editor.test.tsx b/public/app/plugins/datasource/influxdb/components/VisualInfluxQLEditor/Editor.test.tsx index 9caff3d37f9..c763668811d 100644 --- a/public/app/plugins/datasource/influxdb/components/VisualInfluxQLEditor/Editor.test.tsx +++ b/public/app/plugins/datasource/influxdb/components/VisualInfluxQLEditor/Editor.test.tsx @@ -39,6 +39,7 @@ async function assertEditor(query: InfluxQuery, textContent: string) { const onChange = jest.fn(); const onRunQuery = jest.fn(); const datasource: InfluxDatasource = { + retentionPolicies: [], metricFindQuery: () => Promise.resolve([]), } as unknown as InfluxDatasource; const { container } = render( diff --git a/public/app/plugins/datasource/influxdb/datasource.ts b/public/app/plugins/datasource/influxdb/datasource.ts index 88365b8cfbb..04d9dda778f 100644 --- a/public/app/plugins/datasource/influxdb/datasource.ts +++ b/public/app/plugins/datasource/influxdb/datasource.ts @@ -1,5 +1,5 @@ import { cloneDeep, extend, groupBy, has, isString, map as _map, omit, pick, reduce } from 'lodash'; -import { lastValueFrom, merge, Observable, of, throwError } from 'rxjs'; +import { defer, lastValueFrom, merge, mergeMap, Observable, of, throwError } from 'rxjs'; import { catchError, map } from 'rxjs/operators'; import { @@ -33,10 +33,11 @@ import { getTemplateSrv, TemplateSrv } from 'app/features/templating/template_sr import { AnnotationEditor } from './components/AnnotationEditor'; import { FluxQueryEditor } from './components/FluxQueryEditor'; import { BROWSER_MODE_DISABLED_MESSAGE } from './constants'; +import { getAllPolicies } from './influxQLMetadataQuery'; import InfluxQueryModel from './influx_query_model'; import InfluxSeries from './influx_series'; import { prepareAnnotation } from './migrations'; -import { buildRawQuery } from './queryUtils'; +import { buildRawQuery, replaceHardCodedRetentionPolicy } from './queryUtils'; import { InfluxQueryBuilder } from './query_builder'; import ResponseParser from './response_parser'; import { InfluxOptions, InfluxQuery, InfluxVersion } from './types'; @@ -127,6 +128,7 @@ export default class InfluxDatasource extends DataSourceWithBackend, @@ -152,6 +154,7 @@ export default class InfluxDatasource extends DataSourceWithBackend { + if (this.retentionPolicies.length) { + return Promise.resolve(this.retentionPolicies); + } else { + return getAllPolicies(this).catch((err) => { + console.error( + 'Unable to fetch retention policies. Queries will be run without specifying retention policy.', + err + ); + return Promise.resolve(this.retentionPolicies); + }); + } + } + query(request: DataQueryRequest): Observable { if (!this.isProxyAccess) { const error = new Error(BROWSER_MODE_DISABLED_MESSAGE); return throwError(() => error); } + + // When the dashboard first load or on dashboard panel edit mode + // PanelQueryRunner runs the queries to have a visualization on the panel. + // At that point datasource doesn't have the retention policies fetched. + // So hardcoded policy is being sent. Which causes problems. + // To overcome this we check/load policies first and then do the query. + return defer(() => this.getRetentionPolicies()).pipe( + mergeMap((allPolicies) => { + this.retentionPolicies = allPolicies; + const policyFixedRequests = { + ...request, + targets: request.targets.map((t) => ({ + ...t, + policy: replaceHardCodedRetentionPolicy(t.policy, this.retentionPolicies), + })), + }; + return this._query(policyFixedRequests); + }) + ); + } + + _query(request: DataQueryRequest): Observable { // for not-flux queries we call `this.classicQuery`, and that // handles the is-hidden situation. // for the flux-case, we do the filtering here @@ -554,6 +593,8 @@ export default class InfluxDatasource extends DataSourceWithBackend> => { const builder = new InfluxQueryBuilder(target, datasource.database); const q = builder.buildExploreQuery(type, withKey, withMeasurementFilter); - const options = { policy: target.policy }; + const options = { policy: replaceHardCodedRetentionPolicy(target.policy, datasource.retentionPolicies) }; return datasource.metricFindQuery(q, options); }; diff --git a/public/app/plugins/datasource/influxdb/queryUtils.test.ts b/public/app/plugins/datasource/influxdb/queryUtils.test.ts index 1620ae99732..70c3a652cd0 100644 --- a/public/app/plugins/datasource/influxdb/queryUtils.test.ts +++ b/public/app/plugins/datasource/influxdb/queryUtils.test.ts @@ -1,6 +1,12 @@ import { cloneDeep } from 'lodash'; -import { buildRawQuery, changeGroupByPart, changeSelectPart, normalizeQuery } from './queryUtils'; +import { + buildRawQuery, + changeGroupByPart, + changeSelectPart, + normalizeQuery, + replaceHardCodedRetentionPolicy, +} from './queryUtils'; import { InfluxQuery } from './types'; describe('InfluxDB query utils', () => { @@ -435,4 +441,14 @@ describe('InfluxDB query utils', () => { }); }); }); + describe('replaceHardCodedRetentionPolicy', () => { + it('should replace non-existing hardcoded retention policy', () => { + const hardCodedRetentionPolicy = 'default'; + const fetchedRetentionPolicies = ['foo', 'fighters', 'nirvana']; + expect(replaceHardCodedRetentionPolicy(hardCodedRetentionPolicy, fetchedRetentionPolicies)).toBe('foo'); + expect(replaceHardCodedRetentionPolicy('foo', fetchedRetentionPolicies)).toBe('foo'); + expect(replaceHardCodedRetentionPolicy(undefined, fetchedRetentionPolicies)).toBe('foo'); + expect(replaceHardCodedRetentionPolicy(hardCodedRetentionPolicy, [])).toBe(''); + }); + }); }); diff --git a/public/app/plugins/datasource/influxdb/queryUtils.ts b/public/app/plugins/datasource/influxdb/queryUtils.ts index 4e29614906c..c38d6b97077 100644 --- a/public/app/plugins/datasource/influxdb/queryUtils.ts +++ b/public/app/plugins/datasource/influxdb/queryUtils.ts @@ -90,3 +90,19 @@ export function changeGroupByPart(query: InfluxQuery, partIndex: number, newPara }; return { ...query, groupBy: newGroupBy }; } + +// Retention policy was hardcoded as `default` in +// public/app/plugins/datasource/influxdb/components/VisualInfluxQLEditor/FromSection.tsx +// We opted out hardcoded the policy in public/app/plugins/datasource/influxdb/influx_query_model.ts +// Which means if a user has a default retention policy named `default` they cannot use it. +// In https://github.com/grafana/grafana/pull/63820 we introduced a feature to use actual retention policies. +// But this did not consider that some users have hardcoded `default` retention policy in their dashboards. +// This function checks whether the given target has hardcoded retention policy not. +// If it is hardcoded it returns the actual default policy. +export function replaceHardCodedRetentionPolicy(policy: string | undefined, retentionPolicies: string[]): string { + if (!policy || !retentionPolicies.includes(policy)) { + return retentionPolicies[0] ?? ''; + } + + return policy; +} diff --git a/public/app/plugins/datasource/influxdb/specs/datasource.test.ts b/public/app/plugins/datasource/influxdb/specs/datasource.test.ts index e6225f065a2..894d1ad7589 100644 --- a/public/app/plugins/datasource/influxdb/specs/datasource.test.ts +++ b/public/app/plugins/datasource/influxdb/specs/datasource.test.ts @@ -116,6 +116,8 @@ describe('InfluxDataSource', () => { } as FetchResponse); }); + ctx.ds.retentionPolicies = ['']; + try { await lastValueFrom(ctx.ds.query(queryOptions)); } catch (err) { @@ -200,6 +202,17 @@ describe('InfluxDataSource', () => { }); }); + // Some functions are required by the parent datasource class to provide functionality + // such as ad-hoc filters, which requires the definition of the getTagKeys, and getTagValues + describe('Datasource contract', () => { + it('has function called getTagKeys', () => { + expect(Object.getOwnPropertyNames(Object.getPrototypeOf(ctx.ds))).toContain('getTagKeys'); + }); + it('has function called getTagValues', () => { + expect(Object.getOwnPropertyNames(Object.getPrototypeOf(ctx.ds))).toContain('getTagValues'); + }); + }); + describe('Variables should be interpolated correctly', () => { const instanceSettings: any = {}; const text = 'interpolationText';