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
This commit is contained in:
ismail simsek 2023-04-14 15:10:31 +02:00 committed by GitHub
parent 2e2f44c2d3
commit 6d53c87862
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 92 additions and 4 deletions

View File

@ -39,6 +39,7 @@ async function assertEditor(query: InfluxQuery, textContent: string) {
const onChange = jest.fn(); const onChange = jest.fn();
const onRunQuery = jest.fn(); const onRunQuery = jest.fn();
const datasource: InfluxDatasource = { const datasource: InfluxDatasource = {
retentionPolicies: [],
metricFindQuery: () => Promise.resolve([]), metricFindQuery: () => Promise.resolve([]),
} as unknown as InfluxDatasource; } as unknown as InfluxDatasource;
const { container } = render( const { container } = render(

View File

@ -1,5 +1,5 @@
import { cloneDeep, extend, groupBy, has, isString, map as _map, omit, pick, reduce } from 'lodash'; 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 { catchError, map } from 'rxjs/operators';
import { import {
@ -33,10 +33,11 @@ import { getTemplateSrv, TemplateSrv } from 'app/features/templating/template_sr
import { AnnotationEditor } from './components/AnnotationEditor'; import { AnnotationEditor } from './components/AnnotationEditor';
import { FluxQueryEditor } from './components/FluxQueryEditor'; import { FluxQueryEditor } from './components/FluxQueryEditor';
import { BROWSER_MODE_DISABLED_MESSAGE } from './constants'; import { BROWSER_MODE_DISABLED_MESSAGE } from './constants';
import { getAllPolicies } from './influxQLMetadataQuery';
import InfluxQueryModel from './influx_query_model'; import InfluxQueryModel from './influx_query_model';
import InfluxSeries from './influx_series'; import InfluxSeries from './influx_series';
import { prepareAnnotation } from './migrations'; import { prepareAnnotation } from './migrations';
import { buildRawQuery } from './queryUtils'; import { buildRawQuery, replaceHardCodedRetentionPolicy } from './queryUtils';
import { InfluxQueryBuilder } from './query_builder'; import { InfluxQueryBuilder } from './query_builder';
import ResponseParser from './response_parser'; import ResponseParser from './response_parser';
import { InfluxOptions, InfluxQuery, InfluxVersion } from './types'; import { InfluxOptions, InfluxQuery, InfluxVersion } from './types';
@ -127,6 +128,7 @@ export default class InfluxDatasource extends DataSourceWithBackend<InfluxQuery,
httpMode: string; httpMode: string;
isFlux: boolean; isFlux: boolean;
isProxyAccess: boolean; isProxyAccess: boolean;
retentionPolicies: string[];
constructor( constructor(
instanceSettings: DataSourceInstanceSettings<InfluxOptions>, instanceSettings: DataSourceInstanceSettings<InfluxOptions>,
@ -152,6 +154,7 @@ export default class InfluxDatasource extends DataSourceWithBackend<InfluxQuery,
this.responseParser = new ResponseParser(); this.responseParser = new ResponseParser();
this.isFlux = settingsData.version === InfluxVersion.Flux; this.isFlux = settingsData.version === InfluxVersion.Flux;
this.isProxyAccess = instanceSettings.access === 'proxy'; this.isProxyAccess = instanceSettings.access === 'proxy';
this.retentionPolicies = [];
if (this.isFlux) { if (this.isFlux) {
// When flux, use an annotation processor rather than the `annotationQuery` lifecycle // When flux, use an annotation processor rather than the `annotationQuery` lifecycle
@ -166,11 +169,47 @@ export default class InfluxDatasource extends DataSourceWithBackend<InfluxQuery,
} }
} }
async getRetentionPolicies(): Promise<string[]> {
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<InfluxQuery>): Observable<DataQueryResponse> { query(request: DataQueryRequest<InfluxQuery>): Observable<DataQueryResponse> {
if (!this.isProxyAccess) { if (!this.isProxyAccess) {
const error = new Error(BROWSER_MODE_DISABLED_MESSAGE); const error = new Error(BROWSER_MODE_DISABLED_MESSAGE);
return throwError(() => error); 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<InfluxQuery>): Observable<DataQueryResponse> {
// for not-flux queries we call `this.classicQuery`, and that // for not-flux queries we call `this.classicQuery`, and that
// handles the is-hidden situation. // handles the is-hidden situation.
// for the flux-case, we do the filtering here // for the flux-case, we do the filtering here
@ -554,6 +593,8 @@ export default class InfluxDatasource extends DataSourceWithBackend<InfluxQuery,
}); });
} }
// By implementing getTagKeys and getTagValues we add ad-hoc filters functionality
// Used in public/app/features/variables/adhoc/picker/AdHocFilterKey.tsx::fetchFilterKeys
getTagKeys(options: any = {}) { getTagKeys(options: any = {}) {
const queryBuilder = new InfluxQueryBuilder({ measurement: options.measurement || '', tags: [] }, this.database); const queryBuilder = new InfluxQueryBuilder({ measurement: options.measurement || '', tags: [] }, this.database);
const query = queryBuilder.buildExploreQuery('TAG_KEYS'); const query = queryBuilder.buildExploreQuery('TAG_KEYS');

View File

@ -1,4 +1,5 @@
import InfluxDatasource from './datasource'; import InfluxDatasource from './datasource';
import { replaceHardCodedRetentionPolicy } from './queryUtils';
import { InfluxQueryBuilder } from './query_builder'; import { InfluxQueryBuilder } from './query_builder';
import { InfluxQueryTag } from './types'; import { InfluxQueryTag } from './types';
@ -11,7 +12,7 @@ const runExploreQuery = (
): Promise<Array<{ text: string }>> => { ): Promise<Array<{ text: string }>> => {
const builder = new InfluxQueryBuilder(target, datasource.database); const builder = new InfluxQueryBuilder(target, datasource.database);
const q = builder.buildExploreQuery(type, withKey, withMeasurementFilter); 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); return datasource.metricFindQuery(q, options);
}; };

View File

@ -1,6 +1,12 @@
import { cloneDeep } from 'lodash'; import { cloneDeep } from 'lodash';
import { buildRawQuery, changeGroupByPart, changeSelectPart, normalizeQuery } from './queryUtils'; import {
buildRawQuery,
changeGroupByPart,
changeSelectPart,
normalizeQuery,
replaceHardCodedRetentionPolicy,
} from './queryUtils';
import { InfluxQuery } from './types'; import { InfluxQuery } from './types';
describe('InfluxDB query utils', () => { 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('');
});
});
}); });

View File

@ -90,3 +90,19 @@ export function changeGroupByPart(query: InfluxQuery, partIndex: number, newPara
}; };
return { ...query, groupBy: newGroupBy }; 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;
}

View File

@ -116,6 +116,8 @@ describe('InfluxDataSource', () => {
} as FetchResponse); } as FetchResponse);
}); });
ctx.ds.retentionPolicies = [''];
try { try {
await lastValueFrom(ctx.ds.query(queryOptions)); await lastValueFrom(ctx.ds.query(queryOptions));
} catch (err) { } 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', () => { describe('Variables should be interpolated correctly', () => {
const instanceSettings: any = {}; const instanceSettings: any = {};
const text = 'interpolationText'; const text = 'interpolationText';