Notification Rule Processing Engine #5
Conversation
* wip * Add working actions for GMA rules based on Prom-only API * Remove Ruler-loader related code for Grafana rules Co-authored-by: Sonia Augilar <[email protected]> * Remove outdated tests * add some comments * remove commented code * remove showLocation property * Add missing mocks in tests * Add showLocation to GrafanaRuleListItem, improve useAbilities, address PR feedback * Enhance GrafanaGroupLoader tests: Add permission checks and More button functionality - Introduced user permission grants for alerting actions in tests. - Added tests for rendering the More button with action menu options. - Verified that each rule has its own action buttons and handles permissions correctly. - Ensured the edit button is not rendered when user lacks edit permissions. - Confirmed the correct menu actions are displayed when the More button is clicked. * Update translations --------- Co-authored-by: Sonia Aguilar <[email protected]> Co-authored-by: Sonia Augilar <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a significant architectural refactoring for Grafana's notification rule processing engine, moving from a dual API approach (Prometheus + Ruler) to a streamlined single-source architecture. The changes focus on optimizing performance by reducing API calls while maintaining functionality through enhanced type checking and permission management.
Key changes include:
New Rule Type Support: Added GrafanaPromRuleDTO type with provenance tracking in the unified alerting DTO, enabling better governance of rule origins. A new utility function isProvisionedPromRule was added to check provisioning status for Prometheus rules, complementing the existing isProvisionedRule function for ruler rules.
Component Architecture Overhaul: The GrafanaRuleLoader component was completely removed and its functionality consolidated into GrafanaGroupLoader, which now loads entire rule groups instead of individual rules. A new GrafanaRuleListItem component was created to handle rendering of different rule types (alerting, recording, unknown) with proper type checking.
Dual Permission System: Components like AlertRuleMenu and RuleActionsButtons now support checking permissions through both ruler API and Prometheus rule data using OR logic. This maintains backward compatibility while enabling the new optimized code paths that may not have ruler data available.
Hook System Enhancement: New hooks (useGrafanaPromRuleAbilities, useAllGrafanaPromRuleAbilities) were added to handle permissions for Grafana Prometheus rules, with both singular and plural variants for performance optimization.
Performance Optimization: Removed prefetching of ruler group data in prometheusGroupsGenerator.ts to reduce concurrent API calls, and simplified the FilterView to use direct rule rendering instead of intermediate loader components.
Cleanup: Removed unused localization keys and updated tests to accommodate the new architecture with comprehensive permission and UI interaction testing.
Confidence score: 2/5
• This PR introduces significant architectural changes that could break existing functionality, particularly around rule loading and permission checking
• Multiple critical implementation issues exist including missing function imports, potential runtime errors, and incomplete federated rule support
• Files need more attention: public/app/features/alerting/unified/hooks/useAbilities.ts, public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx, public/app/features/alerting/unified/rule-list/FilterView.tsx
14 files reviewed, 5 comments
| <GrafanaRuleListItem | ||
| rule={rule} | ||
| groupIdentifier={groupIdentifier} | ||
| namespaceName={ruleWithOrigin.namespaceName} | ||
| showLocation={true} | ||
| /> |
There was a problem hiding this comment.
logic: Missing key prop for list item. Should be key={key} to prevent React rendering issues.
| <GrafanaRuleListItem | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> | |
| <GrafanaRuleListItem | |
| key={key} | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> |
| */ | ||
| export function useAllGrafanaPromRuleAbilities(rule: GrafanaPromRuleDTO | undefined): Abilities<AlertRuleAction> { | ||
| // For GrafanaPromRuleDTO, we use useIsGrafanaPromRuleEditable instead | ||
| const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); // duplicate |
There was a problem hiding this comment.
style: Remove duplicate comment '// duplicate'
| const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); // duplicate | |
| const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); |
| }; | ||
|
|
||
| if (prometheusRuleType.grafana.alertingRule(rule)) { | ||
| const promAlertingRule = rule && rule.type === PromRuleType.Alerting ? rule : undefined; |
There was a problem hiding this comment.
style: Redundant type check - prometheusRuleType.grafana.alertingRule(rule) already confirms rule.type === PromRuleType.Alerting
| mocks.useGrafanaPromRuleAbility.mockImplementation((rule, action) => { | ||
| return action === AlertRuleAction.Update ? [true, false] : [true, true]; | ||
| }); |
There was a problem hiding this comment.
style: Potential inconsistency: useGrafanaPromRuleAbility is mocked but not used in this test. Consider removing unused mocks to avoid confusion
| duplicate: () => byRole('menuitem', { name: /duplicate/i }), | ||
| copyLink: () => byRole('menuitem', { name: /copy link/i }), | ||
| export: () => byRole('menuitem', { name: /export/i }), | ||
| delete: () => byRole('menuitem', { name: /delete/i }), |
There was a problem hiding this comment.
style: Delete menu item selector defined but never used in tests. Consider adding a test case for the delete action or remove unused selector.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
2 participants
Test 5