AzureMonitor: Improve handling of unsupported template variable cases in URIs (#52054)

* Set error message for certain template variable combinations

- Make use of setError method from query editor
- Update Azure Monitor error type
- Add test for case 2 from https://github.com/grafana/grafana/pull/51331

* Update template variable docs

* Fix lint issues

* Update docs/sources/datasources/azuremonitor/template-variables.md

Co-authored-by: Garrett Guillotte <100453168+gguillotte-grafana@users.noreply.github.com>

* PR comment updates

Co-authored-by: Garrett Guillotte <100453168+gguillotte-grafana@users.noreply.github.com>

docs/sources/datasources/azuremonitor/template-variables.md

@@ -62,3 +62,67 @@ Perf
 | summarize avg(CounterValue) by bin(TimeGenerated, $__interval), Computer
 | order by TimeGenerated asc
 ```
+
+## Limitations
+
+As of Grafana 9.0, a resource URI is constructed to identify resources using the resource picker. On dashboards created prior to Grafana 9.0, Grafana automatically migrates any queries using the prior resource-picking mechanism to use this method.
+
+Some resource types use nested namespaces and resource names, such as `Microsoft.Storage/storageAccounts/tableServices` and `storageAccount/default`, or `Microsoft.Sql/servers/databases` and `serverName/databaseName`. Such template variables cannot be used because the result could be a malformed resource URI.
+
+### Supported cases
+
+#### Standard namespaces and resource names
+
+```kusto
+metricDefinition = $ns
+$ns = Microsoft.Compute/virtualMachines
+resourceName = $rs
+$rs = testvirtualmachine
+```
+
+#### Namespaces with a non-templated sub-namespace
+
+```kusto
+metricDefinition = $ns/tableServices
+$ns = Microsoft.Storage/storageAccounts
+resourceName = $rs/default
+$rs = storageaccount
+```
+
+#### Storage namespaces missing the `default` keyword
+
+```kusto
+metricDefinition = $ns/tableServices
+$ns = Microsoft.Storage/storageAccounts
+resourceName = $rs
+$rs = storageaccount
+```
+
+#### Namespaces with a templated sub-namespace
+
+```kusto
+metricDefinition = $ns/$sns
+$ns = Microsoft.Storage/storageAccounts
+$sns = tableServices
+resourceName = $rs
+$rs = storageaccount
+```
+
+### Unsupported case
+
+If a dashboard uses this unsupported case, migrate it to one of the [supported cases](#supported-cases).
+
+If a namespace or resource name template variable contains multiple segments, Grafana will construct the resource URI incorrectly because the template variable cannot be appropriately split.
+
+For example:
+
+```kusto
+metricDefinition = $ns
+resourceName = $rs
+$ns = 'Microsoft.Storage/storageAccounts/tableServices'
+$rs = 'storageaccount/default'
+```
+
+This would result in an incorrect resource URI containing `Microsoft.Storage/storageAccounts/tableServices/storageaccount/default`. However, the correct URI would have the format `Microsoft.Storage/storageAccounts/storageaccount/tableServices/default`.
+
+An appropriate fix would be to update the template variable that does not match a supported case. If the namespace variable `$ns` is of the form `Microsoft.Storage/storageAccounts/tableServices` this could be split into two variables: `$ns1 = Microsoft.Storage/storageAccounts` and `$ns2 = tableServices`. The metric definition would then take the form `$ns1/$ns2` which leads to a correctly formatted URI.

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/QueryEditor/QueryEditor.tsx

@@ -47,7 +47,7 @@ const QueryEditor: React.FC<AzureMonitorQueryEditorProps> = ({
     [onChange, onRunQuery]
   );
 
-  const query = usePreparedQuery(baseQuery, onQueryChange);
+  const query = usePreparedQuery(baseQuery, onQueryChange, setError);
 
   const subscriptionId = query.subscription || datasource.azureMonitorDatasource.defaultSubscriptionId;
   const variableOptionGroup = {

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/QueryEditor/usePreparedQuery.ts

@@ -4,17 +4,20 @@ import { useEffect, useMemo } from 'react';
 
 import { getTemplateSrv } from '@grafana/runtime';
 
-import { AzureMonitorQuery, AzureQueryType } from '../../types';
+import { AzureMonitorErrorish, AzureMonitorQuery, AzureQueryType } from '../../types';
 import migrateQuery from '../../utils/migrateQuery';
 
 const DEFAULT_QUERY = {
   queryType: AzureQueryType.AzureMonitor,
 };
 
-const prepareQuery = (query: AzureMonitorQuery) => {
+const prepareQuery = (
+  query: AzureMonitorQuery,
+  setError: (errorSource: string, error: AzureMonitorErrorish) => void
+) => {
   // Note: _.defaults does not apply default values deeply.
   const withDefaults = defaults({}, query, DEFAULT_QUERY);
-  const migratedQuery = migrateQuery(withDefaults, getTemplateSrv());
+  const migratedQuery = migrateQuery(withDefaults, getTemplateSrv(), setError);
 
   // If we didn't make any changes to the object, then return the original object to keep the
   // identity the same, and not trigger any other useEffects or anything.
@@ -24,8 +27,12 @@ const prepareQuery = (query: AzureMonitorQuery) => {
 /**
  * Returns queries with some defaults + migrations, and calls onChange function to notify if it changes
  */
-const usePreparedQuery = (query: AzureMonitorQuery, onChangeQuery: (newQuery: AzureMonitorQuery) => void) => {
+const usePreparedQuery = (
-  const preparedQuery = useMemo(() => prepareQuery(query), [query]);
+  query: AzureMonitorQuery,
+  onChangeQuery: (newQuery: AzureMonitorQuery) => void,
+  setError: (errorSource: string, error: AzureMonitorErrorish) => void
+) => {
+  const preparedQuery = useMemo(() => prepareQuery(query, setError), [query, setError]);
 
   useEffect(() => {
     if (preparedQuery !== query) {

public/app/plugins/datasource/grafana-azure-monitor-datasource/types/types.ts

@@ -85,7 +85,7 @@ export interface AzureDataSourceSecureJsonData {
 
 // Represents an errors that come back from frontend requests.
 // Not totally sure how accurate this type is.
-export type AzureMonitorErrorish = Error;
+export type AzureMonitorErrorish = Error | string | React.ReactElement;
 
 // Azure Monitor API Types
 export interface AzureMonitorMetricsMetadataResponse {

public/app/plugins/datasource/grafana-azure-monitor-datasource/utils/messageFromError.ts

@@ -1,3 +1,15 @@
+import { isValidElement } from 'react';
+
+import { AzureMonitorErrorish } from '../types';
+
+export function messageFromElement(error: AzureMonitorErrorish): AzureMonitorErrorish | undefined {
+  if (isValidElement(error)) {
+    return error;
+  } else {
+    return messageFromError(error);
+  }
+}
+
 export default function messageFromError(error: any): string | undefined {
   if (!error || typeof error !== 'object') {
     return undefined;

public/app/plugins/datasource/grafana-azure-monitor-datasource/utils/migrateQuery.test.ts

@@ -1,6 +1,8 @@
+import React from 'react';
+
 import { getTemplateSrv } from '@grafana/runtime';
 
-import { AzureMetricDimension, AzureMonitorQuery, AzureQueryType } from '../types';
+import { AzureMetricDimension, AzureMonitorErrorish, AzureMonitorQuery, AzureQueryType } from '../types';
 
 import migrateQuery from './migrateQuery';
 
@@ -17,6 +19,8 @@ jest.mock('@grafana/runtime', () => {
 
 let templateSrv = getTemplateSrv();
 
+let setErrorMock = jest.fn();
+
 const azureMonitorQueryV7 = {
   appInsights: { dimension: [], metricName: 'select', timeGrain: 'auto' },
   azureLogAnalytics: {
@@ -97,7 +101,7 @@ const modernMetricsQuery: AzureMonitorQuery = {
 
 describe('AzureMonitor: migrateQuery', () => {
   it('modern queries should not change', () => {
-    const result = migrateQuery(modernMetricsQuery, templateSrv);
+    const result = migrateQuery(modernMetricsQuery, templateSrv, setErrorMock);
 
     // MUST use .toBe because we want to assert that the identity of unmigrated queries remains the same
     expect(modernMetricsQuery).toBe(result);
@@ -105,7 +109,7 @@ describe('AzureMonitor: migrateQuery', () => {
 
   describe('migrating from a v7 query to the latest query version', () => {
     it('should build a resource uri', () => {
-      const result = migrateQuery(azureMonitorQueryV7, templateSrv);
+      const result = migrateQuery(azureMonitorQueryV7, templateSrv, setErrorMock);
       expect(result).toMatchObject(
         expect.objectContaining({
           azureMonitor: expect.objectContaining({
@@ -119,7 +123,7 @@ describe('AzureMonitor: migrateQuery', () => {
 
   describe('migrating from a v8 query to the latest query version', () => {
     it('should build a resource uri', () => {
-      const result = migrateQuery(azureMonitorQueryV8, templateSrv);
+      const result = migrateQuery(azureMonitorQueryV8, templateSrv, setErrorMock);
       expect(result).toMatchObject(
         expect.objectContaining({
           azureMonitor: expect.objectContaining({
@@ -130,18 +134,66 @@ describe('AzureMonitor: migrateQuery', () => {
       );
     });
 
-    it('should not build a resource uri with an unsupported template variable', () => {
+    it('should not build a resource uri with an unsupported namespace template variable', () => {
-      replaceMock = jest.fn().mockImplementation((s: string) => s.replace('$ns', 'Microsoft.Storage/storageAccounts'));
+      replaceMock = jest
+        .fn()
+        .mockImplementation((s: string) => s.replace('$ns', 'Microsoft.Storage/storageAccounts/tableServices'));
+      setErrorMock = jest
+        .fn()
+        .mockImplementation((errorSource: string, error: AzureMonitorErrorish) => 'Template Var error');
+      const errorElement = React.createElement(
+        'div',
+        null,
+        `Failed to create resource URI. Validate the metric definition template variable against supported cases `,
+        React.createElement(
+          'a',
+          {
+            href: 'https://grafana.com/docs/grafana/latest/datasources/azuremonitor/template-variables/',
+          },
+          'here.'
+        )
+      );
       templateSrv = getTemplateSrv();
       const query = {
         ...azureMonitorQueryV8,
         azureMonitor: {
-          ...azureMonitorQueryV8,
+          ...azureMonitorQueryV8.azureMonitor,
           metricDefinition: '$ns',
         },
       };
-      const result = migrateQuery(query, templateSrv);
+      const result = migrateQuery(query, templateSrv, setErrorMock);
       expect(result.azureMonitor?.resourceUri).toBeUndefined();
+      expect(setErrorMock).toHaveBeenCalledWith('Resource URI migration', errorElement);
+    });
+
+    it('should not build a resource uri with unsupported resource name template variable', () => {
+      replaceMock = jest.fn().mockImplementation((s: string) => s.replace('$resource', 'resource/default'));
+      setErrorMock = jest
+        .fn()
+        .mockImplementation((errorSource: string, error: AzureMonitorErrorish) => 'Template Var error');
+      const errorElement = React.createElement(
+        'div',
+        null,
+        `Failed to create resource URI. Validate the resource name template variable against supported cases `,
+        React.createElement(
+          'a',
+          {
+            href: 'https://grafana.com/docs/grafana/latest/datasources/azuremonitor/template-variables/',
+          },
+          'here.'
+        )
+      );
+      templateSrv = getTemplateSrv();
+      const query = {
+        ...azureMonitorQueryV8,
+        azureMonitor: {
+          ...azureMonitorQueryV8.azureMonitor,
+          resourceName: '$resource',
+        },
+      };
+      const result = migrateQuery(query, templateSrv, setErrorMock);
+      expect(result.azureMonitor?.resourceUri).toBeUndefined();
+      expect(setErrorMock).toHaveBeenCalledWith('Resource URI migration', errorElement);
     });
   });
 
@@ -150,7 +202,11 @@ describe('AzureMonitor: migrateQuery', () => {
       const dimensionFilters: AzureMetricDimension[] = [
         { dimension: 'TestDimension', operator: 'eq', filters: ['testFilter'] },
       ];
-      const result = migrateQuery({ ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } }, templateSrv);
+      const result = migrateQuery(
+        { ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } },
+        templateSrv,
+        setErrorMock
+      );
       expect(result).toMatchObject(
         expect.objectContaining({
           azureMonitor: expect.objectContaining({
@@ -161,7 +217,11 @@ describe('AzureMonitor: migrateQuery', () => {
     });
     it('correctly updates old filter containing wildcard', () => {
       const dimensionFilters: AzureMetricDimension[] = [{ dimension: 'TestDimension', operator: 'eq', filter: '*' }];
-      const result = migrateQuery({ ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } }, templateSrv);
+      const result = migrateQuery(
+        { ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } },
+        templateSrv,
+        setErrorMock
+      );
       expect(result).toMatchObject(
         expect.objectContaining({
           azureMonitor: expect.objectContaining({
@@ -174,7 +234,11 @@ describe('AzureMonitor: migrateQuery', () => {
     });
     it('correctly updates old filter containing value', () => {
       const dimensionFilters: AzureMetricDimension[] = [{ dimension: 'TestDimension', operator: 'eq', filter: 'test' }];
-      const result = migrateQuery({ ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } }, templateSrv);
+      const result = migrateQuery(
+        { ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } },
+        templateSrv,
+        setErrorMock
+      );
       expect(result).toMatchObject(
         expect.objectContaining({
           azureMonitor: expect.objectContaining({
@@ -189,7 +253,11 @@ describe('AzureMonitor: migrateQuery', () => {
       const dimensionFilters: AzureMetricDimension[] = [
         { dimension: 'TestDimension', operator: 'eq', filter: '*', filters: ['testFilter'] },
       ];
-      const result = migrateQuery({ ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } }, templateSrv);
+      const result = migrateQuery(
+        { ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } },
+        templateSrv,
+        setErrorMock
+      );
       expect(result).toMatchObject(
         expect.objectContaining({
           azureMonitor: expect.objectContaining({
@@ -208,7 +276,11 @@ describe('AzureMonitor: migrateQuery', () => {
       const dimensionFilters: AzureMetricDimension[] = [
         { dimension: 'TestDimension', operator: 'eq', filter: 'testFilter', filters: ['testFilter'] },
       ];
-      const result = migrateQuery({ ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } }, templateSrv);
+      const result = migrateQuery(
+        { ...azureMonitorQueryV8, azureMonitor: { dimensionFilters } },
+        templateSrv,
+        setErrorMock
+      );
       expect(result).toMatchObject(
         expect.objectContaining({
           azureMonitor: expect.objectContaining({

public/app/plugins/datasource/grafana-azure-monitor-datasource/utils/migrateQuery.ts

@@ -1,3 +1,5 @@
+import React from 'react';
+
 import { TemplateSrv } from '@grafana/runtime';
 
 import UrlBuilder from '../azure_monitor/url_builder';
@@ -7,11 +9,15 @@ import {
   setTimeGrain as setMetricsTimeGrain,
 } from '../components/MetricsQueryEditor/setQueryValue';
 import TimegrainConverter from '../time_grain_converter';
-import { AzureMetricDimension, AzureMonitorQuery, AzureQueryType } from '../types';
+import { AzureMetricDimension, AzureMonitorErrorish, AzureMonitorQuery, AzureQueryType } from '../types';
 
 const OLD_DEFAULT_DROPDOWN_VALUE = 'select';
 
-export default function migrateQuery(query: AzureMonitorQuery, templateSrv: TemplateSrv): AzureMonitorQuery {
+export default function migrateQuery(
+  query: AzureMonitorQuery,
+  templateSrv: TemplateSrv,
+  setError: (errorSource: string, error: AzureMonitorErrorish) => void
+): AzureMonitorQuery {
   let workingQuery = query;
 
   // The old angular controller also had a `migrateApplicationInsightsKeys` migraiton that
@@ -23,7 +29,7 @@ export default function migrateQuery(query: AzureMonitorQuery, templateSrv: Temp
   workingQuery = migrateLogAnalyticsToFromTimes(workingQuery);
   workingQuery = migrateToDefaultNamespace(workingQuery);
   workingQuery = migrateDimensionToDimensionFilter(workingQuery);
-  workingQuery = migrateResourceUri(workingQuery, templateSrv);
+  workingQuery = migrateResourceUri(workingQuery, templateSrv, setError);
   workingQuery = migrateDimensionFilterToArray(workingQuery);
 
   return workingQuery;
@@ -98,7 +104,11 @@ function migrateDimensionToDimensionFilter(query: AzureMonitorQuery): AzureMonit
 // Azure Monitor metric queries prior to Grafana version 9 did not include a `resourceUri`.
 // The resourceUri was previously constructed with the subscription id, resource group,
 // metric definition (a.k.a. resource type), and the resource name.
-function migrateResourceUri(query: AzureMonitorQuery, templateSrv: TemplateSrv): AzureMonitorQuery {
+function migrateResourceUri(
+  query: AzureMonitorQuery,
+  templateSrv: TemplateSrv,
+  setError?: (errorSource: string, error: AzureMonitorErrorish) => void
+): AzureMonitorQuery {
   const azureMonitorQuery = query.azureMonitor;
 
   if (!azureMonitorQuery || azureMonitorQuery.resourceUri) {
@@ -116,6 +126,23 @@ function migrateResourceUri(query: AzureMonitorQuery, templateSrv: TemplateSrv):
     // If a metric definition includes template variable with a subresource e.g.
     // Microsoft.Storage/storageAccounts/libraries, it's not possible to generate a valid
     // resource URI
+    if (setError) {
+      setError(
+        'Resource URI migration',
+        React.createElement(
+          'div',
+          null,
+          `Failed to create resource URI. Validate the metric definition template variable against supported cases `,
+          React.createElement(
+            'a',
+            {
+              href: 'https://grafana.com/docs/grafana/latest/datasources/azuremonitor/template-variables/',
+            },
+            'here.'
+          )
+        )
+      );
+    }
     return query;
   }
 
@@ -123,6 +150,23 @@ function migrateResourceUri(query: AzureMonitorQuery, templateSrv: TemplateSrv):
   if (resourceNameArray.some((p) => templateSrv.replace(p).split('/').length > 1)) {
     // If a resource name includes template variable with a subresource e.g.
     // abc123/def456, it's not possible to generate a valid resource URI
+    if (setError) {
+      setError(
+        'Resource URI migration',
+        React.createElement(
+          'div',
+          null,
+          `Failed to create resource URI. Validate the resource name template variable against supported cases `,
+          React.createElement(
+            'a',
+            {
+              href: 'https://grafana.com/docs/grafana/latest/datasources/azuremonitor/template-variables/',
+            },
+            'here.'
+          )
+        )
+      );
+    }
     return query;
   }
 

public/app/plugins/datasource/grafana-azure-monitor-datasource/utils/useLastError.ts

@@ -2,7 +2,7 @@ import { useState, useCallback, useMemo } from 'react';
 
 import { AzureMonitorErrorish } from '../types';
 
-import messageFromError from './messageFromError';
+import { messageFromElement } from './messageFromError';
 
 type SourcedError = [string, AzureMonitorErrorish];
 
@@ -33,7 +33,7 @@ export default function useLastError() {
 
   const errorMessage = useMemo(() => {
     const recentError = errors[0];
-    return recentError && messageFromError(recentError[1]);
+    return recentError && messageFromElement(recentError[1]);
   }, [errors]);
 
   return [errorMessage, addError] as const;