Prometheus: Improve prometheus query variable editor #63529

bohandley · 2023-02-21T20:51:37Z

This is the updated PR for the improved Prom Query Variable Editor that was reverted
The first was reverted because it did not migrate a variable created for a kubernetes-mixin using libsonnet. This is now handled and there are unit tests for this scenario.

Fixes #59559 (allow newlines in Prom query result variable query editor)
Fixes https://github.com/grafana/observability-metrics-squad/issues/48

What is this feature?
This changes the prometheus query variable editor to a group of selects and inputs to better explain the functions, arguments, and endpoint calls used in the prometheus variable queries.

Previous single input:

New function select and variable inputs:

Full proposal here.

Why do we need this feature?
This is in line with the Grafana 10 goal of "Getting Started." More clarity around creating variables in Prometheus will help onboard new customers.

Who is this feature for?
This feature is for anyone using the Prometheus datasource plugin who wants to create template variables.
This is for all users of prometheus datasource plugin, from expert to beginner.

Special notes for your reviewer:

Please create a prometheus variable in the main branch, for example, label_names()
Switch to the new branch and open the variable editor to see that the function has been correctly passed to the inputs.
Check that the variables are returned.
Look for confusing language and readability. Are the tooltips helpful?

…not set there flavor and version

…ev named getTagKeys()

…igration

…ility

…eryEditor.tsx Co-authored-by: Galen Kistler <109082771+gtk-grafana@users.noreply.github.com>

samjewell · 2023-02-28T21:35:01Z

Exciting! Thanks for this 🙏

* add prom query var editor with tests and migrations * fix migration, now query not expr * fix label_values migration * remove comments * fix label_values() variables order * update UI and use more clear language * fix tests * use null coalescing operators * allow users to query label values with label and metric if they have not set there flavor and version * use enums instead of numbers for readability * fix label&metrics switch * update type in qv editor * reuse datasource function to get all label names, getLabelNames(), prev named getTagKeys() * use getLabelNames in the query var editor * make label_values() variables label and metric more readable in the migration * fix tooltip for label_values to remove API reference * clean up tooltips and allow newlines in query_result function * change function wording and exprType to query type/qryType for readability * update prometheus query variable docs * Update public/app/plugins/datasource/prometheus/components/VariableQueryEditor.tsx Co-authored-by: Galen Kistler <109082771+gtk-grafana@users.noreply.github.com> * handle jsonnet grafana as code variables --------- Co-authored-by: Galen Kistler <109082771+gtk-grafana@users.noreply.github.com>

torkelo · 2023-09-19T08:22:33Z

public/app/plugins/datasource/prometheus/types.ts

+export interface PromVariableQuery extends DataQuery {
+  query?: string;
+  expr?: string;
+  qryType?: PromVariableQueryType;


@bohandley missspelled? This now in the persisted model, and we have this cryptic enum now numeric values in the persisted model (PromVariableQueryType)

@torkelo I meant to spell qryType that way but I can change it if that is not correct or not clear. When a user selects the query type of label names, label values, etc, this is the PromVariableQueryType. We use this to identify which should be selected in the new variable editor. This could use some refactoring to make that more clear.

All of this could be refactored though, as I originally built it to migrate to the new editor and then migrate back to the string to preserve the definitions in the variables list. I believe we are moving away from that so all of this could be much clearer with less code.

@bohandley well this is far from clear and is now in millions of dashboard json files as this part of the saved model, so really hurts the as code story (where dashboards are defined using the save model). Not only is "qryType" a very ugly thing to have as a property name in code but also in the save model, AND it just very cryptic numbers (0, 1, 2, 3) that have no meaning, enums must always have string values so we can make sense of them when looking at the save model.

and why do do we have query property and expr property?? some code comments on what these properties are would also help :)

If anyone would change the order of that enum type without a migration many dashboards would now break

btw, think the PR as whole is awesome, just need to make sure the save model is clear and not fragile and inscrutable :)

@bohandley well this is far from clear and is now in millions of dashboard json files as this part of the saved model, so really hurts the as code story (where dashboards are defined using the save model). Not only is "qryType" a very ugly thing to have as a property name in code but also in the save model, AND it just very cryptic numbers (0, 1, 2, 3) that have no meaning, enums must always have string values so we can make sense of them when looking at the save model.

@torkelo thank you! This is true, I had not considered the code story as it relates to dashboards, my apologies. This makes total sense. To see qryType: 1 in the dashboard json is both ugly and unclear as it relates to the code story. I will make sure any enums have string values moving forward.

and why do do we have query property and expr property?? some code comments on what these properties are would also help :)

Yes! I can add the code comments to the appropriate spot. I have some in the editor but I can see that this doesn't help when developers are reading the types to understand building a dashboard as code. The query and expr property are both there because I also had to migrate from standard variable support (which uses expr) and custom variable support (which uses query). So both fields would be present. I can add code comments for that!

grafana/public/app/plugins/datasource/prometheus/components/VariableQueryEditor.tsx

Lines 71 to 73 in 5aacdd9

// 1. Changing from standard to custom variable editor changes the string attr from expr to query

// 2. jsonnet grafana as code passes a variable as a string

const variableQuery = variableMigration(query);

I am sure there is a cleaner way to write all of this now, and I will create an issue to clean up some some of this. I will also add a reference point to make sure that any changes in the save model are as you say "clear, not fragile and inscrutable" 💯 Thank you for the feedback @torkelo!

@bohandley great! 👍

To track these issues: #75175 Thank you!

torkelo · 2023-09-20T07:40:00Z

public/app/plugins/datasource/prometheus/variables.ts

-  toDataQuery(query: StandardVariableQuery): PromQuery {
-    return {
-      refId: 'PrometheusDatasource-VariableQuery',
-      expr: query.query,
-    };
-  }


@bohandley
Without this I am unsure how this works with old dashboards? I can only see the migration being done for the variable query editor not when queries are executed. But maybe I am missing it

I think I know why it works still now, so don't worry about the comment above

bohandley and others added 21 commits February 9, 2023 15:22

add prom query var editor with tests and migrations

c0fa865

fix migration, now query not expr

d5bf616

fix label_values migration

757b3cc

remove comments

3c2fd45

fix label_values() variables order

f9eb1c1

update UI and use more clear language

4fd36a7

fix tests

2044f7a

use null coalescing operators

bd52f37

allow users to query label values with label and metric if they have …

67ad83d

…not set there flavor and version

use enums instead of numbers for readability

1b2945c

fix label&metrics switch

420259f

update type in qv editor

7575792

reuse datasource function to get all label names, getLabelNames(), pr…

5064482

…ev named getTagKeys()

use getLabelNames in the query var editor

46e62aa

make label_values() variables label and metric more readable in the m…

3407154

…igration

fix tooltip for label_values to remove API reference

ed9efe8

clean up tooltips and allow newlines in query_result function

75a0eb0

change function wording and exprType to query type/qryType for readab…

6766852

…ility

update prometheus query variable docs

9158cac

Update public/app/plugins/datasource/prometheus/components/VariableQu…

f37e340

…eryEditor.tsx Co-authored-by: Galen Kistler <109082771+gtk-grafana@users.noreply.github.com>

handle jsonnet grafana as code variables

72f87c3

bohandley added datasource/Prometheus area/frontend add to changelog area/datasource/frontend no-backport Skip backport of PR labels Feb 21, 2023

bohandley added this to the 9.5.0 milestone Feb 21, 2023

bohandley self-assigned this Feb 21, 2023

bohandley requested review from a team as code owners February 21, 2023 20:51

bohandley requested review from joshhunt, Eve832 and GrafanaWriter as code owners February 21, 2023 20:51

grafanabot added datasource/Tempo type/docs labels Feb 21, 2023

itsmylife approved these changes Feb 23, 2023

View reviewed changes

bohandley merged commit 2372b7c into main Feb 27, 2023

bohandley deleted the bohandley/improve-prom-query-var-editor branch February 27, 2023 13:38

dprokop mentioned this pull request Mar 9, 2023

QueryVariable: Enable custom variable support grafana/scenes#41

Merged

gtk-grafana mentioned this pull request Apr 13, 2023

Prometheus: Ad hoc filters not working in 9.5 #66520

Closed

torkelo reviewed Sep 19, 2023

View reviewed changes

torkelo reviewed Sep 20, 2023

View reviewed changes

bohandley mentioned this pull request Sep 20, 2023

Prometheus: Refactor variable query editor for clarity #75175

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus: Improve prometheus query variable editor #63529

Prometheus: Improve prometheus query variable editor #63529

bohandley commented Feb 21, 2023

samjewell commented Feb 28, 2023

torkelo Sep 19, 2023

bohandley Sep 19, 2023

torkelo Sep 20, 2023 •

edited

torkelo Sep 20, 2023

torkelo Sep 20, 2023

torkelo Sep 20, 2023

bohandley Sep 20, 2023

torkelo Sep 20, 2023

bohandley Sep 20, 2023 •

edited

torkelo Sep 20, 2023

torkelo Sep 20, 2023

	// 1. Changing from standard to custom variable editor changes the string attr from expr to query
	// 2. jsonnet grafana as code passes a variable as a string
	const variableQuery = variableMigration(query);

Prometheus: Improve prometheus query variable editor #63529

Prometheus: Improve prometheus query variable editor #63529

Conversation

bohandley commented Feb 21, 2023

samjewell commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkelo Sep 20, 2023 • 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

bohandley Sep 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkelo Sep 20, 2023 •

edited

bohandley Sep 20, 2023 •

edited