From 3395ad03a74ad131a4079e1d81a3766d81e000cc Mon Sep 17 00:00:00 2001 From: ismail simsek Date: Fri, 4 Aug 2023 16:57:41 +0300 Subject: [PATCH] InfluxDB: Fix retention policy handling for InfluxDB 3.0 engine by bringing back the hardcoded default (#72467) * Revert retention policy handling * Remove comment * Send retention policy with request when it's possible --- .../hooks/useRetentionPolicies.test.ts | 55 ------------------- .../influxql/hooks/useRetentionPolicies.ts | 15 ----- .../query/influxql/visual/FromSection.tsx | 10 +++- .../influxql/visual/VisualInfluxQLEditor.tsx | 14 +---- .../plugins/datasource/influxdb/datasource.ts | 48 ++-------------- .../datasource/influxdb/influx_query_model.ts | 9 ++- .../influxdb/influxql_metadata_query.ts | 3 +- .../datasource/influxdb/queryUtils.test.ts | 24 +------- .../plugins/datasource/influxdb/queryUtils.ts | 16 ------ .../datasource/influxdb/response_parser.ts | 15 +---- .../app/plugins/datasource/influxdb/types.ts | 2 + 11 files changed, 29 insertions(+), 182 deletions(-) delete mode 100644 public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.test.ts delete mode 100644 public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.ts diff --git a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.test.ts b/public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.test.ts deleted file mode 100644 index f52d2beebf4..00000000000 --- a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.test.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { renderHook } from '@testing-library/react-hooks'; - -import { FetchResponse } from '@grafana/runtime/src'; -import config from 'app/core/config'; - -import { getMockDS, getMockDSInstanceSettings, mockBackendService } from '../../../../../specs/mocks'; - -import { useRetentionPolicies } from './useRetentionPolicies'; - -jest.mock('@grafana/runtime', () => ({ - ...jest.requireActual('@grafana/runtime'), -})); - -describe('useRetentionPolicies', () => { - it('should return all policies when influxdbBackendMigration feature toggle enabled', async () => { - const instanceSettings = getMockDSInstanceSettings(); - const datasource = getMockDS(instanceSettings); - mockBackendService(response); - - config.featureToggles.influxdbBackendMigration = true; - const { result, waitForNextUpdate } = renderHook(() => useRetentionPolicies(datasource)); - await waitForNextUpdate(); - expect(result.current.retentionPolicies.length).toEqual(4); - expect(result.current.retentionPolicies[0]).toEqual('autogen'); - }); -}); - -const response: FetchResponse = { - config: { - url: 'mock-response-url', - }, - headers: new Headers(), - ok: false, - redirected: false, - status: 0, - statusText: '', - type: 'basic', - url: '', - data: { - results: { - metadataQuery: { - status: 200, - frames: [ - { - schema: { - refId: 'metadataQuery', - fields: [{ name: 'value', type: 'string', typeInfo: { frame: 'string' } }], - }, - data: { values: [['autogen', 'bar', '5m_avg', '1m_avg']] }, - }, - ], - }, - }, - }, -}; diff --git a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.ts b/public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.ts deleted file mode 100644 index c25b795294c..00000000000 --- a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/hooks/useRetentionPolicies.ts +++ /dev/null @@ -1,15 +0,0 @@ -import { useEffect, useState } from 'react'; - -import InfluxDatasource from '../../../../../datasource'; -import { getAllPolicies } from '../../../../../influxql_metadata_query'; - -export const useRetentionPolicies = (datasource: InfluxDatasource) => { - const [retentionPolicies, setRetentionPolicies] = useState([]); - useEffect(() => { - getAllPolicies(datasource).then((data) => { - setRetentionPolicies(data); - }); - }, [datasource]); - - return { retentionPolicies }; -}; diff --git a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/FromSection.tsx b/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/FromSection.tsx index e0a27555da8..979114a71fd 100644 --- a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/FromSection.tsx +++ b/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/FromSection.tsx @@ -1,5 +1,6 @@ import React from 'react'; +import { DEFAULT_POLICY } from '../../../../../types'; import { toSelectableValue } from '../utils/toSelectableValue'; import { Seg } from './Seg'; @@ -32,7 +33,12 @@ export const FromSection = ({ }: Props): JSX.Element => { const handlePolicyLoadOptions = async () => { const allPolicies = await getPolicyOptions(); - return allPolicies.map(toSelectableValue); + // if `default` does not exist in the list of policies, we add it + const allPoliciesWithDefault = allPolicies.some((p) => p === 'default') + ? allPolicies + : [DEFAULT_POLICY, ...allPolicies]; + + return allPoliciesWithDefault.map(toSelectableValue); }; const handleMeasurementLoadOptions = async (filter: string) => { @@ -44,7 +50,7 @@ export const FromSection = ({ <> { onChange(v.value, measurement); diff --git a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/VisualInfluxQLEditor.tsx b/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/VisualInfluxQLEditor.tsx index a3d423cf27c..9b6b79fb0cc 100644 --- a/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/VisualInfluxQLEditor.tsx +++ b/public/app/plugins/datasource/influxdb/components/editor/query/influxql/visual/VisualInfluxQLEditor.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import React, { useEffect, useId, useMemo } from 'react'; +import React, { useId, useMemo } from 'react'; import { GrafanaTheme2 } from '@grafana/data'; import { InlineLabel, SegmentSection, useStyles2 } from '@grafana/ui'; @@ -23,7 +23,6 @@ import { } from '../../../../../queryUtils'; import { InfluxQuery, InfluxQueryTag } from '../../../../../types'; import { DEFAULT_RESULT_FORMAT } from '../../../constants'; -import { useRetentionPolicies } from '../hooks/useRetentionPolicies'; import { filterTags } from '../utils/filterTags'; import { getNewGroupByPartOptions, getNewSelectPartOptions, makePartList } from '../utils/partListUtils'; import { withTemplateVariableOptions } from '../utils/withTemplateVariableOptions'; @@ -52,17 +51,6 @@ export const VisualInfluxQLEditor = (props: Props): JSX.Element => { const query = normalizeQuery(props.query); const { datasource } = props; const { measurement, policy } = query; - const { retentionPolicies } = useRetentionPolicies(datasource); - - useEffect(() => { - if (!policy && retentionPolicies.length > 0) { - props.onChange({ - ...query, - policy: retentionPolicies[0], - }); - props.onRunQuery(); - } - }, [policy, props, query, retentionPolicies]); const allTagKeys = useMemo(async () => { const tagKeys = (await getTagKeysForMeasurementAndTags(datasource, [], measurement, policy)).map( diff --git a/public/app/plugins/datasource/influxdb/datasource.ts b/public/app/plugins/datasource/influxdb/datasource.ts index 6806d3d2611..59da1fdcdb0 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 { defer, lastValueFrom, merge, mergeMap, Observable, of, throwError } from 'rxjs'; +import { lastValueFrom, merge, Observable, of, throwError } from 'rxjs'; import { catchError, map } from 'rxjs/operators'; import { @@ -36,12 +36,11 @@ import { FluxQueryEditor } from './components/editor/query/flux/FluxQueryEditor' import { BROWSER_MODE_DISABLED_MESSAGE } from './constants'; import InfluxQueryModel from './influx_query_model'; import InfluxSeries from './influx_series'; -import { getAllPolicies } from './influxql_metadata_query'; import { buildMetadataQuery } from './influxql_query_builder'; import { prepareAnnotation } from './migrations'; -import { buildRawQuery, replaceHardCodedRetentionPolicy } from './queryUtils'; +import { buildRawQuery } from './queryUtils'; import ResponseParser from './response_parser'; -import { InfluxOptions, InfluxQuery, InfluxVersion } from './types'; +import { DEFAULT_POLICY, InfluxOptions, InfluxQuery, InfluxVersion } from './types'; export default class InfluxDatasource extends DataSourceWithBackend { type: string; @@ -57,7 +56,6 @@ export default class InfluxDatasource extends DataSourceWithBackend, @@ -83,7 +81,6 @@ export default class InfluxDatasource extends DataSourceWithBackend { - // Only For InfluxQL Mode - if (this.version === InfluxVersion.InfluxQL && !this.retentionPolicies.length) { - 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); - }); - } - - 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 = t.policy ? this.templateSrv.replace(t.policy, {}, 'regex') : ''; - return { - ...t, - policy: replaceHardCodedRetentionPolicy(t.policy, this.retentionPolicies), - }; - }), - }; - return this._query(policyFixedRequests); - }) - ); + return this._query(request); } _query(request: DataQueryRequest): Observable { @@ -463,7 +425,7 @@ export default class InfluxDatasource extends DataSourceWithBackend { @@ -43,9 +37,7 @@ describe('InfluxDB query utils', () => { ], ], }) - ).toBe( - 'SELECT mean("value") FROM "default"."measurement" WHERE $timeFilter GROUP BY time($__interval)' + ' fill(null)' - ); + ).toBe('SELECT mean("value") FROM "measurement" WHERE $timeFilter GROUP BY time($__interval)' + ' fill(null)'); }); it('should handle small query', () => { expect( @@ -245,7 +237,7 @@ describe('InfluxDB query utils', () => { }) ).toBe( `SELECT holt_winters_with_fit(mean("usage_idle"), 30, 5), median("usage_guest") ` + - `FROM "default"."cpu" ` + + `FROM "cpu" ` + `WHERE ("cpu" = 'cpu2' OR "cpu" = 'cpu3' AND "cpu" = 'cpu1') ` + `AND $timeFilter ` + `GROUP BY time($__interval), "cpu", "host" fill(none) ` + @@ -441,14 +433,4 @@ 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 c38d6b97077..4e29614906c 100644 --- a/public/app/plugins/datasource/influxdb/queryUtils.ts +++ b/public/app/plugins/datasource/influxdb/queryUtils.ts @@ -90,19 +90,3 @@ 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/response_parser.ts b/public/app/plugins/datasource/influxdb/response_parser.ts index 796ebc5812e..a9aa5ac8cc6 100644 --- a/public/app/plugins/datasource/influxdb/response_parser.ts +++ b/public/app/plugins/datasource/influxdb/response_parser.ts @@ -38,20 +38,7 @@ export default class ResponseParser { // (while the newer versions—first). if (isValueFirst) { - // We want to know whether the given retention policy is the default one or not. - // If it is default policy then we should add it to the beginning. - // The index 4 gives us if that policy is default or not. - // https://docs.influxdata.com/influxdb/v1.8/query_language/explore-schema/#show-retention-policies - // Only difference is v0.9. In that version we don't receive shardGroupDuration value. - // https://archive.docs.influxdata.com/influxdb/v0.9/query_language/schema_exploration/#show-retention-policies - // Since it is always the last value we will check that last value always. - if (isRetentionPolicyQuery && value[value.length - 1] === true) { - const newSetValues = [value[0].toString(), ...Array.from(res)]; - res.clear(); - newSetValues.forEach((sv) => res.add(sv)); - } else { - res.add(value[0].toString()); - } + res.add(value[0].toString()); } else if (value[1] !== undefined) { res.add(value[1].toString()); } else { diff --git a/public/app/plugins/datasource/influxdb/types.ts b/public/app/plugins/datasource/influxdb/types.ts index 692949a2885..b9e257e3fe9 100644 --- a/public/app/plugins/datasource/influxdb/types.ts +++ b/public/app/plugins/datasource/influxdb/types.ts @@ -1,5 +1,7 @@ import { AdHocVariableFilter, DataQuery, DataSourceJsonData } from '@grafana/data'; +export const DEFAULT_POLICY = 'default'; + export enum InfluxVersion { InfluxQL = 'InfluxQL', Flux = 'Flux',