Azure Monitor: Fix migration issue with MetricDefinitionsQuery template variable query types #55262

yaelleC · 2022-09-15T18:20:50Z

What this PR does / why we need it:
The new migration does not take into account MetricDefinitionsQuery type and if a template variable had that kind property, clicking on it in the template variables view triggers an infinite loop and the browser crashes.

This PR adds back the type thus fixing the issue

As MetricDefinitionQuery was removed previously by us, I also updated the imported dashboards to reflect that change

Which issue(s) this PR fixes:

Fixes escalation #3850

Special notes for your reviewer:

yaelleC · 2022-09-15T18:21:46Z

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

@@ -295,6 +298,7 @@ const migrateGrafanaTemplateVariableFn = (query: AzureMonitorQuery) => {
      break;
  }

+  delete migratedQuery.grafanaTemplateVariableFn;


not sure if this was left on purpose (maybe to track migrations or roll back?)

If no reason then I think removing it makes the panel JSON cleaner

this is also a safety measure, if for some reason the migration is not correct, if the old information is there it could be run again. In any case, I think it's safe to remove it in this case.

Removing it breaks almost every test in variables.test.ts with timeout errors 🤔 That's why I added it back, do you have any idea why that is?

those tests use this method:

observables.subscribe((result: DataQueryResponseData) => { expect(result.data[0].source).toEqual(expectedResults); done(); });

if the expect fails, done is never called so the test hangs until reaching the timeout. If you scroll, you should be able to see the real error.

Yes, I get a Cannot read properties of undefined (reading 'source') meaning result.data[0] is undefined there.
I tried logging the result but got nothing back 🤔

I don't see how that could relate to removing the grafanaTemplateVariableFn property 🤷‍♀️ .

Should I merge without the delete for a quick fix? Or you think it could mean there is another bug hidden somewhere that the delete surfaces?

I haven't checked the code but that probably means that grafanaTemplateVariableFn is used elsewhere (either in functional code or in the test). But since there is no need to remove the property I would merge this as it is.

grafanabot · 2022-09-15T18:30:32Z

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

github-actions · 2022-09-16T07:34:20Z

Backend code coverage report for PR #55262
No changes

github-actions · 2022-09-16T07:34:21Z

Frontend code coverage report for PR #55262

Plugin	Main	PR	Difference
* public app features data-connections mocks store.navIndex.mock.ts	* public app features data-connections mocks store.navIndex.mock.ts%	* public app features data-connections mocks store.navIndex.mock.ts%	%

grafanabot · 2022-09-16T07:35:32Z

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

andresmgot

LGTM

andresmgot · 2022-09-16T07:47:05Z

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

@@ -258,7 +258,7 @@ const migrateGrafanaTemplateVariableFn = (query: AzureMonitorQuery) => {
    return query;
  }

-  const migratedQuery: AzureMonitorQuery = {


this can still be a const?

Indeed, I thought I had to change it when I had the delete for grafanaTemplateVariableFn but you're right 👍

andresmgot · 2022-09-16T07:47:43Z

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

@@ -43,6 +43,13 @@ export interface MetricNamespaceQuery extends BaseGrafanaTemplateVariableQuery {
  metricNamespace?: string;
  resourceName?: string;
 }
+export interface MetricDefinitionsQuery extends BaseGrafanaTemplateVariableQuery {


can you add a /* @deprecated message?

andresmgot · 2022-09-16T07:52:30Z

...ic/app/plugins/datasource/grafana-azure-monitor-datasource/dashboards/appInsightsGeoMap.json

@@ -822,7 +822,7 @@
        "options": [],
        "query": {
          "grafanaTemplateVariableFn": {
-            "kind": "MetricDefinitionsQuery",
+            "kind": "MetricNamespaceQuery",


usually it's good to leave these queries as the "old" way to check if we break migrations but for this case I guess it's okay to update them.

I guess it did not help us spot that we did break migrations ;)

I would rather update them in case someone uses them as template to create another curated dashboard.
But we could leave one (not importable) and use it in the tests...
WDYT?

yep, it's not automated so it's difficult to catch errors ... but it's helpful to reproduce issues :) anyway I also get your point for people using these as templates so it's also fine to update them.

grafanabot · 2022-09-16T08:37:50Z

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

asimpson

Nice work!

…te variable query types (#55262) * updated imported dashboards * Adds check for MetricDefinitionsQuery in migration * Removed delete from migratio * switched back to const for migratedQ * Added depreacted to MetricDefinitionQuery (cherry picked from commit 1e9f5a5)

…te variable query types (#55262) (#55359) * updated imported dashboards * Adds check for MetricDefinitionsQuery in migration * Removed delete from migratio * switched back to const for migratedQ * Added depreacted to MetricDefinitionQuery (cherry picked from commit 1e9f5a5) Co-authored-by: Yaelle Chaudy <42030685+yaelleC@users.noreply.github.com>

yaelleC added 2 commits September 15, 2022 20:08

updated imported dashboards

7683de4

Adds check for MetricDefinitionsQuery in migration

55a8be3

yaelleC requested a review from a team as a code owner September 15, 2022 18:20

yaelleC requested review from asimpson and andresmgot and removed request for a team September 15, 2022 18:20

grafanabot added area/frontend datasource/Azure Azure Monitor Datasource labels Sep 15, 2022

yaelleC commented Sep 15, 2022

View reviewed changes

yaelleC added backport v9.1.x Bot will automatically open backport PR add to changelog labels Sep 15, 2022

yaelleC added this to the 9.1.6 milestone Sep 15, 2022

grafanabot added the enterprise-failed label Sep 15, 2022

Removed delete from migratio

30757ab

andresmgot approved these changes Sep 16, 2022

View reviewed changes

yaelleC added 2 commits September 16, 2022 10:22

switched back to const for migratedQ

ce385b9

Added depreacted to MetricDefinitionQuery

a5f2401

asimpson approved these changes Sep 16, 2022

View reviewed changes

yaelleC merged commit 1e9f5a5 into main Sep 19, 2022

yaelleC deleted the fix_migration_issue_MetricDefinitionsQuery branch September 19, 2022 08:32

grafanabot mentioned this pull request Sep 19, 2022

[v9.1.x] Azure Monitor: Fix migration issue with MetricDefinitionsQuery template variable query types #55359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure Monitor: Fix migration issue with MetricDefinitionsQuery template variable query types #55262

Azure Monitor: Fix migration issue with MetricDefinitionsQuery template variable query types #55262

yaelleC commented Sep 15, 2022

yaelleC Sep 15, 2022

andresmgot Sep 16, 2022

yaelleC Sep 16, 2022

andresmgot Sep 16, 2022

yaelleC Sep 16, 2022

andresmgot Sep 16, 2022

grafanabot commented Sep 15, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

grafanabot commented Sep 16, 2022

andresmgot left a comment

andresmgot Sep 16, 2022

yaelleC Sep 16, 2022

andresmgot Sep 16, 2022

yaelleC Sep 16, 2022

andresmgot Sep 16, 2022

yaelleC Sep 16, 2022

andresmgot Sep 16, 2022

grafanabot commented Sep 16, 2022

asimpson left a comment

Azure Monitor: Fix migration issue with MetricDefinitionsQuery template variable query types #55262

Azure Monitor: Fix migration issue with MetricDefinitionsQuery template variable query types #55262

Conversation

yaelleC commented Sep 15, 2022

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

grafanabot commented Sep 15, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

grafanabot commented Sep 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented Sep 16, 2022

asimpson left a comment

Choose a reason for hiding this comment