Alerting: Fix changing datasource and creating new query not using defaults. #63092

soniaAguilarPeiron · 2023-02-08T12:09:01Z

What is this feature?

This PR sets default query from the data source when creating new alert rule, changing data source or adding a new query in the alert rule form.

When creating a new alert rule => uses default query in case of existing.
When changing data source => in case of different type => uses default query in case of existing, otherwise, clears the query .
When creating a new query => uses default query in case of existing

Why do we need this feature?

Data sources can include a default value for query that should be used in the cases described before.

Who is this feature for?

All users

Which issue(s) does this PR fix?:

Fixes #61280

grafanabot · 2023-02-10T08:28:34Z

Hello @soniaAguilarPeiron!
Backport pull requests need to be either:

Pull requests which address bugs,
Urgent fixes which need product approval, in order to get merged,
Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

gillesdemey

Is there a way we can split this PR in to two distinct problems:

clearing the model when switching DS
applying getDefaultQuery

I feel like the former is more important than the latter and I think there might be some opportunities to improve on the latter while not blocking a bug fix for 1.

gillesdemey · 2023-02-22T10:34:46Z

public/app/features/alerting/unified/utils/rule-form.ts

@@ -199,9 +202,46 @@ export function recordingRulerRuleToRuleForm(
  };
 }

-export const getDefaultQueries = (): AlertQuery[] => {
+export const getDefaultQueriesAsync = async (): Promise<{
+  queries: AlertQuery[] | null;


Why would we return null here instead of an empty array?

we use null to differentiate between the resolved value that can be an empty array or the null that indicates in some parts of the code that we don't have the value yet.

gillesdemey · 2023-02-22T10:38:56Z

...unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap

        },
+        "datasourceUid": "",


This diff seems weird, why did the type and uid change? And why is the datasourceUid in the model not the same as at the root (some uid)?

now in the test we are using this default query in the payload (we didn't have this payload before)

const defaultQuery: AlertQuery = { refId: 'A', queryType: '', =>>>>>>>>>> datasourceUid: '',=>>>>>>>>>> model: { refId: 'A', expression: 'THIS IS THE DEFAULT EXPRESSION' }, relativeTimeRange: getDefaultRelativeTimeRange(), };

where the queryType is empty and also the datasource. That's why we get this snapshot.

gillesdemey · 2023-02-22T12:14:31Z

public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx

@@ -73,6 +89,26 @@ type Props = {
  prefill?: Partial<RuleFormValues>; // Existing implies we modify existing rule. Prefill only provides default form values
 };

+export const useGetDefaults = (queryParams: UrlQueryMap, existing: RuleWithLocation<RulerRuleDTO> | undefined) => {
+  const [defaultDsAndQueries, setDefaultDsAndQueries] = useState<{
+    queries: AlertQuery[] | null;


Dito null

we use null to differentiate between the resolved value that can be an empty array or the null that indicates in some parts of the code that we don't have the value yet

gillesdemey · 2023-02-22T12:16:11Z

public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx

+  }>({ queries: null });
+
+  useEffect(() => {
+    const getDefaultQueries_ = async () => {


We can turn this in to an iife, but I prefer using useAsync if we can

gillesdemey · 2023-02-22T12:44:54Z

public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx

+                prefill={!!prefill}
+                onDataChange={checkAlertCondition}
+                asyncDefaultQueries={defaultDsAndQueries.queries}
+                asyncDataSource={defaultDsAndQueries.ds}


I'm a bit confused here why we need to pass these separately since getDefaultQueries is a function we can call on the DataSourceApi.

this defaultDsAndQueries.queries is not the same as we get from DataSourceApi.getDefaultQuery. This defaultDsAndQueries.queries is:

queries: [ { refId: 'A', datasourceUid: dataSource.uid, queryType: '', relativeTimeRange, model: { refId: 'A', hide: false, ...ds?.getDefaultQuery?.(CoreApp.UnifiedAlerting), }, }, ...getDefaultExpressions('B', 'C'), ],

gillesdemey · 2023-02-22T13:03:44Z

public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx

-              <QueryAndExpressionsStep editingExistingRule={!!existing} onDataChange={checkAlertCondition} />
+              <QueryAndExpressionsStep
+                editingExistingRule={!!existing}
+                prefill={!!prefill}


I'm not sure if I understand why these props have to be passed all the way down in to the QueryAndExpressionsStep, it looks like we're using them to make some assertions on when to dispatch setDataQueries?

We need to pass all the way down because:

we need default queries for initialising the form with the default values. (when creating a new alert rule)

we need also these default queries as you said in the QueryAndExpressionsStep to set queries once this async value is set, and also for the onAddNewQuery (to set the default)

at the same time we need to keep updated this async default value in the ExpressionEditor when we are creating a new alert.

Gotcha, that makes sense!

soniaAguilarPeiron · 2023-02-22T16:54:04Z

@gillesdemey I updated the description of the PR, because this PR is actually setting the defaults. What I see is that the problem of not clearing the model is not happening. I was testing it and I didn't find any issues on that. @konrad147 mentioned the same. Maybe something related to data source plugins changed and this error is gone?

gillesdemey

Everything looks good, let's ship it!

…faults. (#63092) * Set default query when changing data source or adding new alert query * Set default query when creating new alert rule * Set fefault query for cloud and recording rules * Create hook for getting defaults in AlertRuleForm * fixing tests * Use conditionals with 'if' for more clarity and rename lazy to async * Add loading indicator for default queries * Fix tests * Make newModel a sync method and fix tests error * Use useAsync instead of useEffect for an async call (cherry picked from commit b42f973)

… using defaults. (#63720) Alerting: Fix changing datasource and creating new query not using defaults. (#63092) * Set default query when changing data source or adding new alert query * Set default query when creating new alert rule * Set fefault query for cloud and recording rules * Create hook for getting defaults in AlertRuleForm * fixing tests * Use conditionals with 'if' for more clarity and rename lazy to async * Add loading indicator for default queries * Fix tests * Make newModel a sync method and fix tests error * Use useAsync instead of useEffect for an async call (cherry picked from commit b42f973) Co-authored-by: Sonia Aguilar <33540275+soniaAguilarPeiron@users.noreply.github.com>

…faults. (#63092) * Set default query when changing data source or adding new alert query * Set default query when creating new alert rule * Set fefault query for cloud and recording rules * Create hook for getting defaults in AlertRuleForm * fixing tests * Use conditionals with 'if' for more clarity and rename lazy to async * Add loading indicator for default queries * Fix tests * Make newModel a sync method and fix tests error * Use useAsync instead of useEffect for an async call

…using defaults. (#63092)" This reverts commit b42f973.

soniaAguilarPeiron added area/alerting Grafana Alerting area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Feb 8, 2023

soniaAguilarPeiron added this to the 9.4.1 milestone Feb 8, 2023

soniaAguilarPeiron requested a review from a team as a code owner February 8, 2023 12:09

soniaAguilarPeiron self-assigned this Feb 8, 2023

soniaAguilarPeiron requested a review from a team as a code owner February 8, 2023 12:09

soniaAguilarPeiron requested a review from a team February 8, 2023 12:09

soniaAguilarPeiron requested a review from a team as a code owner February 8, 2023 12:09

soniaAguilarPeiron requested review from joshhunt, JoaoSilvaGrafana, mckn, oscarkilhed and mdvictor and removed request for a team February 8, 2023 12:09

soniaAguilarPeiron marked this pull request as draft February 8, 2023 12:09

soniaAguilarPeiron force-pushed the alerting/SetDefaultQuery branch from da3f8d0 to 53c6d27 Compare February 8, 2023 12:24

soniaAguilarPeiron changed the title ~~Alerting: Set default query~~ Alerting: Fix changing datasource not clearing model and using defaults. Feb 8, 2023

soniaAguilarPeiron force-pushed the alerting/SetDefaultQuery branch 3 times, most recently from 3a1cf72 to 504b66e Compare February 9, 2023 10:10

soniaAguilarPeiron marked this pull request as ready for review February 9, 2023 10:10

soniaAguilarPeiron force-pushed the alerting/SetDefaultQuery branch from 504b66e to 284321c Compare February 10, 2023 08:22

soniaAguilarPeiron requested a review from a team as a code owner February 10, 2023 08:22

soniaAguilarPeiron added backport v9.4.x Mark PR for automatic backport to v9.4.x type/bug labels Feb 10, 2023

gillesdemey reviewed Feb 22, 2023

View reviewed changes

soniaAguilarPeiron added 10 commits February 22, 2023 16:50

Set default query when changing data source or adding new alert query

afc023e

Set default query when creating new alert rule

ad95917

Set fefault query for cloud and recording rules

36e8ce7

Create hook for getting defaults in AlertRuleForm

545b84c

fixing tests

ab7a503

Use conditionals with 'if' for more clarity and rename lazy to async

c2407b6

Add loading indicator for default queries

fba2721

Fix tests

9c620eb

Make newModel a sync method and fix tests error

bb6804a

Use useAsync instead of useEffect for an async call

66fc7ed

soniaAguilarPeiron force-pushed the alerting/SetDefaultQuery branch from d398a58 to 66fc7ed Compare February 22, 2023 15:51

soniaAguilarPeiron requested a review from gillesdemey February 22, 2023 15:57

soniaAguilarPeiron changed the title ~~Alerting: Fix changing datasource not clearing model and using defaults.~~ Alerting: Fix changing datasource and creating new query not using defaults. Feb 22, 2023

gillesdemey approved these changes Feb 24, 2023

View reviewed changes

soniaAguilarPeiron merged commit b42f973 into main Feb 24, 2023

soniaAguilarPeiron deleted the alerting/SetDefaultQuery branch February 24, 2023 12:43

grafanabot removed the missing-labels label Feb 24, 2023

grafanabot mentioned this pull request Feb 24, 2023

[v9.4.x] Alerting: Fix changing datasource and creating new query not using defaults. #63720

Merged

zerok modified the milestones: 9.4.1, 9.4.2 Feb 27, 2023

zerok modified the milestones: 9.4.2, 9.4.3, 9.4.4 Mar 2, 2023

gillesdemey added a commit that referenced this pull request Mar 6, 2023

Revert "Alerting: Fix changing datasource and creating new query not …

ac3aa39

…using defaults. (#63092)" This reverts commit b42f973.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Fix changing datasource and creating new query not using defaults. #63092

Alerting: Fix changing datasource and creating new query not using defaults. #63092

soniaAguilarPeiron commented Feb 8, 2023 •

edited

grafanabot commented Feb 10, 2023

gillesdemey left a comment

gillesdemey Feb 22, 2023

soniaAguilarPeiron Feb 22, 2023 •

edited

gillesdemey Feb 22, 2023

soniaAguilarPeiron Feb 22, 2023 •

edited

gillesdemey Feb 22, 2023

soniaAguilarPeiron Feb 22, 2023 •

edited

gillesdemey Feb 22, 2023

soniaAguilarPeiron Feb 22, 2023

gillesdemey Feb 22, 2023

soniaAguilarPeiron Feb 22, 2023 •

edited

gillesdemey Feb 22, 2023

soniaAguilarPeiron Feb 22, 2023

gillesdemey Feb 24, 2023

soniaAguilarPeiron commented Feb 22, 2023

gillesdemey left a comment

Alerting: Fix changing datasource and creating new query not using defaults. #63092

Alerting: Fix changing datasource and creating new query not using defaults. #63092

Conversation

soniaAguilarPeiron commented Feb 8, 2023 • edited

grafanabot commented Feb 10, 2023

gillesdemey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soniaAguilarPeiron Feb 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soniaAguilarPeiron Feb 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soniaAguilarPeiron Feb 22, 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

soniaAguilarPeiron Feb 22, 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

soniaAguilarPeiron commented Feb 22, 2023

gillesdemey left a comment

Choose a reason for hiding this comment

soniaAguilarPeiron commented Feb 8, 2023 •

edited

soniaAguilarPeiron Feb 22, 2023 •

edited

soniaAguilarPeiron Feb 22, 2023 •

edited

soniaAguilarPeiron Feb 22, 2023 •

edited

soniaAguilarPeiron Feb 22, 2023 •

edited