From 8808de0b45ade393ce8e92afe9220bd24e146071 Mon Sep 17 00:00:00 2001 From: Andreas Christou Date: Thu, 9 Nov 2023 21:13:07 +0000 Subject: [PATCH] AzureMonitor: Improve error handling and update tests (#77967) * Improve error handling and update tests * Assert errors are nil * Refactor away redundant code --- .../metrics/azuremonitor-datasource.go | 15 +- pkg/tsdb/azuremonitor/metrics/url-builder.go | 13 -- .../azuremonitor/metrics/url-builder_test.go | 181 +++++++++--------- 3 files changed, 103 insertions(+), 106 deletions(-) diff --git a/pkg/tsdb/azuremonitor/metrics/azuremonitor-datasource.go b/pkg/tsdb/azuremonitor/metrics/azuremonitor-datasource.go index 86d6aa23e2f..a58e3d4abb9 100644 --- a/pkg/tsdb/azuremonitor/metrics/azuremonitor-datasource.go +++ b/pkg/tsdb/azuremonitor/metrics/azuremonitor-datasource.go @@ -109,14 +109,17 @@ func (e *AzureMonitorDatasource) buildQueries(queries []backend.DataQuery, dsInf MetricNamespace: azJSONModel.MetricNamespace, ResourceName: resourceName, } - azureURL = ub.BuildMetricsURL() - // POST requests are only supported at the subscription level - filterInBody = false + + // Construct the resourceURI (for legacy query objects pre Grafana 9) resourceUri, err := ub.buildResourceURI() if err != nil { return nil, err } + + // POST requests are only supported at the subscription level + filterInBody = false if resourceUri != nil { + azureURL = fmt.Sprintf("%s/providers/microsoft.insights/metrics", *resourceUri) resourceMap[*resourceUri] = dataquery.AzureMonitorResource{ResourceGroup: resourceGroup, ResourceName: resourceName} } } else { @@ -128,7 +131,11 @@ func (e *AzureMonitorDatasource) buildQueries(queries []backend.DataQuery, dsInf MetricNamespace: azJSONModel.MetricNamespace, ResourceName: r.ResourceName, } - resourceUri, _ := ub.buildResourceURI() + resourceUri, err := ub.buildResourceURI() + if err != nil { + return nil, err + } + if resourceUri != nil { resourceMap[*resourceUri] = r } diff --git a/pkg/tsdb/azuremonitor/metrics/url-builder.go b/pkg/tsdb/azuremonitor/metrics/url-builder.go index 5785b94f976..1d1b015ed78 100644 --- a/pkg/tsdb/azuremonitor/metrics/url-builder.go +++ b/pkg/tsdb/azuremonitor/metrics/url-builder.go @@ -73,19 +73,6 @@ func (params *urlBuilder) buildResourceURI() (*string, error) { return &resourceURI, nil } -// BuildMetricsURL checks the metric properties to see which form of the url -// should be returned -func (params *urlBuilder) BuildMetricsURL() string { - resourceURI := params.ResourceURI - - // Prior to Grafana 9, we had a legacy query object rather than a resourceURI, so we manually create the resource URI - if resourceURI == nil || *resourceURI == "" { - resourceURI, _ = params.buildResourceURI() - } - - return fmt.Sprintf("%s/providers/microsoft.insights/metrics", *resourceURI) -} - // BuildSubscriptionMetricsURL returns a URL for querying metrics for all resources in a subscription // It requires to set a $filter and a region parameter func BuildSubscriptionMetricsURL(subscription string) string { diff --git a/pkg/tsdb/azuremonitor/metrics/url-builder_test.go b/pkg/tsdb/azuremonitor/metrics/url-builder_test.go index 09e9a3a9b71..2c06308d9bd 100644 --- a/pkg/tsdb/azuremonitor/metrics/url-builder_test.go +++ b/pkg/tsdb/azuremonitor/metrics/url-builder_test.go @@ -6,95 +6,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestURLBuilder(t *testing.T) { - t.Run("AzureMonitor URL Builder", func(t *testing.T) { - t.Run("when only resource uri is provided it returns resource/uri/providers/microsoft.insights/metrics", func(t *testing.T) { - ub := &urlBuilder{ - ResourceURI: strPtr("/subscriptions/sub/resource/uri"), - } - - url := ub.BuildMetricsURL() - assert.Equal(t, "/subscriptions/sub/resource/uri/providers/microsoft.insights/metrics", url) - }) - - t.Run("when resource uri and legacy fields are provided the legacy fields are ignored", func(t *testing.T) { - ub := &urlBuilder{ - ResourceURI: strPtr("/subscriptions/sub/resource/uri"), - DefaultSubscription: strPtr("default-sub"), - ResourceGroup: strPtr("rg"), - MetricNamespace: strPtr("Microsoft.NetApp/netAppAccounts/capacityPools/volumes"), - ResourceName: strPtr("rn1/rn2/rn3"), - } - - url := ub.BuildMetricsURL() - assert.Equal(t, "/subscriptions/sub/resource/uri/providers/microsoft.insights/metrics", url) - }) - - t.Run("Legacy URL Builder params", func(t *testing.T) { - t.Run("when metric definition is in the short form", func(t *testing.T) { - ub := &urlBuilder{ - DefaultSubscription: strPtr("default-sub"), - ResourceGroup: strPtr("rg"), - MetricNamespace: strPtr("Microsoft.Compute/virtualMachines"), - ResourceName: strPtr("rn"), - } - - url := ub.BuildMetricsURL() - assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/rn/providers/microsoft.insights/metrics", url) - }) - - t.Run("when metric definition is in the short form and a subscription is defined", func(t *testing.T) { - ub := &urlBuilder{ - DefaultSubscription: strPtr("default-sub"), - Subscription: strPtr("specified-sub"), - ResourceGroup: strPtr("rg"), - MetricNamespace: strPtr("Microsoft.Compute/virtualMachines"), - ResourceName: strPtr("rn"), - } - - url := ub.BuildMetricsURL() - assert.Equal(t, "/subscriptions/specified-sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/rn/providers/microsoft.insights/metrics", url) - }) - - t.Run("when metric definition is Microsoft.Storage/storageAccounts/blobServices", func(t *testing.T) { - ub := &urlBuilder{ - DefaultSubscription: strPtr("default-sub"), - ResourceGroup: strPtr("rg"), - MetricNamespace: strPtr("Microsoft.Storage/storageAccounts/blobServices"), - ResourceName: strPtr("rn1/default"), - } - - url := ub.BuildMetricsURL() - assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Storage/storageAccounts/rn1/blobServices/default/providers/microsoft.insights/metrics", url) - }) - - t.Run("when metric definition is Microsoft.Storage/storageAccounts/fileServices", func(t *testing.T) { - ub := &urlBuilder{ - DefaultSubscription: strPtr("default-sub"), - ResourceGroup: strPtr("rg"), - MetricNamespace: strPtr("Microsoft.Storage/storageAccounts/fileServices"), - ResourceName: strPtr("rn1/default"), - } - - url := ub.BuildMetricsURL() - assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Storage/storageAccounts/rn1/fileServices/default/providers/microsoft.insights/metrics", url) - }) - - t.Run("when metric definition is Microsoft.NetApp/netAppAccounts/capacityPools/volumes", func(t *testing.T) { - ub := &urlBuilder{ - DefaultSubscription: strPtr("default-sub"), - ResourceGroup: strPtr("rg"), - MetricNamespace: strPtr("Microsoft.NetApp/netAppAccounts/capacityPools/volumes"), - ResourceName: strPtr("rn1/rn2/rn3"), - } - - url := ub.BuildMetricsURL() - assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.NetApp/netAppAccounts/rn1/capacityPools/rn2/volumes/rn3/providers/microsoft.insights/metrics", url) - }) - }) - }) -} - func TestBuildResourceURI(t *testing.T) { t.Run("AzureMonitor Resource URI Builder", func(t *testing.T) { t.Run("when there is no resource uri", func(t *testing.T) { @@ -113,6 +24,59 @@ func TestBuildResourceURI(t *testing.T) { assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Web/serverFarms/rn1", url) }) + t.Run("when only resource uri is provided it returns the resource URI", func(t *testing.T) { + ub := &urlBuilder{ + ResourceURI: strPtr("/subscriptions/sub/resource/uri"), + } + + url, err := ub.buildResourceURI() + assert.Nil(t, err) + assert.Equal(t, "/subscriptions/sub/resource/uri", *url) + }) + + t.Run("when resource uri and legacy fields are provided the legacy fields are ignored", func(t *testing.T) { + ub := &urlBuilder{ + ResourceURI: strPtr("/subscriptions/sub/resource/uri"), + DefaultSubscription: strPtr("default-sub"), + ResourceGroup: strPtr("rg"), + MetricNamespace: strPtr("Microsoft.NetApp/netAppAccounts/capacityPools/volumes"), + ResourceName: strPtr("rn1/rn2/rn3"), + } + + url, err := ub.buildResourceURI() + assert.Nil(t, err) + assert.Equal(t, "/subscriptions/sub/resource/uri", *url) + }) + + t.Run("Legacy URL Builder params", func(t *testing.T) { + t.Run("when metric definition is in the short form", func(t *testing.T) { + ub := &urlBuilder{ + DefaultSubscription: strPtr("default-sub"), + ResourceGroup: strPtr("rg"), + MetricNamespace: strPtr("Microsoft.Compute/virtualMachines"), + ResourceName: strPtr("rn"), + } + + url, err := ub.buildResourceURI() + assert.Nil(t, err) + assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/rn", *url) + }) + }) + + t.Run("when metric definition is in the short form and a subscription is defined", func(t *testing.T) { + ub := &urlBuilder{ + DefaultSubscription: strPtr("default-sub"), + Subscription: strPtr("specified-sub"), + ResourceGroup: strPtr("rg"), + MetricNamespace: strPtr("Microsoft.Compute/virtualMachines"), + ResourceName: strPtr("rn"), + } + + url, err := ub.buildResourceURI() + assert.Nil(t, err) + assert.Equal(t, "/subscriptions/specified-sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/rn", *url) + }) + t.Run("when metric definition is Microsoft.Storage/storageAccounts/blobServices", func(t *testing.T) { ub := &urlBuilder{ DefaultSubscription: strPtr("default-sub"), @@ -129,6 +93,45 @@ func TestBuildResourceURI(t *testing.T) { assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Storage/storageAccounts/rn1/blobServices/default", url) }) + t.Run("when metric definition is Microsoft.Storage/storageAccounts/tableServices", func(t *testing.T) { + ub := &urlBuilder{ + DefaultSubscription: strPtr("default-sub"), + ResourceGroup: strPtr("rg"), + MetricNamespace: strPtr("Microsoft.Storage/storageAccounts/tableServices"), + ResourceName: strPtr("rn1/default"), + } + + url, err := ub.buildResourceURI() + assert.Nil(t, err) + assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Storage/storageAccounts/rn1/tableServices/default", *url) + }) + + t.Run("when metric definition is Microsoft.Storage/storageAccounts/fileServices", func(t *testing.T) { + ub := &urlBuilder{ + DefaultSubscription: strPtr("default-sub"), + ResourceGroup: strPtr("rg"), + MetricNamespace: strPtr("Microsoft.Storage/storageAccounts/fileServices"), + ResourceName: strPtr("rn1/default"), + } + + url, err := ub.buildResourceURI() + assert.Nil(t, err) + assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.Storage/storageAccounts/rn1/fileServices/default", *url) + }) + + t.Run("when metric definition is Microsoft.NetApp/netAppAccounts/capacityPools/volumes", func(t *testing.T) { + ub := &urlBuilder{ + DefaultSubscription: strPtr("default-sub"), + ResourceGroup: strPtr("rg"), + MetricNamespace: strPtr("Microsoft.NetApp/netAppAccounts/capacityPools/volumes"), + ResourceName: strPtr("rn1/rn2/rn3"), + } + + url, err := ub.buildResourceURI() + assert.Nil(t, err) + assert.Equal(t, "/subscriptions/default-sub/resourceGroups/rg/providers/Microsoft.NetApp/netAppAccounts/rn1/capacityPools/rn2/volumes/rn3", *url) + }) + t.Run("when metricDefinition or metricNamespace is not defined an error is thrown", func(t *testing.T) { ub := &urlBuilder{}