Alerting: Enable preview for recording rules #63260

VikaCep · 2023-02-10T14:43:32Z

What is this feature?

Allows to preview queries when creating/editing Recording Rules.

Why do we need this feature?

Previously, it was only possible to do it for Alerting rules.

Who is this feature for?

All users.

Which issue(s) does this PR fix?:

Fixes #35699

Special notes for your reviewer:

VikaCep · 2023-02-24T17:01:50Z

@gillesdemey I'm still working on this PR, but I'd like to have your input so far if you don't mind.
I've been struggling to get the Editor from the datasource itself to work with our queries (I've made some commits trying out that solution in this other branch but couldn't get the query to be visualized) so in the end it was easier to use the alerting QueryEditor component as it handles effectively creating a model for the new query and also displaying it using the visualization component.
The problem I'm facing now is displaying the recording rule query properly in the edit screen as the format that comes from the API for recording queries consists of just a string expression which is not compatible with what the QueryEditor expects. But before investing more time on this I wanted to have your opinion on the current implementation. I'd appreciate any feedback, thank you!

gillesdemey · 2023-03-01T16:03:53Z

🍐-ed up with @VikaCep on this one and we think we have a good way forward that allows us to render the <QueryEditor /> as long as we pass the right information in to the query.model.

It reuses QueryEditor and propagates a few properties to allow to filter the visible datasources and customize what's shown in the editor header

Otherwise it would get mixed up with the alert rules queries when switching back and forth from this option. This also allows me to initialize these queries with the right datasource

As now we use the query editor for recording rules which already includes a datasource picker within

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

...lerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx

soniaAguilarPeiron

Very nice!!, I added some comments 👀

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

soniaAguilarPeiron

Nice one! 🎉

konrad147 · 2023-03-10T09:56:46Z

I would try to avoid using the QueryEditor component. It renders a few components that might be misleading for users (collapse, query name, re-order handler). It also diverges from Prometheus' rules experience.
Could we try to use PanelRenderer for the visualization part? You can also take a look at public/app/features/alerting/unified/components/rule-viewer/RuleViewerVisualization.tsx to see how it works

Alternatively, we should tweak the query editor to also work for Prometheus rules so the experience is consistent

…ew-for-recording-rules

konrad147

Looks nice! 🎉 one nit: I'd add a bit of margin between the query editor and the viz wrapper

gillesdemey · 2023-03-20T12:07:47Z

I think this one still needs the tests to be updated, code LGTM!

VikaCep · 2023-03-21T20:13:32Z

Failing tests have been addressed and I applied some margin to the VizWrapper component as suggested by Konrad ✅

* Create RecordingRuleEditor component It reuses QueryEditor and propagates a few properties to allow to filter the visible datasources and customize what's shown in the editor header * Set recording rules queries as a new state prop Otherwise it would get mixed up with the alert rules queries when switching back and forth from this option. This also allows me to initialize these queries with the right datasource * Show CloudRulesSourcePicker only for Loki/Mimir rules As now we use the query editor for recording rules which already includes a datasource picker within * Fix lint and tests * Fix saving a recording rule * Show expression when editing the recording rule * Show query editor back for cloud rules * Fix duplicated import * Tweak after rebase * Remove ts-ignore * Refactor to use queries state instead of recordingRuleQueries * Refacrtor RecordingRuleEditor to use ds QueryEditor * Revert extra properties previously added to QueryEditor components * Remove console.log * Fix saving/editing a recording rule * Fix tests * Add margin to vizwrapper component

grafanabot added the area/frontend label Feb 10, 2023

VikaCep force-pushed the alerting/enable-preview-for-recording-rules branch 2 times, most recently from 8a0ae14 to 4335e52 Compare March 6, 2023 12:40

VikaCep changed the title ~~[WIP] Alerting: enable preview for recording rules~~ Alerting: enable preview for recording rules Mar 6, 2023

VikaCep added this to the 9.5.0 milestone Mar 6, 2023

VikaCep added the no-backport Skip backport of PR label Mar 6, 2023

VikaCep self-assigned this Mar 6, 2023

VikaCep marked this pull request as ready for review March 6, 2023 12:56

VikaCep requested review from a team as code owners March 6, 2023 12:56

VikaCep requested review from axelavargas and polibb and removed request for a team March 6, 2023 12:56

VikaCep added 9 commits March 6, 2023 13:38

Create RecordingRuleEditor component

d881a91

It reuses QueryEditor and propagates a few properties to allow to filter the visible datasources and customize what's shown in the editor header

Set recording rules queries as a new state prop

b9e2813

Otherwise it would get mixed up with the alert rules queries when switching back and forth from this option. This also allows me to initialize these queries with the right datasource

Show CloudRulesSourcePicker only for Loki/Mimir rules

212b415

As now we use the query editor for recording rules which already includes a datasource picker within

Fix lint and tests

0a1ae0c

Fix saving a recording rule

3fc3188

Show expression when editing the recording rule

47fdc0d

Show query editor back for cloud rules

20696b7

Fix duplicated import

491bd33

Tweak after rebase

158e721

VikaCep force-pushed the alerting/enable-preview-for-recording-rules branch from 4335e52 to 158e721 Compare March 6, 2023 16:50

soniaAguilarPeiron reviewed Mar 8, 2023

View reviewed changes

public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx Outdated Show resolved Hide resolved

soniaAguilarPeiron reviewed Mar 8, 2023

View reviewed changes

...lerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx Outdated Show resolved Hide resolved

soniaAguilarPeiron reviewed Mar 8, 2023

View reviewed changes

public/app/features/alerting/unified/types/rule-form.ts Outdated Show resolved Hide resolved

axelavargas removed their request for review March 8, 2023 11:06

VikaCep added 2 commits March 8, 2023 12:37

Remove ts-ignore

3969fc8

Refactor to use queries state instead of recordingRuleQueries

ca9107a

VikaCep added the add to changelog label Mar 8, 2023

VikaCep requested a review from soniaAguilarPeiron March 8, 2023 17:59

soniaAguilarPeiron approved these changes Mar 9, 2023

View reviewed changes

soniaAguilarPeiron requested review from konrad147 and gillesdemey March 9, 2023 11:06

VikaCep added 5 commits March 17, 2023 15:31

Merge remote-tracking branch 'origin/main' into alerting/enable-previ…

553baf9

…ew-for-recording-rules

Merge remote-tracking branch 'origin/main' into alerting/enable-previ…

cd19a64

…ew-for-recording-rules

Refacrtor RecordingRuleEditor to use ds QueryEditor

a47b2f4

Revert extra properties previously added to QueryEditor components

cdb533f

Remove console.log

f7354ba

VikaCep removed the request for review from polibb March 17, 2023 22:31

konrad147 approved these changes Mar 20, 2023

View reviewed changes

Fix saving/editing a recording rule

9baaf6b

gillesdemey changed the title ~~Alerting: enable preview for recording rules~~ Alerting: Enable preview for recording rules Mar 21, 2023

VikaCep added 2 commits March 21, 2023 16:55

Fix tests

1a41e40

Add margin to vizwrapper component

bec0277

VikaCep merged commit a1fc515 into main Mar 22, 2023
6 checks passed

VikaCep deleted the alerting/enable-preview-for-recording-rules branch March 22, 2023 12:21

VikaCep mentioned this pull request Mar 22, 2023

Alerting: Improve query types usage to prevent ts-errors #65238

Merged

VikaCep added the add to what's new label Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Enable preview for recording rules #63260

Alerting: Enable preview for recording rules #63260

VikaCep commented Feb 10, 2023 •

edited

VikaCep commented Feb 24, 2023

gillesdemey commented Mar 1, 2023

soniaAguilarPeiron left a comment

soniaAguilarPeiron left a comment

konrad147 commented Mar 10, 2023 •

edited

konrad147 left a comment

gillesdemey commented Mar 20, 2023

VikaCep commented Mar 21, 2023

Alerting: Enable preview for recording rules #63260

Alerting: Enable preview for recording rules #63260

Conversation

VikaCep commented Feb 10, 2023 • edited

VikaCep commented Feb 24, 2023

gillesdemey commented Mar 1, 2023

soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

konrad147 commented Mar 10, 2023 • edited

konrad147 left a comment

Choose a reason for hiding this comment

gillesdemey commented Mar 20, 2023

VikaCep commented Mar 21, 2023

VikaCep commented Feb 10, 2023 •

edited

konrad147 commented Mar 10, 2023 •

edited