AzureMonitor: Ensure resourceURI template variable is migrated #56095

aangelisc · 2022-09-30T13:40:21Z

Related to grafana/support-escalations#4104. #52897 removed a check on the existence of the resourceUri prop. This then leads to a query containing a resource URI that is a template variable being filtered out.

This PR readds the check on resourceUri to ensure queries will get migrated.

- Do not filter queries containing a resource URI template - Update migration - Add test

github-actions · 2022-09-30T13:51:01Z

Backend code coverage report for PR #56095
No changes

github-actions · 2022-09-30T13:51:02Z

Frontend code coverage report for PR #56095

Plugin	Main	PR	Difference
grafana-azure-monitor-datasource	76.27%	76.27%	0%

andresmgot

LGTM! Just one question here.

BTW, it may be worth it to also backport this to 9.1.x (just in case the user cannot upgrade to the new minor anytime soon)

andresmgot · 2022-09-30T15:32:58Z

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

@@ -146,7 +146,7 @@ function migrateDimensionToResourceObj(query: AzureMonitorQuery): AzureMonitorQu
        resourceGroup: details?.resourceGroup,
        metricNamespace: details?.metricNamespace,
        resourceName: details?.resourceName,
-        resourceUri: undefined,
+        resourceUri: details.subscription ? undefined : query.azureMonitor.resourceUri,


🤔 why does this depend on having a subscription if the resourceUri already includes it?

I added this to maintain the behaviour of setting the resourceUri to undefined. I'm assuming (it may be a poor assumption) that if the subscription value is set then we can unset the resourceUri, otherwise we maintain the value so that the migration can be carried out later when applyTemplateVariables is called.

The current behaviour of resourceUri: undefined means that we remove the template variable value before it is actually applied.

I would not remove it if it's defined: If it's defined, it should take precedence over the rest of params (subs, group, name...) and the backend will skip the URI generation.

Does this not go against our deprecation of resourceUri?

You are right, we need to clean up the resourceUri value, since if not, the UI, showing a subs + group + name, won't reflect what is actually being used in the backend (the resource URI). BTW, that also means that even after this PR, the client won't be able to see the selection in the resource picker (even though the query will be executed).

I was thinking about our conversation in #55372. If we have a template variable with multiple resources from different resource groups, the more straight forward way to support that is to have a list of resource IDs so supporting both formats sounds like something we will need to do. In any case, it's not something we are prepared to do at the moment.

Going to the original comment here, the condition is probably better to also take into account the resource group and name, since sometimes the subscription is automatically added using the default value from the data source configuration.

Suggested change

resourceUri: details.subscription ? undefined : query.azureMonitor.resourceUri,

resourceUri: (details.subscription && details?.resourceGroup && details?.resourceName) ? undefined : query.azureMonitor.resourceUri,

yaelleC · 2022-10-03T12:20:14Z

...s/datasource/grafana-azure-monitor-datasource/azure_monitor/azure_monitor_datasource.test.ts

+        },
+      });
+      const templatedQuery = ctx.ds.azureMonitorDatasource.applyTemplateVariables(query, {});
+      expect(templatedQuery).toMatchObject({


Should we have a test for not undefined resourceUri ?

Do you mean a test for the case where resourceUri is entirely undefined or a test for the case where we return the resourceUri because it could not be parsed?

yaelleC · 2022-10-03T12:25:08Z

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

+        resourceUri:
+          details?.subscription && details?.resourceGroup && details?.resourceName
+            ? undefined
+            : query.azureMonitor.resourceUri,


if we leave the resourceUri , does it mean the query will be migrated again next time? As query.azureMonitor?.resourceUri will be true?
Maybe I'm misunderstanding what you're trying to achieve here 😅

If the resourceUri is a template variable then we'd require the query to be migrated every time to account for any changes to the variable 😊

The goal of this PR is to ensure that if a template variable is used for the resourceUri (as is the case for the support escalation) then it should be appropriately migrated to the relevant components.

well, we don't need to migrate it in the case it's a template variable, since it will never be parsed. Maybe you can update the condition to something like:

if (query.azureMonitor?.resourceUri && !query.azureMonitor.resourceUri.startsWith('$')) {

To be clear that template variables won't be migrated (but the result will be the same than with this code).

What would be nice, is to modify the "Advance" section of the resource picker to show the resource URI (in this case, the template variable) rather than the empty fields that they are going to see for the Subscription + Resource Group + Resource Name. But you can do that in a separate PR

Gotcha! Thanks, I think Andres' suggestion makes sense, maybe we can add some comment in the code for the next person that is trying to make sense of it?

// migrating if the query is using the deprecated `resourceURI` prop and it's not a template variable

Could we always set the uri as undefined if we change the condition then?
If not, could we maybe extract details?.subscription && details?.resourceGroup && details?.resourceName and give it a name that would help us understand what we are trying to achieve, such as isWellFormedURI or isNotTemplateVariableURI?

I think it's helpful to not set always the uri as undefined, to cover corner cases, like template variables covering weird parts of the resource URI (e.g. /subscriptions/$restOfTheThing). +1 to define the condition as a variable.

I've made the suggested changes and will set this to auto-merge 😊 Thanks!

yaelleC

LGTM thanks for the changes 👍

andresmgot

👍

grafanabot · 2022-10-04T08:40:52Z

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/36009

* Ensure resourceURI template vars are migrated - Do not filter queries containing a resource URI template - Update migration - Add test * Update condition * Review (cherry picked from commit 462ca50)

… (#56249) * Ensure resourceURI template vars are migrated - Do not filter queries containing a resource URI template - Update migration - Add test * Update condition * Review (cherry picked from commit 462ca50) Co-authored-by: Andreas Christou <andreas.christou@grafana.com>

… (#56250) * Ensure resourceURI template vars are migrated - Do not filter queries containing a resource URI template - Update migration - Add test * Update condition * Review (cherry picked from commit 462ca50) Co-authored-by: Andreas Christou <andreas.christou@grafana.com>

Ensure resourceURI template vars are migrated

15c57b4

- Do not filter queries containing a resource URI template - Update migration - Add test

aangelisc added type/bug effort/small datasource/Azure Azure Monitor Datasource add to changelog backport v9.2.x Mark PR for automatic backport to v9.2.x labels Sep 30, 2022

aangelisc added this to the 9.2.0 milestone Sep 30, 2022

aangelisc self-assigned this Sep 30, 2022

aangelisc requested a review from a team as a code owner September 30, 2022 13:40

aangelisc requested review from andresmgot and yaelleC and removed request for a team September 30, 2022 13:40

grafanabot added the area/frontend label Sep 30, 2022

grafanabot added the enterprise-ok label Sep 30, 2022

andresmgot approved these changes Sep 30, 2022

View reviewed changes

aangelisc added the backport v9.1.x Bot will automatically open backport PR label Sep 30, 2022

Update condition

f27a64c

yaelleC reviewed Oct 3, 2022

View reviewed changes

Review

e1c6910

aangelisc modified the milestones: 9.2.0, 9.1.7 Oct 4, 2022

aangelisc enabled auto-merge (squash) October 4, 2022 08:23

yaelleC approved these changes Oct 4, 2022

View reviewed changes

Merge branch 'main' into 4104-azure-monitor-update-migration

e518957

andresmgot approved these changes Oct 4, 2022

View reviewed changes

grafanabot added enterprise-failed and removed enterprise-ok labels Oct 4, 2022

aangelisc merged commit 462ca50 into main Oct 4, 2022

aangelisc deleted the 4104-azure-monitor-update-migration branch October 4, 2022 08:44

grafanabot mentioned this pull request Oct 4, 2022

[v9.1.x] AzureMonitor: Ensure resourceURI template variable is migrated #56249

Merged

grafanabot mentioned this pull request Oct 4, 2022

[v9.2.x] AzureMonitor: Ensure resourceURI template variable is migrated #56250

Merged

grafanabot added enterprise-ok and removed enterprise-failed labels Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AzureMonitor: Ensure resourceURI template variable is migrated #56095

AzureMonitor: Ensure resourceURI template variable is migrated #56095

aangelisc commented Sep 30, 2022

github-actions bot commented Sep 30, 2022

github-actions bot commented Sep 30, 2022

andresmgot left a comment

andresmgot Sep 30, 2022

aangelisc Sep 30, 2022

andresmgot Oct 3, 2022

aangelisc Oct 3, 2022

andresmgot Oct 3, 2022 •

edited

yaelleC Oct 3, 2022

aangelisc Oct 3, 2022

yaelleC Oct 3, 2022

aangelisc Oct 3, 2022

andresmgot Oct 4, 2022

yaelleC Oct 4, 2022

andresmgot Oct 4, 2022

aangelisc Oct 4, 2022

yaelleC left a comment

andresmgot left a comment

grafanabot commented Oct 4, 2022

	resourceUri: details.subscription ? undefined : query.azureMonitor.resourceUri,
	resourceUri: (details.subscription && details?.resourceGroup && details?.resourceName) ? undefined : query.azureMonitor.resourceUri,

AzureMonitor: Ensure resourceURI template variable is migrated #56095

AzureMonitor: Ensure resourceURI template variable is migrated #56095

Conversation

aangelisc commented Sep 30, 2022

github-actions bot commented Sep 30, 2022

github-actions bot commented Sep 30, 2022

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Oct 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaelleC left a comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

grafanabot commented Oct 4, 2022

andresmgot Oct 3, 2022 •

edited