-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
Alerting: Use new endpoints to fetch single GMA rule on view and edit pages #87625
Conversation
…iew-edit-performance
/deploy-to-hg |
|
|
/deploy-to-hg |
|
|
This is looking really nice. I discussed with @konrad147 already but repeating here:
|
@@ -43,6 +43,7 @@ export const alertingApi = createApi({ | |||
'DataSourceSettings', | |||
'GrafanaLabels', | |||
'CombinedAlertRule', | |||
'GrafanaRulerRule', |
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.
👌
/deploy-to-hg |
/deploy-to-hg |
|
|
}: { | ||
ruleIdentifier: RuleIdentifier; | ||
}): RequestState<RuleWithLocation> { | ||
const ruleSource = getRulesSourceFromIdentifier(ruleIdentifier); |
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.
Do we really need this ruleSource?
We are only using the ruleSource name that it's already included in the ruleIdentifier.
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'm not the original author of this piece but it looks to be to resolve the rulesSourceName
(which is a string) to either grafana
or an instance of DataSourceInstanceSettings
which we don't have in the rule identifier if I'm not mistaken.
(I do have some experiments set aside that pass the entire data source settings with the rule identifier so we don't have to resolve ad-hoc but this seems like a separate piece of work)
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.
What I mean is that if you look to this function this ruleSource
is not used for anything. It only appears in a check
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.
LGTM!
/deploy-to-hg |
|
|
What is this feature?
In #86845 we added the ability to retrieve the alert rule definition for Grafana-managed rules by a UID.
This PR refactors rule fetching for Grafana-managed rules to use this new endpoint and significantly increase the speed at which we can render the detail view for a single alert rule.
Depends on #88295