AzureMonitor: Fix ResourcePicker hanging (#71886)

* AzureMonitor: Fix ResourcePicker hanging Removed location fetching for every subscription. Fixes #70523 * fix region for multi metricnames * fix tests * fix e2e * remove regions/locations from variable editor * remove region pieces from e2e
2025-02-25 18:55:37 -06:00 · 2023-07-25 10:18:03 -04:00 · 2023-07-25 10:18:03 -04:00 · 722f787eaa
commit 722f787eaa
parent d31d175109
11 changed files with 16 additions and 156 deletions
--- a/e2e/cloud-plugins-suite/azure-monitor.spec.ts
+++ b/e2e/cloud-plugins-suite/azure-monitor.spec.ts
@ -126,10 +126,6 @@ const addAzureMonitorVariable = (
        .input()
        .find('input')
        .type(`${options?.namespace}{enter}`);
-      e2eSelectors.variableEditor.region
-        .input()
-        .find('input')
-        .type(`${options?.region}{enter}`);
      break;
    case AzureQueryType.MetricNamesQuery:
      e2eSelectors.variableEditor.subscription
@ -207,14 +203,11 @@ e2e.scenario({
        zone: 'Coordinated Universal Time',
      },
    });
-    e2e()
-      .intercept(/locations/)
-      .as('locations');
    e2e.flows.addPanel({
      dataSourceName,
      visitDashboardAtStart: false,
      queriesForm: () => {
-        e2eSelectors.queryEditor.resourcePicker.select.button().click().wait('@locations');
+        e2eSelectors.queryEditor.resourcePicker.select.button().click();
        e2eSelectors.queryEditor.resourcePicker.search
          .input()
          .wait(100)
@ -309,14 +302,10 @@ e2e.scenario({
      subscription: '$subscription',
      resourceGroup: '$resourceGroups',
    });
-    addAzureMonitorVariable('region', AzureQueryType.LocationsQuery, false, {
-      subscription: '$subscription',
-    });
    addAzureMonitorVariable('resource', AzureQueryType.ResourceNamesQuery, false, {
      subscription: '$subscription',
      resourceGroup: '$resourceGroups',
      namespace: '$namespace',
-      region: '$region',
    });
    e2e.pages.Dashboard.SubMenu.submenuItemLabels('subscription').click();
    e2e.pages.Dashboard.SubMenu.submenuItemValueDropDownOptionTexts('grafanalabs-datasources-dev').click();
@ -330,8 +319,6 @@ e2e.scenario({
      .parent()
      .find('input')
      .type('microsoft.storage/storageaccounts{downArrow}{enter}');
-    e2e.pages.Dashboard.SubMenu.submenuItemLabels('region').parent().find('button').click();
-    e2e.pages.Dashboard.SubMenu.submenuItemLabels('region').parent().find('input').type('uk south{downArrow}{enter}');
    e2e.pages.Dashboard.SubMenu.submenuItemLabels('resource').parent().find('button').click();
    e2e.pages.Dashboard.SubMenu.submenuItemLabels('resource')
      .parent()
@ -346,7 +333,6 @@ e2e.scenario({
        e2eSelectors.queryEditor.resourcePicker.advanced.subscription.input().find('input').type('$subscription');
        e2eSelectors.queryEditor.resourcePicker.advanced.resourceGroup.input().find('input').type('$resourceGroups');
        e2eSelectors.queryEditor.resourcePicker.advanced.namespace.input().find('input').type('$namespaces');
-        e2eSelectors.queryEditor.resourcePicker.advanced.region.input().find('input').type('$region');
        e2eSelectors.queryEditor.resourcePicker.advanced.resource.input().find('input').type('$resource');
        e2eSelectors.queryEditor.resourcePicker.apply.button().click();
        e2eSelectors.queryEditor.metricsQueryEditor.metricName.input().find('input').type('Transactions{enter}');
--- a/public/app/plugins/datasource/azuremonitor/mocks/resourcePickerRows.ts
+++ b/public/app/plugins/datasource/azuremonitor/mocks/resourcePickerRows.ts
@ -1,5 +1,4 @@
 import { ResourceRowGroup, ResourceRowType } from '../components/ResourcePicker/types';
-import { AzureMonitorLocations } from '../types';

 export const createMockSubscriptions = (): ResourceRowGroup => [
  {
@ -133,6 +132,3 @@ export const mockSearchResults = (): ResourceRowGroup => [
    location: 'northeurope',
  },
 ];
-
-export const mockGetValidLocations = (): Map<string, AzureMonitorLocations> =>
-  new Map([['northeurope', { displayName: 'North Europe', name: 'northeurope', supportsLogs: true }]]);
--- a/public/app/plugins/datasource/azuremonitor/components/MetricsQueryEditor/MetricsQueryEditor.test.tsx
+++ b/public/app/plugins/datasource/azuremonitor/components/MetricsQueryEditor/MetricsQueryEditor.test.tsx
@ -10,7 +10,6 @@ import createMockQuery from '../../__mocks__/query';
 import {
  createMockResourceGroupsBySubscription,
  createMockSubscriptions,
-  mockGetValidLocations,
  mockResourcesByResourceGroup,
 } from '../../__mocks__/resourcePickerRows';
 import { selectors } from '../../e2e/selectors';
@ -46,7 +45,6 @@ export function createMockResourcePickerData() {
  mockResourcePicker.getResourcesForResourceGroup = jest.fn().mockResolvedValue(mockResourcesByResourceGroup());
  mockResourcePicker.getResourceURIFromWorkspace = jest.fn().mockReturnValue('');
  mockResourcePicker.getResourceURIDisplayProperties = jest.fn().mockResolvedValue({});
-  mockResourcePicker.getLocations = jest.fn().mockResolvedValue(mockGetValidLocations());
  return mockResourcePicker;
 }

--- a/public/app/plugins/datasource/azuremonitor/components/MetricsQueryEditor/dataHooks.ts
+++ b/public/app/plugins/datasource/azuremonitor/components/MetricsQueryEditor/dataHooks.ts
@ -128,7 +128,8 @@ const defaultMetricMetadata: MetricMetadata = {
 export const useMetricMetadata = (query: AzureMonitorQuery, datasource: Datasource, onChange: OnChangeFn) => {
  const [metricMetadata, setMetricMetadata] = useState<MetricMetadata>(defaultMetricMetadata);
  const { subscription } = query;
-  const { resources, metricNamespace, metricName, aggregation, timeGrain, customNamespace } = query.azureMonitor ?? {};
+  const { resources, metricNamespace, metricName, aggregation, timeGrain, customNamespace, region } =
+    query.azureMonitor ?? {};
  const { resourceGroup, resourceName } = getResourceGroupAndName(resources);
  const multipleResources = (resources && resources.length > 1) ?? false;

@ -138,11 +139,11 @@ export const useMetricMetadata = (query: AzureMonitorQuery, datasource: Datasour
      setMetricMetadata(defaultMetricMetadata);
      return;
    }
-
    datasource.azureMonitorDatasource
      .getMetricMetadata(
        { subscription, resourceGroup, resourceName, metricNamespace, metricName, customNamespace },
-        multipleResources
+        multipleResources,
+        region
      )
      .then((metadata) => {
        // TODO: Move the aggregationTypes and timeGrain defaults into `getMetricMetadata`
@ -161,6 +162,7 @@ export const useMetricMetadata = (query: AzureMonitorQuery, datasource: Datasour
        });
      });
  }, [
+    region,
    datasource,
    subscription,
    resourceGroup,
--- a/public/app/plugins/datasource/azuremonitor/components/ResourcePicker/NestedRow.tsx
+++ b/public/app/plugins/datasource/azuremonitor/components/ResourcePicker/NestedRow.tsx
@ -78,7 +78,7 @@ const NestedRow = ({

        <td className={styles.cell}>{row.typeLabel}</td>

-        <td className={styles.cell}>{row.locationDisplayName ?? '-'}</td>
+        <td className={styles.cell}>{row.location ?? '-'}</td>
      </tr>

      {isOpen &&
--- a/public/app/plugins/datasource/azuremonitor/components/ResourcePicker/types.ts
+++ b/public/app/plugins/datasource/azuremonitor/components/ResourcePicker/types.ts
@ -12,7 +12,6 @@ export interface ResourceRow {
  name: string;
  type: ResourceRowType;
  typeLabel: string;
-  locationDisplayName?: string;
  location?: string;
  children?: ResourceRowGroup;
 }
--- a/public/app/plugins/datasource/azuremonitor/components/VariableEditor/VariableEditor.test.tsx
+++ b/public/app/plugins/datasource/azuremonitor/components/VariableEditor/VariableEditor.test.tsx
@ -258,12 +258,10 @@ describe('VariableEditor:', () => {
      await waitFor(() => expect(screen.getByText('Logs')).toBeInTheDocument());
      await selectAndRerender('select query type', 'Resource Names', onChange, rerender);
      await selectAndRerender('select subscription', 'Primary Subscription', onChange, rerender);
-      await selectAndRerender('select region', 'North Europe', onChange, rerender);
      expect(onChange).toHaveBeenCalledWith(
        expect.objectContaining({
          queryType: AzureQueryType.ResourceNamesQuery,
          subscription: 'sub',
-          region: 'northeurope',
          refId: 'A',
        })
      );
@ -325,21 +323,5 @@ describe('VariableEditor:', () => {
        })
      );
    });
-
-    it('should run the query if requesting regions', async () => {
-      const onChange = jest.fn();
-      const { rerender } = render(<VariableEditor {...defaultProps} onChange={onChange} />);
-      // wait for initial load
-      await waitFor(() => expect(screen.getByText('Logs')).toBeInTheDocument());
-      await selectAndRerender('select query type', 'Regions', onChange, rerender);
-      await selectAndRerender('select subscription', 'Primary Subscription', onChange, rerender);
-      expect(onChange).toHaveBeenCalledWith(
-        expect.objectContaining({
-          queryType: AzureQueryType.LocationsQuery,
-          subscription: 'sub',
-          refId: 'A',
-        })
-      );
-    });
  });
 });
--- a/public/app/plugins/datasource/azuremonitor/components/VariableEditor/VariableEditor.tsx
+++ b/public/app/plugins/datasource/azuremonitor/components/VariableEditor/VariableEditor.tsx
@ -30,7 +30,6 @@ const VariableEditor = (props: Props) => {
    { label: 'Subscriptions', value: AzureQueryType.SubscriptionsQuery },
    { label: 'Resource Groups', value: AzureQueryType.ResourceGroupsQuery },
    { label: 'Namespaces', value: AzureQueryType.NamespacesQuery },
-    { label: 'Regions', value: AzureQueryType.LocationsQuery },
    { label: 'Resource Names', value: AzureQueryType.ResourceNamesQuery },
    { label: 'Metric Names', value: AzureQueryType.MetricNamesQuery },
    { label: 'Workspaces', value: AzureQueryType.WorkspacesQuery },
@ -51,7 +50,6 @@ const VariableEditor = (props: Props) => {
  const [requireSubscription, setRequireSubscription] = useState(false);
  const [hasResourceGroup, setHasResourceGroup] = useState(false);
  const [hasNamespace, setHasNamespace] = useState(false);
-  const [hasRegion, setHasRegion] = useState(false);
  const [requireResourceGroup, setRequireResourceGroup] = useState(false);
  const [requireNamespace, setRequireNamespace] = useState(false);
  const [requireResource, setRequireResource] = useState(false);
@ -59,7 +57,6 @@ const VariableEditor = (props: Props) => {
  const [resourceGroups, setResourceGroups] = useState<SelectableValue[]>([]);
  const [namespaces, setNamespaces] = useState<SelectableValue[]>([]);
  const [resources, setResources] = useState<SelectableValue[]>([]);
-  const [regions, setRegions] = useState<SelectableValue[]>([]);
  const [errorMessage, setError] = useLastError();
  const queryType = typeof query === 'string' ? '' : query.queryType;

@ -91,7 +88,6 @@ const VariableEditor = (props: Props) => {
        setRequireSubscription(true);
        setHasResourceGroup(true);
        setHasNamespace(true);
-        setHasRegion(true);
        break;
      case AzureQueryType.MetricNamesQuery:
        setRequireSubscription(true);
@ -99,9 +95,6 @@ const VariableEditor = (props: Props) => {
        setRequireNamespace(true);
        setRequireResource(true);
        break;
-      case AzureQueryType.LocationsQuery:
-        setRequireSubscription(true);
-        break;
    }
  }, [queryType]);

@ -142,16 +135,6 @@ const VariableEditor = (props: Props) => {
    }
  }, [datasource, subscription, resourceGroup]);

-  useEffect(() => {
-    if (subscription) {
-      datasource.azureMonitorDatasource.getLocations([subscription]).then((rgs) => {
-        const regions: SelectableValue[] = [];
-        rgs.forEach((r) => regions.push({ label: r.displayName, value: r.name }));
-        setRegions(regions);
-      });
-    }
-  }, [datasource, subscription, resourceGroup]);
-
  const namespace = (typeof query === 'object' && query.namespace) || '';
  useEffect(() => {
    if (subscription) {
@ -208,13 +191,6 @@ const VariableEditor = (props: Props) => {
    });
  };

-  const onChangeRegion = (selectableValue: SelectableValue) => {
-    onChange({
-      ...query,
-      region: selectableValue.value,
-    });
-  };
-
  const onChangeResource = (selectableValue: SelectableValue) => {
    onChange({
      ...query,
@ -320,22 +296,6 @@ const VariableEditor = (props: Props) => {
          />
        </InlineField>
      )}
-      {hasRegion && (
-        <InlineField
-          label="Select region"
-          labelWidth={20}
-          data-testid={selectors.components.variableEditor.region.input}
-        >
-          <Select
-            aria-label="select region"
-            onChange={onChangeRegion}
-            options={regions.concat(variableOptionGroup)}
-            width={25}
-            value={query.region || null}
-            placeholder="Optional"
-          />
-        </InlineField>
-      )}
      {requireResource && (
        <InlineField
          label="Select resource"
--- a/public/app/plugins/datasource/azuremonitor/resourcePicker/resourcePickerData.test.ts
+++ b/public/app/plugins/datasource/azuremonitor/resourcePicker/resourcePickerData.test.ts
@ -5,7 +5,6 @@ import {
 } from '../__mocks__/argResourcePickerResponse';
 import createMockDatasource from '../__mocks__/datasource';
 import { createMockInstanceSetttings } from '../__mocks__/instanceSettings';
-import { mockGetValidLocations } from '../__mocks__/resourcePickerRows';
 import { ResourceRowType } from '../components/ResourcePicker/types';
 import { AzureGraphResponse } from '../types';

@ -32,11 +31,7 @@ const createResourcePickerData = (responses: AzureGraphResponse[]) => {
    postResource.mockResolvedValueOnce(res);
  });
  resourcePickerData.postResource = postResource;
-  const locationsMap = mockGetValidLocations();
-  const getLocations = jest.spyOn(resourcePickerData, 'getLocations').mockResolvedValue(locationsMap);
-  resourcePickerData.locationsMap = locationsMap;
-  resourcePickerData.locations = Array.from(locationsMap.values()).map((location) => `"${location.name}"`);
-  return { resourcePickerData, postResource, mockDatasource, getValidLocations: getLocations };
+  return { resourcePickerData, postResource, mockDatasource };
 };

 describe('AzureMonitor resourcePickerData', () => {
@ -252,7 +247,7 @@ describe('AzureMonitor resourcePickerData', () => {
        name: 'web-server',
        type: 'Resource',
        location: 'northeurope',
-        locationDisplayName: 'North Europe',
+        locationDisplayName: 'northeurope',
        resourceGroupName: 'dev',
        typeLabel: 'Microsoft.Compute/virtualMachines',
        uri: '/subscriptions/def-456/resourceGroups/dev/providers/Microsoft.Compute/virtualMachines/web-server',
@ -329,7 +324,7 @@ describe('AzureMonitor resourcePickerData', () => {
        id: 'vmname',
        name: 'vmName',
        type: 'Resource',
-        location: 'North Europe',
+        location: 'northeurope',
        resourceGroupName: 'rgName',
        typeLabel: 'Virtual machines',
        uri: '/subscriptions/subId/resourceGroups/rgName/providers/Microsoft.Compute/virtualMachines/vmname',
@ -359,7 +354,7 @@ describe('AzureMonitor resourcePickerData', () => {
        id: 'rgName',
        name: 'rgName',
        type: 'ResourceGroup',
-        location: 'North Europe',
+        location: 'northeurope',
        resourceGroupName: 'rgName',
        typeLabel: 'Resource groups',
        uri: '/subscriptions/subId/resourceGroups/rgName',
@ -392,35 +387,6 @@ describe('AzureMonitor resourcePickerData', () => {
    });
  });

-  describe('getValidLocations', () => {
-    it('returns a locations map', async () => {
-      const { resourcePickerData, getValidLocations } = createResourcePickerData([createMockARGSubscriptionResponse()]);
-      getValidLocations.mockRestore();
-      const subscriptions = await resourcePickerData.getSubscriptions();
-      const locations = await resourcePickerData.getLocations(subscriptions);
-
-      expect(locations.size).toBe(1);
-      expect(locations.has('northeurope')).toBe(true);
-      expect(locations.get('northeurope')?.name).toBe('northeurope');
-      expect(locations.get('northeurope')?.displayName).toBe('North Europe');
-    });
-
-    it('returns the raw locations map if provider is undefined', async () => {
-      const { resourcePickerData, mockDatasource, getValidLocations } = createResourcePickerData([
-        createMockARGSubscriptionResponse(),
-      ]);
-      getValidLocations.mockRestore();
-      mockDatasource.azureMonitorDatasource.getProvider = jest.fn().mockResolvedValue(undefined);
-      const subscriptions = await resourcePickerData.getSubscriptions();
-      const locations = await resourcePickerData.getLocations(subscriptions);
-
-      expect(locations.size).toBe(1);
-      expect(locations.has('northeurope')).toBe(true);
-      expect(locations.get('northeurope')?.name).toBe('northeurope');
-      expect(locations.get('northeurope')?.displayName).toBe('North Europe');
-    });
-  });
-
  describe('fetchInitialRows', () => {
    it('returns a list of subscriptions', async () => {
      const { resourcePickerData } = createResourcePickerData([createMockARGSubscriptionResponse()]);
--- a/public/app/plugins/datasource/azuremonitor/resourcePicker/resourcePickerData.ts
+++ b/public/app/plugins/datasource/azuremonitor/resourcePicker/resourcePickerData.ts
@ -1,7 +1,7 @@
 import { uniq } from 'lodash';

 import { DataSourceInstanceSettings } from '@grafana/data';
-import { DataSourceWithBackend } from '@grafana/runtime';
+import { DataSourceWithBackend, reportInteraction } from '@grafana/runtime';

 import { logsResourceTypes, resourceTypeDisplayNames } from '../azureMetadata';
 import AzureMonitorDatasource from '../azure_monitor/azure_monitor_datasource';
@ -18,7 +18,6 @@ import {
  AzureDataSourceJsonData,
  AzureGraphResponse,
  AzureMonitorResource,
-  AzureMonitorLocations,
  AzureMonitorQuery,
  AzureResourceGraphOptions,
  AzureResourceSummaryItem,
@ -39,8 +38,6 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit
  resultLimit = 200;
  azureMonitorDatasource;
  supportedMetricNamespaces = '';
-  locationsMap: Map<string, AzureMonitorLocations> = new Map();
-  locations: string[] = [];

  constructor(
    instanceSettings: DataSourceInstanceSettings<AzureDataSourceJsonData>,
@ -57,11 +54,6 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit
  ): Promise<ResourceRowGroup> {
    const subscriptions = await this.getSubscriptions();

-    if (this.locationsMap.size === 0) {
-      this.locationsMap = await this.getLocations(subscriptions);
-      this.locations = Array.from(this.locationsMap.values()).map((location) => `"${location.name}"`);
-    }
-
    if (!currentSelection) {
      return subscriptions;
    }
@ -140,7 +132,7 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit
        resourceGroupName: item.resourceGroup,
        type,
        typeLabel: resourceTypeDisplayNames[item.type] || item.type,
-        location: this.locationsMap.get(item.location)?.displayName || item.location,
+        location: item.location,
      };
    });
  };
@ -243,14 +235,10 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit
    resourceGroupId: string,
    type: ResourcePickerQueryType
  ): Promise<ResourceRowGroup> {
-    if (!this.locations) {
-      return [];
-    }
-
    const { data: response } = await this.makeResourceGraphRequest<RawAzureResourceItem[]>(`
      resources
      | where id hasprefix "${resourceGroupId}"
-      ${await this.filterByType(type)} and location in (${this.locations})
+      ${await this.filterByType(type)}
    `);

    return response.map((item) => {
@ -265,7 +253,7 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit
        resourceGroupName: item.resourceGroup,
        type: ResourceRowType.Resource,
        typeLabel: resourceTypeDisplayNames[item.type] || item.type,
-        locationDisplayName: this.locationsMap.get(item.location)?.displayName || item.location,
+        locationDisplayName: item.location,
        location: item.location,
      };
    });
@ -370,6 +358,7 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit

  private async fetchAllNamespaces() {
    const subscriptions = await this.getSubscriptions();
+    reportInteraction('grafana_ds_azuremonitor_subscriptions_loaded', { subscriptions: subscriptions.length });
    let supportedMetricNamespaces: string[] = [];
    for await (const subscription of subscriptions) {
      const namespaces = await this.azureMonitorDatasource.getMetricNamespaces(
@ -392,12 +381,6 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit
    this.supportedMetricNamespaces = uniq(supportedMetricNamespaces).join(',');
  }

-  async getLocations(subscriptions: ResourceRowGroup): Promise<Map<string, AzureMonitorLocations>> {
-    const subscriptionIds = subscriptions.map((sub) => sub.id);
-    const locations = await this.azureMonitorDatasource.getLocations(subscriptionIds);
-    return locations;
-  }
-
  parseRows(resources: Array<string | AzureMonitorResource>): ResourceRow[] {
    const resourceObjs = parseMultipleResourceDetails(resources);
    const newSelectedRows: ResourceRow[] = [];
@ -422,7 +405,6 @@ export default class ResourcePickerData extends DataSourceWithBackend<AzureMonit
        uri: resourceToString(resource),
        typeLabel:
          resourceTypeDisplayNames[resource.metricNamespace?.toLowerCase() ?? ''] ?? resource.metricNamespace ?? '',
-        locationDisplayName: this.locationsMap.get(resource.region ?? '')?.displayName || resource.region,
        location: resource.region,
      });
    });
--- a/public/app/plugins/datasource/azuremonitor/variables.ts
+++ b/public/app/plugins/datasource/azuremonitor/variables.ts
@ -109,17 +109,6 @@ export class VariableSupport extends CustomVariableSupport<DataSource, AzureMoni
              };
            }
            return { data: [] };
-          case AzureQueryType.LocationsQuery:
-            if (queryObj.subscription && this.hasValue(queryObj.subscription)) {
-              const locationMap = await this.datasource.azureMonitorDatasource.getLocations([queryObj.subscription]);
-              const res: Array<{ text: string; value: string }> = [];
-              locationMap.forEach((loc) => {
-                res.push({ text: loc.displayName, value: loc.name });
-              });
-              return {
-                data: res?.length ? [toDataFrame(res)] : [],
-              };
-            }
          default:
            request.targets[0] = queryObj;
            const queryResp = await lastValueFrom(this.datasource.query(request));