From efe274b46543eb304c304129115bea8551b33f6e Mon Sep 17 00:00:00 2001 From: Dominik Broj Date: Thu, 30 Nov 2023 12:05:13 +0100 Subject: [PATCH] Improve loading initial data (#3450) # What this PR does - Split initial data into required base data and master data (options for dropdowns) - Prioritize loading base data over master data - Enable retrying single promises instead of always retrying all of them when only one fails ## Which issue(s) this PR fixes https://github.com/grafana/oncall/issues/3300 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- grafana-plugin/package.json | 1 + .../RenderConditionally.tsx | 5 +- .../EscalationChainSteps.tsx | 6 ++ .../OutgoingWebhookForm.config.tsx | 2 +- .../PersonalNotificationSettings.tsx | 9 +-- .../alert_receive_channel.ts | 3 +- grafana-plugin/src/models/cloud/cloud.ts | 1 + .../escalation_policy/escalation_policy.ts | 9 +-- .../src/models/grafana_team/grafana_team.ts | 3 +- grafana-plugin/src/models/label/label.ts | 7 +-- .../src/models/organization/organization.ts | 5 +- .../outgoing_webhook/outgoing_webhook.ts | 4 +- grafana-plugin/src/models/user/user.ts | 10 +--- .../outgoing_webhooks/OutgoingWebhooks.tsx | 4 ++ .../src/plugin/GrafanaPluginRootPage.tsx | 60 +++++++++---------- .../src/state/rootBaseStore/index.ts | 34 ++++++----- grafana-plugin/src/utils/async.test.ts | 33 ++++++++++ grafana-plugin/src/utils/async.ts | 9 +++ grafana-plugin/yarn.lock | 5 ++ 19 files changed, 129 insertions(+), 81 deletions(-) create mode 100644 grafana-plugin/src/utils/async.test.ts create mode 100644 grafana-plugin/src/utils/async.ts diff --git a/grafana-plugin/package.json b/grafana-plugin/package.json index 5219ad4a..65a9aa65 100644 --- a/grafana-plugin/package.json +++ b/grafana-plugin/package.json @@ -125,6 +125,7 @@ "@grafana/labels": "~1.3.5", "@grafana/runtime": "9.3.0-beta1", "@grafana/ui": "^10.2.0", + "@lifeomic/attempt": "^3.0.3", "@opentelemetry/api": "^1.3.0", "array-move": "^4.0.0", "change-case": "^4.1.1", diff --git a/grafana-plugin/src/components/RenderConditionally/RenderConditionally.tsx b/grafana-plugin/src/components/RenderConditionally/RenderConditionally.tsx index e6a5d8b3..40b4a0d8 100644 --- a/grafana-plugin/src/components/RenderConditionally/RenderConditionally.tsx +++ b/grafana-plugin/src/components/RenderConditionally/RenderConditionally.tsx @@ -3,9 +3,10 @@ import React, { FC, ReactNode } from 'react'; interface RenderConditionallyProps { shouldRender?: boolean; children: ReactNode; + backupChildren?: ReactNode; } -const RenderConditionally: FC = ({ shouldRender, children }) => - shouldRender ? <>{children} : null; +const RenderConditionally: FC = ({ shouldRender, children, backupChildren = null }) => + shouldRender ? <>{children} : <>{backupChildren}; export default RenderConditionally; diff --git a/grafana-plugin/src/containers/EscalationChainSteps/EscalationChainSteps.tsx b/grafana-plugin/src/containers/EscalationChainSteps/EscalationChainSteps.tsx index fe00a744..134a91c9 100644 --- a/grafana-plugin/src/containers/EscalationChainSteps/EscalationChainSteps.tsx +++ b/grafana-plugin/src/containers/EscalationChainSteps/EscalationChainSteps.tsx @@ -37,6 +37,12 @@ const EscalationChainSteps = observer((props: EscalationChainStepsProps) => { escalationPolicyStore.updateEscalationPolicies(id); }, [id]); + useEffect(() => { + escalationPolicyStore.updateWebEscalationPolicyOptions(); + escalationPolicyStore.updateEscalationPolicyOptions(); + escalationPolicyStore.updateNumMinutesInWindowOptions(); + }, []); + const handleSortEnd = useCallback( ({ oldIndex, newIndex }: any) => { escalationPolicyStore.moveEscalationPolicyToPosition(oldIndex, newIndex, id); diff --git a/grafana-plugin/src/containers/OutgoingWebhookForm/OutgoingWebhookForm.config.tsx b/grafana-plugin/src/containers/OutgoingWebhookForm/OutgoingWebhookForm.config.tsx index 32a110c5..c3d7bb25 100644 --- a/grafana-plugin/src/containers/OutgoingWebhookForm/OutgoingWebhookForm.config.tsx +++ b/grafana-plugin/src/containers/OutgoingWebhookForm/OutgoingWebhookForm.config.tsx @@ -22,7 +22,7 @@ export const WebhookTriggerType = { }; export function createForm( - presets: OutgoingWebhookPreset[], + presets: OutgoingWebhookPreset[] = [], hasLabelsFeature?: boolean ): { name: string; diff --git a/grafana-plugin/src/containers/PersonalNotificationSettings/PersonalNotificationSettings.tsx b/grafana-plugin/src/containers/PersonalNotificationSettings/PersonalNotificationSettings.tsx index 6133730f..fef69a4f 100644 --- a/grafana-plugin/src/containers/PersonalNotificationSettings/PersonalNotificationSettings.tsx +++ b/grafana-plugin/src/containers/PersonalNotificationSettings/PersonalNotificationSettings.tsx @@ -63,7 +63,6 @@ const PersonalNotificationSettings = observer((props: PersonalNotificationSettin ); const allNotificationPolicies = userStore.notificationPolicies[userPk]; - const title = ( @@ -91,11 +90,9 @@ const PersonalNotificationSettings = observer((props: PersonalNotificationSettin ); } - const notificationPolicies = - allNotificationPolicies && - allNotificationPolicies.filter( - (notificationPolicy: NotificationPolicyType) => notificationPolicy.important === isImportant - ); + const notificationPolicies = allNotificationPolicies?.filter( + (notificationPolicy: NotificationPolicyType) => notificationPolicy.important === isImportant + ); const offset = isImportant ? allNotificationPolicies.findIndex((notificationPolicy: NotificationPolicyType) => notificationPolicy.important) diff --git a/grafana-plugin/src/models/alert_receive_channel/alert_receive_channel.ts b/grafana-plugin/src/models/alert_receive_channel/alert_receive_channel.ts index 73c31cff..e2792a61 100644 --- a/grafana-plugin/src/models/alert_receive_channel/alert_receive_channel.ts +++ b/grafana-plugin/src/models/alert_receive_channel/alert_receive_channel.ts @@ -317,10 +317,9 @@ export class AlertReceiveChannelStore extends BaseStore { return this.updateChannelFilters(channelFilter.alert_receive_channel, true); } - @action + @action.bound async updateAlertReceiveChannelOptions() { const response = await makeRequest(`/alert_receive_channels/integration_options/`, {}); - this.alertReceiveChannelOptions = response; } diff --git a/grafana-plugin/src/models/cloud/cloud.ts b/grafana-plugin/src/models/cloud/cloud.ts index 24e84e55..5c8f5e99 100644 --- a/grafana-plugin/src/models/cloud/cloud.ts +++ b/grafana-plugin/src/models/cloud/cloud.ts @@ -68,6 +68,7 @@ export class CloudStore extends BaseStore { return await makeRequest(`${this.path}${id}`, { method: 'GET' }); } + @action.bound async loadCloudConnectionStatus() { this.cloudConnectionStatus = await this.getCloudConnectionStatus(); } diff --git a/grafana-plugin/src/models/escalation_policy/escalation_policy.ts b/grafana-plugin/src/models/escalation_policy/escalation_policy.ts index 3d140fe5..3872f8af 100644 --- a/grafana-plugin/src/models/escalation_policy/escalation_policy.ts +++ b/grafana-plugin/src/models/escalation_policy/escalation_policy.ts @@ -34,26 +34,23 @@ export class EscalationPolicyStore extends BaseStore { this.path = '/escalation_policies/'; } - @action + @action.bound async updateWebEscalationPolicyOptions() { const response = await makeRequest('/escalation_policies/escalation_options/', {}); - this.webEscalationChoices = response; } - @action + @action.bound async updateEscalationPolicyOptions() { const response = await makeRequest('/escalation_policies/', { method: 'OPTIONS', }); - this.escalationChoices = get(response, 'actions.POST', []); } - @action + @action.bound async updateNumMinutesInWindowOptions() { const response = await makeRequest('/escalation_policies/num_minutes_in_window_options/', {}); - this.numMinutesInWindowOptions = response; } diff --git a/grafana-plugin/src/models/grafana_team/grafana_team.ts b/grafana-plugin/src/models/grafana_team/grafana_team.ts index 023150be..04dd55a8 100644 --- a/grafana-plugin/src/models/grafana_team/grafana_team.ts +++ b/grafana-plugin/src/models/grafana_team/grafana_team.ts @@ -30,7 +30,7 @@ export class GrafanaTeamStore extends BaseStore { }; } - @action + @action.bound async updateItems(query = '', includeNoTeam = true, onlyIncludeNotifiableTeams = false, short = true) { const result = await makeRequest(`${this.path}`, { params: { @@ -40,7 +40,6 @@ export class GrafanaTeamStore extends BaseStore { only_include_notifiable_teams: onlyIncludeNotifiableTeams ? 'true' : 'false', }, }); - this.items = { ...this.items, ...result.reduce( diff --git a/grafana-plugin/src/models/label/label.ts b/grafana-plugin/src/models/label/label.ts index b9638879..1138550e 100644 --- a/grafana-plugin/src/models/label/label.ts +++ b/grafana-plugin/src/models/label/label.ts @@ -1,4 +1,4 @@ -import { action, observable, runInAction } from 'mobx'; +import { action, observable } from 'mobx'; import BaseStore from 'models/base_store'; import { makeRequest } from 'network'; @@ -23,10 +23,7 @@ export class LabelStore extends BaseStore { @action.bound public async loadKeys() { const { data } = await onCallApi.GET('/labels/keys/', undefined); - - runInAction(() => { - this.keys = data; - }); + this.keys = data; return data; } diff --git a/grafana-plugin/src/models/organization/organization.ts b/grafana-plugin/src/models/organization/organization.ts index 0c2ac3dd..024cf392 100644 --- a/grafana-plugin/src/models/organization/organization.ts +++ b/grafana-plugin/src/models/organization/organization.ts @@ -15,9 +15,10 @@ export class OrganizationStore extends BaseStore { this.path = '/organization/'; } - @action + @action.bound async loadCurrentOrganization() { - this.currentOrganization = await makeRequest(this.path, {}); + const organization = await makeRequest(this.path, {}); + this.currentOrganization = organization; } @action diff --git a/grafana-plugin/src/models/outgoing_webhook/outgoing_webhook.ts b/grafana-plugin/src/models/outgoing_webhook/outgoing_webhook.ts index 296771de..9e869374 100644 --- a/grafana-plugin/src/models/outgoing_webhook/outgoing_webhook.ts +++ b/grafana-plugin/src/models/outgoing_webhook/outgoing_webhook.ts @@ -101,8 +101,8 @@ export class OutgoingWebhookStore extends BaseStore { }); } - @action - async updateOutgoingWebhookPresets() { + @action.bound + async updateOutgoingWebhookPresetsOptions() { const response = await makeRequest(`/webhooks/preset_options/`, {}); this.outgoingWebhookPresets = response; } diff --git a/grafana-plugin/src/models/user/user.ts b/grafana-plugin/src/models/user/user.ts index a3d10505..1d6f18f2 100644 --- a/grafana-plugin/src/models/user/user.ts +++ b/grafana-plugin/src/models/user/user.ts @@ -62,14 +62,12 @@ export class UserStore extends BaseStore { @action async loadCurrentUser() { const response = await makeRequest('/user/', {}); - const timezone = await this.refreshTimezone(response.pk); this.items = { ...this.items, [response.pk]: { ...response, timezone }, }; - this.currentUserPk = response.pk; } @@ -164,7 +162,7 @@ export class UserStore extends BaseStore { return { page_size: this.searchResult.page_size, count: this.searchResult.count, - results: this.searchResult.results && this.searchResult.results.map((userPk: User['pk']) => this.items?.[userPk]), + results: this.searchResult.results?.map((userPk: User['pk']) => this.items?.[userPk]), }; } @@ -371,12 +369,11 @@ export class UserStore extends BaseStore { this.updateItem(userPk); // to update notification_chain_verbal } - @action + @action.bound async updateNotificationPolicyOptions() { const response = await makeRequest('/notification_policies/', { method: 'OPTIONS', }); - this.notificationChoices = get(response, 'actions.POST', []); } @@ -390,10 +387,9 @@ export class UserStore extends BaseStore { }); } - @action + @action.bound async updateNotifyByOptions() { const response = await makeRequest('/notification_policies/notify_by_options/', {}); - this.notifyByOptions = response; } diff --git a/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx b/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx index 448f3406..99afa405 100644 --- a/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx +++ b/grafana-plugin/src/pages/outgoing_webhooks/OutgoingWebhooks.tsx @@ -64,6 +64,10 @@ class OutgoingWebhooks extends React.Component { }; export const Root = observer((props: AppRootProps) => { - const store = useStore(); - - const [basicDataLoaded, setBasicDataLoaded] = useState(false); + const { isBasicDataLoaded, loadBasicData, loadMasterData } = useStore(); const [pageTitle, setPageTitle] = useState(''); - useEffect(() => { - runQueuedUpdateData(0); - }, []); - const location = useLocation(); + useEffect(() => { + loadBasicData(); + // defer loading master data as it's not used in first sec by user in order to prioritize fetching base data + const timeout = setTimeout(() => { + loadMasterData(); + }, 1000); + + return () => clearTimeout(timeout); + }, []); + useEffect(() => { let link = document.createElement('link'); link.type = 'text/css'; @@ -109,6 +114,10 @@ export const Root = observer((props: AppRootProps) => { return (pages[page] || pages[DEFAULT_PAGE]).getPageNav(pageTitle); }; + if (!userHasAccess) { + return ; + } + return ( {!isTopNavbar() && ( @@ -124,11 +133,14 @@ export const Root = observer((props: AppRootProps) => { 'page-body': !isTopNavbar(), })} > - {userHasAccess ? ( - // Otherwise we'll run into concurrency issues - !basicDataLoaded ? ( - - ) : ( + } + > + } + > @@ -182,7 +194,7 @@ export const Root = observer((props: AppRootProps) => { }} > )} - > + /> { }} > )} - > - + /> - ) - ) : ( - - )} + + ); - - async function runQueuedUpdateData(attemptCount: number) { - if (attemptCount === 10) { - return; - } - - try { - await store.updateBasicData(); - setBasicDataLoaded(true); - } catch { - setTimeout(() => runQueuedUpdateData(attemptCount + 1), 1000); - } - } }); diff --git a/grafana-plugin/src/state/rootBaseStore/index.ts b/grafana-plugin/src/state/rootBaseStore/index.ts index 44cc0f91..76d20346 100644 --- a/grafana-plugin/src/state/rootBaseStore/index.ts +++ b/grafana-plugin/src/state/rootBaseStore/index.ts @@ -33,6 +33,7 @@ import { UserGroupStore } from 'models/user_group/user_group'; import { makeRequest } from 'network'; import { AppFeature } from 'state/features'; import PluginState from 'state/plugin'; +import { retryFailingPromises } from 'utils/async'; import { APP_VERSION, CLOUD_VERSION_REGEX, @@ -45,6 +46,9 @@ import FaroHelper from 'utils/faro'; // ------ Dashboard ------ // export class RootBaseStore { + @observable + isBasicDataLoaded = false; + @observable currentTimezone: Timezone = moment.tz.guess() as Timezone; @@ -83,7 +87,7 @@ export class RootBaseStore { @observable onCallApiUrl: string; - // -------------------------- + // stores userStore = new UserStore(this); cloudStore = new CloudStore(this); directPagingStore = new DirectPagingStore(this); @@ -108,9 +112,8 @@ export class RootBaseStore { labelsStore = new LabelStore(this); loaderStore = LoaderStore; - // stores - - async updateBasicData() { + @action.bound + async loadBasicData() { const updateFeatures = async () => { await this.updateFeatures(); @@ -121,18 +124,21 @@ export class RootBaseStore { } }; - return Promise.all([ - this.userStore.loadCurrentUser(), - this.organizationStore.loadCurrentOrganization(), - this.grafanaTeamStore.updateItems(), - updateFeatures(), + await retryFailingPromises([ + this.userStore.loadCurrentUser, + this.organizationStore.loadCurrentOrganization, + this.grafanaTeamStore.updateItems, + updateFeatures, + ]); + this.isBasicDataLoaded = true; + } + + @action.bound + async loadMasterData() { + Promise.all([ this.userStore.updateNotificationPolicyOptions(), this.userStore.updateNotifyByOptions(), this.alertReceiveChannelStore.updateAlertReceiveChannelOptions(), - this.outgoingWebhookStore.updateOutgoingWebhookPresets(), - this.escalationPolicyStore.updateWebEscalationPolicyOptions(), - this.escalationPolicyStore.updateEscalationPolicyOptions(), - this.escalationPolicyStore.updateNumMinutesInWindowOptions(), ]); } @@ -282,7 +288,7 @@ export class RootBaseStore { return this.license === GRAFANA_LICENSE_OSS; } - @observable + @action.bound async updateFeatures() { const response = await makeRequest('/features/', {}); this.features = response.reduce( diff --git a/grafana-plugin/src/utils/async.test.ts b/grafana-plugin/src/utils/async.test.ts new file mode 100644 index 00000000..5c943f66 --- /dev/null +++ b/grafana-plugin/src/utils/async.test.ts @@ -0,0 +1,33 @@ +import { retryFailingPromises } from './async'; + +describe('retryFailingPromises', () => { + it('should retry only failing promises X times and return correct result', async () => { + const MAX_ATTEMPTS = 5; + + // We mimic that fetch1 always resolves, fetch2 always rejects and fetch3 resolves only on 2nd attempt + let attempts1 = 0; + let attempts2 = 0; + let attempts3 = 0; + const fetch1 = async () => Promise.resolve(++attempts1); + const fetch2 = async () => Promise.reject(++attempts2); + const fetch3 = async () => + new Promise((resolve, reject) => { + attempts3++; + if (attempts3 === 2) { + resolve(attempts3); + } + reject(attempts3); + }); + + const result = await retryFailingPromises([fetch1, fetch2, fetch3], { maxAttempts: MAX_ATTEMPTS, delayInMs: 50 }); + + expect(attempts1).toBe(1); + expect(attempts2).toBe(MAX_ATTEMPTS); + expect(attempts3).toBe(2); + expect(result).toEqual([ + { status: 'fulfilled', value: 1 }, + { status: 'rejected', reason: 5 }, + { status: 'fulfilled', value: 2 }, + ]); + }); +}); diff --git a/grafana-plugin/src/utils/async.ts b/grafana-plugin/src/utils/async.ts new file mode 100644 index 00000000..0ebafde1 --- /dev/null +++ b/grafana-plugin/src/utils/async.ts @@ -0,0 +1,9 @@ +import { retry } from '@lifeomic/attempt'; + +export const retryFailingPromises = async ( + asyncActions: Array<() => Promise>, + { maxAttempts = 3, delayInMs = 500 }: { maxAttempts?: number; delayInMs?: number } = {} +) => + maxAttempts === 0 + ? Promise.allSettled(asyncActions) + : Promise.allSettled(asyncActions.map((asyncAction) => retry(asyncAction, { maxAttempts, delay: delayInMs }))); diff --git a/grafana-plugin/yarn.lock b/grafana-plugin/yarn.lock index c2da9267..3aa546d3 100644 --- a/grafana-plugin/yarn.lock +++ b/grafana-plugin/yarn.lock @@ -2863,6 +2863,11 @@ resolved "https://registry.yarnpkg.com/@leeoniya/ufuzzy/-/ufuzzy-1.0.8.tgz#6a01b561749df84ff28637051865fdde3cbfc3a9" integrity sha512-HQ6aJlYpWLq1f9AiApJl0aOIXlJUtuhBOYfSfv5rt3XNYkCBveojtnL6FvOVpJ2gEJ2wqgMW8xOHkLVYAbXghg== +"@lifeomic/attempt@^3.0.3": + version "3.0.3" + resolved "https://registry.yarnpkg.com/@lifeomic/attempt/-/attempt-3.0.3.tgz#e742a5b85eb673e2f1746b0f39cb932cbc6145bb" + integrity sha512-GlM2AbzrErd/TmLL3E8hAHmb5Q7VhDJp35vIbyPVA5Rz55LZuRr8pwL3qrwwkVNo05gMX1J44gURKb4MHQZo7w== + "@mapbox/jsonlint-lines-primitives@~2.0.2": version "2.0.2" resolved "https://registry.yarnpkg.com/@mapbox/jsonlint-lines-primitives/-/jsonlint-lines-primitives-2.0.2.tgz#ce56e539f83552b58d10d672ea4d6fc9adc7b234"