Alerting: Reduce number of request fetching rules in the dashboard view using rtkq #86991
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
soniaAguilarPeiron
added
area/alerting
soniaAguilarPeiron
requested review from
oscarkilhed,
kaydelaney,
gillesdemey,
tomratcliffe and
konrad147
and removed request for
a team
soniaAguilarPeiron
force-pushed
the
alerting/reduce-requests-in-dashboard-view
branch
2 times, most recently
from
4be1a3f
to
2372877
Compare
soniaAguilarPeiron
force-pushed
the
alerting/reduce-requests-in-dashboard-view
branch
from
2372877
to
9613576
Compare
…sts-in-dashboard-view
|
One general comment, since the old dashboard runner is going to be deprecated we should (also) implement this in the new scenes runner ( |
gillesdemey
reviewed
Apr 29, 2024
public/app/features/query/state/DashboardQueryRunner/UnifiedAlertStatesWorker.ts
Outdated
Show resolved
Hide resolved
konrad147
reviewed
Apr 30, 2024
public/app/features/query/state/DashboardQueryRunner/UnifiedAlertStatesWorker.ts
Outdated
Show resolved
Hide resolved
gillesdemey
reviewed
Apr 30, 2024
public/app/features/query/state/DashboardQueryRunner/UnifiedAlertStatesWorker.test.ts
Outdated
Show resolved
Hide resolved
gillesdemey
reviewed
Apr 30, 2024
public/app/features/dashboard-scene/panel-edit/PanelDataPane/PanelDataAlertingTab.test.tsx
Show resolved
Hide resolved
soniaAguilarPeiron
force-pushed
the
alerting/reduce-requests-in-dashboard-view
branch
from
9dda6db
to
c9b219b
Compare
…sts-in-dashboard-view
soniaAguilarPeiron
force-pushed
the
alerting/reduce-requests-in-dashboard-view
branch
from
d29a532
to
9a4b019
Compare
soniaAguilarPeiron
force-pushed
the
alerting/reduce-requests-in-dashboard-view
branch
from
9a4b019
to
40708e8
Compare
…d as optional params and rename the hook
| @@ -316,25 +337,6 @@ describe('PanelAlertTabContent', () => { | |||
| }; | |||
|
|
|||
| expect(defaultsWithDeterministicTime).toMatchSnapshot(); | |||
|
|
|||
There was a problem hiding this comment.
we agreed on removing this test after talking to @konrad147 as it's not clear that is testing the right thing.
I tested manually, and the link to the alert rule is correctly updated once the variable changes
…sts-in-dashboard-view
axelavargas
reviewed
May 9, 2024
There was a problem hiding this comment.
Awesome work @soniaAguilarPeiron , I was checking the code, and overall looks good :).
I left a question regarding some functionality I don't know was intentional to remove.
tomratcliffe
approved these changes
May 10, 2024
obetomuniz
added a commit
that referenced
this pull request
May 10, 2024
…-tracker * gops-configuration-tracker: (56 commits) SAML: add referemce to azure ad limitations (#87571) Update dependency glob to v10.3.14 Chore: Updated go.work.sum file (#87622) Update dependency @types/lodash to v4.17.1 (#87419) AdHocFilters: Use queries in ad hoc filters api calls (#87624) TimeSeries: Improve keyboard focus and fix spacebar override (#86848) Dropdown: Fixes double call to onVisibilityChange (#87607) [DOC] Adds missing link to Pyroscope core data source (#85493) Docs: Add Configuring Elasticsearch documentation for required privileges (#87593) Update dependency eslint-plugin-jsdoc to v48.2.4 Chore: Add login protection when user is trying different uppercase letters (#87588) Explore: lookup datasource by name when present in legacy URLs (#85222) Update dependency @prometheus-io/lezer-promql to v0.52.0 (#87554) Update dependency systemjs to v6.15.1 (#87594) Chore: Remove deprecated re-exported template variable types (#87459) Chore: Use getNextRefId instead of deprecated getNextRefIdChar (#87460) Chore: Remove use of deprecated method in AC code (#87541) Chore:: Update authlib version (#87603) Alerting: Reduce number of request fetching rules in the dashboard view using rtkq (#86991) refactor: rename variable ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this feature?
This pull request resolves the issue of fetching dashboard alert rules twice within the dashboard view.
The problem was that while rendering the dashboard view we were mixing Redux store selects and RTKQ requests...leading to a loss of caching ability and duplicate requests.
To address this, I opted to use
RTK Queryfor caching, aligning with our ongoing transition to this approach in the alerting codebase.For this, in this PR, a new
useCombinedRulesByDashboardhook is created to return combined rules using RTKQ instead of the Redux store. This hook is used inusePanelCombinedRules(necessary in the panel alert tab), and in theAlertRulesDrawerContent.Actually, this change will only avoid duplicated requests in the
AlertRulesDrawerContentand theUnifiedAlertStatesWorkerthat are the ones using the same params in the request (dashboardId), while in the alert tab is using dashboardId + panelId parametersWhy do we need this feature?
Fetching rules twice , could be expensive.
Who is this feature for?
All users.
Special notes for your reviewer:
After the change:
Please check that: