-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prometheus: Variable query editor improvements #69884
Conversation
@bohandley looks like this might address #69859 |
I check out the branch and open my existing dashboard with a bunch of variables in it. Manual testing results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks but the rest is 🚀
variableEditor, | ||
}: MetricsLabelsSectionProps) { | ||
// fixing the use of 'as' from refactoring | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the type is QueryBuilderLabelFilter[]
. Do we need @ts-ignore
here?
I see that it is about LabelFilter.tsx
and specifically it is about onLabelsChange
function. Could you please check if that's possible to have the proper types? Let's avoid ts-ignore
as much as we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsmylife tsmylife This is what we had before
onChange={onChangeLabels as (labelFilters: Array<Partial<QueryBuilderLabelFilter>>) => void}
https://github.com/grafana/grafana/pull/69884/files#r1229794280
and
const onChangeLabels = (labels: QueryBuilderLabelFilter[]) => {
onChange({ ...query, labels });
};
https://github.com/grafana/grafana/pull/69884/files#r1229795429
When I moved this into a new file, using onChangeLabels as
was not permitted by prettier. I removed the as
and using the type QueryBuilderLabelFilter[]
this caused a type error.
https://github.com/grafana/grafana/pull/69884/files#r1229803709
I am not sure how to fix this as the function definition. The deinition, onChangeLabels as (labelFilters: Array<Partial<QueryBuilderLabelFilter>>) => void}
, is same in the LabelFilters.tsx component but betterer won't allow using as
. Do you have any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have any suggestions but I checked it briefly and realized that it is not a simple change to make. So we can leave it like this and address it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create an issue with the tech-debt label to address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create the issue once this PR is merged to track this.
const onGetLabelNames = async (forLabel: Partial<QueryBuilderLabelFilter>): Promise<SelectableValue[]> => { | ||
// If no metric we need to use a different method | ||
if (!query.metric) { | ||
// Todo add caching but inside language provider! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gtk-grafana I see it in this PR. Do we track this todo in somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the todo, the prometheusResourceBrowserCache
will cache this
public/app/plugins/datasource/prometheus/querybuilder/components/MetricSelect.tsx
Show resolved
Hide resolved
debounceDuration={datasource.getDebounceTimeInMilliseconds()} | ||
getLabelValuesAutofillSuggestions={getLabelValuesAutocompleteSuggestions} | ||
labelsFilters={query.labels} | ||
// eslint-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link for discussion about not using ts-ignore
@@ -35,171 +32,6 @@ export interface Props { | |||
export const PromQueryBuilder = React.memo<Props>((props) => { | |||
const { datasource, query, onChange, onRunQuery, data, showExplain } = props; | |||
const [highlightedOp, setHighlightedOp] = useState<QueryBuilderOperation | undefined>(); | |||
const onChangeLabels = (labels: QueryBuilderLabelFilter[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link for discussion about not using ts-ignore
debounceDuration={datasource.getDebounceTimeInMilliseconds()} | ||
getLabelValuesAutofillSuggestions={getLabelValuesAutocompleteSuggestions} | ||
labelsFilters={query.labels} | ||
onChange={onChangeLabels} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link for discussion about not using ts-ignore
*EDIT labels are now wrapped
*EDIT see latest commit for improved behavior for selects and inputs. Running onChange now for selecting a label |
@itsmylife wrap is working, looks better now 🎁 Labelfiltersflex.mov |
The wrapping looks good. I experienced strange behavior though. Probably not related to your changes but let's see. |
I was not able to reproduce this. It could be a timing issue due to dashboard setup? I have tried this with a prometheus instance and cortex but cannot reproduce this. After our discussion on slack, we are attributing this to a dashboard setup. I will make a note of this issue for future work. Thank you! Screen.Recording.2023-06-16.at.10.05.27.AM.mov |
I created a brand new dashboard (I was using my old one) and tried again. So I cannot reproduce it either :) So I think it is good to go. |
* refactor metric select and label filters, add variables to label names dropdown * use MetricsLabelsSection in variable editor * use variable editor UI with MetricsLabelsSection * filter label names by optional metric and allow metric to be cleared * fix tests * testing types for onChangeLabels * testing types for onChangeLabels * testing types for onChangeLabels * Ismails review, remove comment and indent * wrap label filters * improved behavior for selects and inputs, add tests and document behavior
Fixes #69859
What is this feature?
The Prom variable query editor allows a user to choose a
label_values
query type. This allows a user to choose a label name and an optional metric. These are the following updates to thelabel_values
query type inputs.Why do we need this feature?
The previous metric input for the label_values query type was a text input that was too small and the metric was not able to be filtered. There was also a recent PR to allow for the label_values label input to accept variables. These changes listed above add functionality to the query variable editor and allow the customer greater flexibility in creating a variable.
Who is this feature for?
Anyone using the prom query variable editor.
Please check that: