CloudWatch: Deprecate dynamic labels feature toggle, remove support for Alias in frontend (#67222)

2 years ago · 8516e63937
parent e5aeb7c322
commit 8516e63937
7 changed files with 108 additions and 158 deletions
--- a/.betterer.results
+++ b/.betterer.results
@ -3881,10 +3881,6 @@ exports[`better eslint`] = {
    "public/app/plugins/datasource/cloudwatch/components/LogsQueryField.tsx:5381": [
      [0, 0, 0, "Do not use any type assertions.", "0"]
    ],
-    "public/app/plugins/datasource/cloudwatch/components/MetricsQueryEditor/Alias.tsx:5381": [
-      [0, 0, 0, "Unexpected any. Specify a different type.", "0"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "1"]
-    ],
    "public/app/plugins/datasource/cloudwatch/datasource.ts:5381": [
      [0, 0, 0, "Unexpected any. Specify a different type.", "0"]
    ],
--- a/public/app/plugins/datasource/cloudwatch/components/MetricsQueryEditor/Alias.tsx
+++ b/public/app/plugins/datasource/cloudwatch/components/MetricsQueryEditor/Alias.tsx
@ -1,47 +0,0 @@
-import { debounce } from 'lodash';
-import React, { useState } from 'react';
-
-import { IconButton, Input, Tooltip } from '@grafana/ui';
-
-export interface Props {
-  onChange: (alias: any) => void;
-  value: string;
-  id?: string;
-}
-
-export const Alias = ({ value = '', onChange, id }: Props) => {
-  const [alias, setAlias] = useState(value);
-
-  const propagateOnChange = debounce(onChange, 1500);
-
-  onChange = (e: any) => {
-    setAlias(e.target.value);
-    propagateOnChange(e.target.value);
-  };
-
-  return (
-    <div style={{ display: 'flex', alignItems: 'center' }}>
-      <Input id={id} type="text" value={alias} onChange={onChange} aria-label="Optional alias" />
-      <Tooltip
-        content={
-          <span>
-            Alias pattern will be deprecated in Grafana 10. See{' '}
-            <a
-              target="__blank"
-              rel="noopener noreferrer"
-              href="https://grafana.com/docs/grafana/latest/datasources/aws-cloudwatch/query-editor/#common-query-editor-fields"
-            >
-              documentation
-            </a>{' '}
-            for how to use dynamic labels.
-          </span>
-        }
-        interactive
-        theme="error"
-        placement="right"
-      >
-        <IconButton name="exclamation-triangle" variant="destructive" style={{ marginLeft: 4 }} />
-      </Tooltip>
-    </div>
-  );
-};
--- a/public/app/plugins/datasource/cloudwatch/components/MetricsQueryEditor/MetricsQueryEditor.test.tsx
+++ b/public/app/plugins/datasource/cloudwatch/components/MetricsQueryEditor/MetricsQueryEditor.test.tsx
@ -111,27 +111,22 @@ describe('QueryEditor', () => {
    });
  });

-  describe('when dynamic labels feature toggle is enabled', () => {
-    it('should render label field', async () => {
-      const props = setup();
-      const originalValue = config.featureToggles.cloudWatchDynamicLabels;
-      config.featureToggles.cloudWatchDynamicLabels = true;
-
-      render(
-        <MetricsQueryEditor
-          {...props}
-          query={{ ...props.query, refId: 'A', alias: 'Period: {{period}} InstanceId: {{InstanceId}}' }}
-        />
-      );
-
-      expect(await screen.findByText('Label')).toBeInTheDocument();
-      expect(screen.queryByText('Alias')).toBeNull();
-      expect(screen.getByText("Period: ${PROP('Period')} InstanceId: ${PROP('Dim.InstanceId')}"));
-
-      config.featureToggles.cloudWatchDynamicLabels = originalValue;
-    });
+  it('should render label field and not alias field', async () => {
+    const props = setup();
+
+    render(
+      <MetricsQueryEditor
+        {...props}
+        query={{ ...props.query, refId: 'A', alias: 'Period: {{period}} InstanceId: {{InstanceId}}' }}
+      />
+    );
+
+    expect(await screen.findByText('Label')).toBeInTheDocument();
+    expect(screen.queryByText('Alias')).toBeNull();
+    expect(screen.getByText("Period: ${PROP('Period')} InstanceId: ${PROP('Dim.InstanceId')}"));
  });

+  // TODO: delete this test when dynamic labels feature flag is removed
  describe('when dynamic labels feature toggle is disabled', () => {
    it('should render alias field', async () => {
      const props = setup();
@ -141,9 +136,9 @@ describe('QueryEditor', () => {
      const expected = 'Period: {{period}} InstanceId: {{InstanceId}}';
      render(<MetricsQueryEditor {...props} query={{ ...props.query, refId: 'A', alias: expected }} />);

-      expect(await screen.findByText('Alias')).toBeInTheDocument();
-      expect(screen.queryByText('Label')).toBeNull();
-      expect(screen.getByLabelText('Alias - optional')).toHaveValue(expected);
+      expect(await screen.findByText('Label')).toBeInTheDocument();
+      expect(screen.queryByText('Alias')).toBeNull();
+      expect(screen.getByText("Period: ${PROP('Period')} InstanceId: ${PROP('Dim.InstanceId')}"));

      config.featureToggles.cloudWatchDynamicLabels = originalValue;
    });
--- a/public/app/plugins/datasource/cloudwatch/components/MetricsQueryEditor/MetricsQueryEditor.tsx
+++ b/public/app/plugins/datasource/cloudwatch/components/MetricsQueryEditor/MetricsQueryEditor.tsx
@ -2,7 +2,6 @@ import React, { ChangeEvent, useCallback, useEffect, useState } from 'react';

 import { QueryEditorProps, SelectableValue } from '@grafana/data';
 import { EditorField, EditorRow, InlineSelect, Space } from '@grafana/experimental';
-import { config } from '@grafana/runtime';
 import { ConfirmModal, Input, RadioButtonGroup } from '@grafana/ui';

 import { MathExpressionQueryField, MetricStatEditor, SQLBuilderEditor } from '../';
@ -19,8 +18,6 @@ import {
 import { DynamicLabelsField } from '../DynamicLabelsField';
 import { SQLCodeEditor } from '../SQLCodeEditor';

-import { Alias } from './Alias';
-
 export interface Props extends QueryEditorProps<CloudWatchDatasource, CloudWatchQuery, CloudWatchJsonData> {
  query: CloudWatchMetricsQuery;
  extraHeaderElementLeft?: React.Dispatch<JSX.Element | undefined>;
@ -182,28 +179,18 @@ export const MetricsQueryEditor = (props: Props) => {
          />
        </EditorField>

-        {config.featureToggles.cloudWatchDynamicLabels ? (
-          <EditorField
-            label="Label"
-            width={26}
-            optional
-            tooltip="Change time series legend name using Dynamic labels. See documentation for details."
-          >
-            <DynamicLabelsField
-              width={52}
-              label={migratedQuery.label ?? ''}
-              onChange={(label) => props.onChange({ ...query, label })}
-            ></DynamicLabelsField>
-          </EditorField>
-        ) : (
-          <EditorField label="Alias" width={26} optional tooltip="Change time series legend name using this field.">
-            <Alias
-              id={`${query.refId}-cloudwatch-metric-query-editor-alias`}
-              value={migratedQuery.alias ?? ''}
-              onChange={(value: string) => onChange({ ...migratedQuery, alias: value })}
-            />
-          </EditorField>
-        )}
+        <EditorField
+          label="Label"
+          width={26}
+          optional
+          tooltip="Change time series legend name using Dynamic labels. See documentation for details."
+        >
+          <DynamicLabelsField
+            width={52}
+            label={migratedQuery.label ?? ''}
+            onChange={(label) => props.onChange({ ...query, label })}
+          ></DynamicLabelsField>
+        </EditorField>
      </EditorRow>
    </>
  );
--- a/public/app/plugins/datasource/cloudwatch/migrations/metricQueryMigrations.test.ts
+++ b/public/app/plugins/datasource/cloudwatch/migrations/metricQueryMigrations.test.ts
@ -25,68 +25,87 @@ describe('metricQueryMigrations', () => {
      matchExact: false,
      expression: '',
    };
-    describe('when feature is enabled', () => {
-      describe('and label was not previously migrated', () => {
-        const cases: TestScenario[] = [
-          { description: 'Metric name', alias: '{{metric}}', label: "${PROP('MetricName')}" },
-          { description: 'Trim pattern', alias: '{{  metric     }}', label: "${PROP('MetricName')}" },
-          { description: 'Namespace', alias: '{{namespace}}', label: "${PROP('Namespace')}" },
-          { description: 'Period', alias: '{{period}}', label: "${PROP('Period')}" },
-          { description: 'Region', alias: '{{region}}', label: "${PROP('Region')}" },
-          { description: 'Statistic', alias: '{{stat}}', label: "${PROP('Stat')}" },
-          { description: 'Label', alias: '{{label}}', label: '${LABEL}' },
-          {
-            description: 'Non-existing alias pattern',
-            alias: '{{anything_else}}',
-            label: "${PROP('Dim.anything_else')}",
-          },
-          {
-            description: 'Combined patterns',
-            alias: 'some {{combination}} of {{label}} and {{metric}}',
-            label: "some ${PROP('Dim.combination')} of ${LABEL} and ${PROP('MetricName')}",
-          },
-          {
-            description: 'Combined patterns not trimmed',
-            alias: 'some {{combination  }}{{ label}} and {{metric}}',
-            label: "some ${PROP('Dim.combination')}${LABEL} and ${PROP('MetricName')}",
-          },
-        ];
-        test.each(cases)('given old alias %p, it should be migrated to label: %p', ({ alias, label }) => {
-          config.featureToggles.cloudWatchDynamicLabels = true;
-          const testQuery = { ...baseQuery, alias };
-          const result = migrateAliasPatterns(testQuery);
-          expect(result.label).toBe(label);
-          expect(result.alias).toBe(alias);
-        });
+    describe('when label was not previously migrated', () => {
+      const cases: TestScenario[] = [
+        { description: 'Metric name', alias: '{{metric}}', label: "${PROP('MetricName')}" },
+        { description: 'Trim pattern', alias: '{{  metric     }}', label: "${PROP('MetricName')}" },
+        { description: 'Namespace', alias: '{{namespace}}', label: "${PROP('Namespace')}" },
+        { description: 'Period', alias: '{{period}}', label: "${PROP('Period')}" },
+        { description: 'Region', alias: '{{region}}', label: "${PROP('Region')}" },
+        { description: 'Statistic', alias: '{{stat}}', label: "${PROP('Stat')}" },
+        { description: 'Label', alias: '{{label}}', label: '${LABEL}' },
+        {
+          description: 'Non-existing alias pattern',
+          alias: '{{anything_else}}',
+          label: "${PROP('Dim.anything_else')}",
+        },
+        {
+          description: 'Combined patterns',
+          alias: 'some {{combination}} of {{label}} and {{metric}}',
+          label: "some ${PROP('Dim.combination')} of ${LABEL} and ${PROP('MetricName')}",
+        },
+        {
+          description: 'Combined patterns not trimmed',
+          alias: 'some {{combination  }}{{ label}} and {{metric}}',
+          label: "some ${PROP('Dim.combination')}${LABEL} and ${PROP('MetricName')}",
+        },
+      ];
+      test.each(cases)('given old alias %p, it should be migrated to label: %p', ({ alias, label }) => {
+        const testQuery = { ...baseQuery, alias };
+        const result = migrateAliasPatterns(testQuery);
+        expect(result.label).toBe(label);
+        expect(result.alias).toBe(alias);
      });
+    });

-      describe('and label was previously migrated', () => {
-        const cases: TestScenario[] = [
-          {
-            alias: '',
-            label: "${PROP('MetricName')}",
-          },
-          { alias: '{{metric}}', label: "${PROP('Period')}" },
-          { alias: '{{namespace}}', label: '' },
-        ];
-        test.each(cases)('it should not be migrated once again', ({ alias, label }) => {
-          config.featureToggles.cloudWatchDynamicLabels = true;
-          const testQuery = { ...baseQuery, alias, label };
-          const result = migrateAliasPatterns(testQuery);
-          expect(result.label).toBe(label);
-          expect(result.alias).toBe(alias);
-        });
+    describe('when label was previously migrated', () => {
+      const cases: TestScenario[] = [
+        {
+          alias: '',
+          label: "${PROP('MetricName')}",
+        },
+        { alias: '{{metric}}', label: "${PROP('Period')}" },
+        { alias: '{{namespace}}', label: '' },
+      ];
+      test.each(cases)('it should not be migrated once again', ({ alias, label }) => {
+        const testQuery = { ...baseQuery, alias, label };
+        const result = migrateAliasPatterns(testQuery);
+        expect(result.label).toBe(label);
+        expect(result.alias).toBe(alias);
      });
    });

+    // TODO: delete this test when dynamic labels feature flag is removed
    describe('when feature is disabled', () => {
      const cases: TestScenario[] = [
        { alias: '{{period}}', label: "${PROP('MetricName')}" },
-        { alias: '{{metric}}', label: '' },
+        { alias: '{{metric}}', label: "${PROP('MetricName')}" },
        { alias: '', label: "${PROP('MetricName')}" },
-        { alias: '', label: undefined },
+        { alias: '', label: '' },
+        { description: 'Metric name', alias: '{{metric}}', label: "${PROP('MetricName')}" },
+        { description: 'Trim pattern', alias: '{{  metric     }}', label: "${PROP('MetricName')}" },
+        { description: 'Namespace', alias: '{{namespace}}', label: "${PROP('Namespace')}" },
+        { description: 'Period', alias: '{{period}}', label: "${PROP('Period')}" },
+        { description: 'Region', alias: '{{region}}', label: "${PROP('Region')}" },
+        { description: 'Statistic', alias: '{{stat}}', label: "${PROP('Stat')}" },
+        { description: 'Label', alias: '{{label}}', label: '${LABEL}' },
+        {
+          description: 'Non-existing alias pattern',
+          alias: '{{anything_else}}',
+          label: "${PROP('Dim.anything_else')}",
+        },
+        {
+          description: 'Combined patterns',
+          alias: 'some {{combination}} of {{label}} and {{metric}}',
+          label: "some ${PROP('Dim.combination')} of ${LABEL} and ${PROP('MetricName')}",
+        },
+        {
+          description: 'Combined patterns not trimmed',
+          alias: 'some {{combination  }}{{ label}} and {{metric}}',
+          label: "some ${PROP('Dim.combination')}${LABEL} and ${PROP('MetricName')}",
+        },
      ];
-      test.each(cases)('should not migrate alias', ({ alias, label }) => {
+      test.each(cases)('should migrate alias anyway', ({ alias, label }) => {
        config.featureToggles.cloudWatchDynamicLabels = false;
        const testQuery = { ...baseQuery, alias: `${alias}` };
        if (label !== undefined) {
--- a/public/app/plugins/datasource/cloudwatch/migrations/metricQueryMigrations.ts
+++ b/public/app/plugins/datasource/cloudwatch/migrations/metricQueryMigrations.ts
@ -1,7 +1,5 @@
 import deepEqual from 'fast-deep-equal';

-import { config } from '@grafana/runtime';
-
 import { CloudWatchMetricsQuery } from '../types';

 // Call this function to migrate queries from within the plugin.
@ -20,8 +18,9 @@ const aliasPatterns: Record<string, string> = {
  label: `LABEL`,
 };

+// migrateAliasPatterns in the context of https://github.com/grafana/grafana/issues/48434
 export function migrateAliasPatterns(query: CloudWatchMetricsQuery): CloudWatchMetricsQuery {
-  if (config.featureToggles.cloudWatchDynamicLabels && !query.hasOwnProperty('label')) {
+  if (!query.hasOwnProperty('label')) {
    const newQuery = { ...query };
    if (!query.hasOwnProperty('label')) {
      const regex = /{{\s*(.+?)\s*}}/g;
--- a/public/app/plugins/datasource/cloudwatch/migrations/useMigratedMetricsQuery.test.ts
+++ b/public/app/plugins/datasource/cloudwatch/migrations/useMigratedMetricsQuery.test.ts
@ -10,10 +10,9 @@ import useMigratedMetricsQuery from './useMigratedMetricsQuery';

 describe('usePrepareMetricsQuery', () => {
  const DEFAULT_TEST_QUERY: CloudWatchMetricsQuery = { ...DEFAULT_METRICS_QUERY, refId: 'testId' };
-  describe('when dynamic labels are true and there is no label', () => {
+  describe('when there is no label', () => {
    const testQuery: CloudWatchMetricsQuery = { ...DEFAULT_TEST_QUERY, alias: 'test' };
    it('should replace label with alias and trigger onChangeQuery', async () => {
-      config.featureToggles.cloudWatchDynamicLabels = true;
      const expectedQuery: CloudWatchMetricsQuery = migrateAliasPatterns(testQuery);
      const onChangeQuery = jest.fn();
      const { result } = renderHook(() => useMigratedMetricsQuery(testQuery, onChangeQuery));
@ -31,14 +30,16 @@ describe('usePrepareMetricsQuery', () => {
      expect(onChangeQuery).toHaveBeenCalledTimes(0);
    });
  });
+  // TODO: delete this test when dynamic labels feature flag is removed
  describe('when dynamic labels feature flag is disabled', () => {
    const testQuery: CloudWatchMetricsQuery = { ...DEFAULT_TEST_QUERY };
-    it('should not replace label or trigger onChange', async () => {
+    it('should replace label or trigger onChange', async () => {
+      const expectedQuery: CloudWatchMetricsQuery = migrateAliasPatterns(testQuery);
      config.featureToggles.cloudWatchDynamicLabels = false;
      const onChangeQuery = jest.fn();
      const { result } = renderHook(() => useMigratedMetricsQuery(testQuery, onChangeQuery));
-      expect(result.current).toEqual(testQuery);
-      expect(onChangeQuery).toHaveBeenCalledTimes(0);
+      expect(onChangeQuery).toHaveBeenLastCalledWith(result.current);
+      expect(result.current).toEqual(expectedQuery);
    });
  });
 });