Skip to content
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

Fix alert groups automatic refresh #4463

Merged
merged 8 commits into from
Jun 6, 2024
Merged

Fix alert groups automatic refresh #4463

merged 8 commits into from
Jun 6, 2024

Conversation

teodosii
Copy link
Member

@teodosii teodosii commented Jun 5, 2024

What this PR does

  • Fixes AG auto-refresh (every 15s)
  • Reflect correct data on the UI after doing a bulk update
  • Few tweaks regarding UX around bulk updates/loading
  • Added some missing typescript support

Which issue(s) this PR closes

Closes #4454

@teodosii teodosii requested a review from a team June 5, 2024 11:31
@teodosii teodosii added pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes labels Jun 5, 2024
@@ -48,7 +48,7 @@ interface RemoteFiltersProps extends WithStoreProps {
grafanaTeamStore: GrafanaTeamStore;
skipFilterOptionFn?: (filterOption: FilterOption) => boolean;
}
interface RemoteFiltersState {
export interface RemoteFiltersState {
Copy link
Member Author

@teodosii teodosii Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exporting these to have the typing needed in Incidents for now. This might need some additional work as we're missing a lot of typescript on the filters, but it's out of scope for this PR.

static async updateBulkActionAndRefresh(
data: ApiSchemas['AlertGroupBulkActionRequest'],
alertGroupStore: AlertGroupStore,
onFinally?: () => void
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made more sense to be here rather than in the component, it also accepts AutoLoadingState

@@ -79,7 +79,7 @@ export class AlertGroupStore {
const newAlerts = new Map(
results.map((alert: ApiSchemas['AlertGroup']) => {
const oldAlert = this.alerts.get(alert.pk) || {};
const mergedAlertData = { ...oldAlert, ...alert, undoAction: alert.undoAction };
const mergedAlertData = { ...oldAlert, ...alert };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the undoAction, it didn't seem to help in any way 🤔

this.setLiveUpdatesPaused(true);
await actionToMethodMap[action](id, delay);
} finally {
this.setLiveUpdatesPaused(false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Centralizing these 2 calls here rather than in each method

const hasSelected = selectedIncidentIds.length > 0;
const isLoading = LoaderHelper.isLoading(store.loaderStore, ActionKey.FETCH_INCIDENTS);
const hasInvalidatedAlert = Boolean(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this didn't add any value whatsoever, due to this we were showing "Out of date results" which didn't make sense as we didn't invalidate any data whatsoever

@brojd
Copy link
Contributor

brojd commented Jun 5, 2024

Can we refetch stats whenever alert status is updated (either with bulk or single update)? Right now user needs to wait for the next polling tick:

Screen.Recording.2024-06-05.at.14.24.16.mov

@teodosii teodosii added this pull request to the merge queue Jun 6, 2024
Merged via the queue into dev with commit b9d344c Jun 6, 2024
21 checks passed
@teodosii teodosii deleted the rares/ag-refresh-fix branch June 6, 2024 08:32
brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

- Fixes AG auto-refresh (every 15s)
- Reflect correct data on the UI after doing a bulk update
- Few tweaks regarding UX around bulk updates/loading
- Added some missing typescript support

## Which issue(s) this PR closes

Closes #4454
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with refreshing the alert groups
2 participants