Skip to content

Commit

Permalink
[SLOs] Try async creation of resources !! (elastic#192836)
Browse files Browse the repository at this point in the history 
## Summary

Try async as much as possible while creating SLO !!

Also UI form won't wait now for creating burn rate rule, it will be
created async loading state in toast !!
### Before


![image](https://github.com/user-attachments/assets/dc73ce58-3fc4-475f-a6ae-71ac98a6432c)

### After


![image](https://github.com/user-attachments/assets/4439bb8b-6768-4d8b-b208-1bad28f17ae5)
  • Loading branch information
@shahzad31
shahzad31 authored Oct 1, 2024
1 parent c8c7439 commit 9e117c3
Show file tree
Hide file tree
Showing 18 changed files with 220 additions and 139 deletions.
35 changes: 31 additions & 4 deletions ...ution/slo/public/hooks/use_create_rule.ts → ...ublic/hooks/use_create_burn_rate_rule.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import React from 'react';
import { useMutation } from '@tanstack/react-query';
import { i18n } from '@kbn/i18n';
import { BASE_ALERTING_API_PATH, RuleTypeParams } from '@kbn/alerting-plugin/common';
Expand All @@ -13,18 +14,23 @@ import type {
CreateRuleRequestBody,
CreateRuleResponse,
} from '@kbn/alerting-plugin/common/routes/rule/apis/create';
import { EuiFlexGroup, EuiFlexItem, EuiLoadingSpinner } from '@elastic/eui';
import { toMountPoint } from '@kbn/react-kibana-mount';
import { useKibana } from '../utils/kibana_react';

export function useCreateRule<Params extends RuleTypeParams = never>() {
const {
http,
i18n: i18nStart,
notifications: { toasts },
theme,
} = useKibana().services;

const createRule = useMutation<
return useMutation<
CreateRuleResponse<Params>,
Error,
{ rule: CreateRuleRequestBody<Params> }
{ rule: CreateRuleRequestBody<Params> },
{ loadingToastId?: string }
>(
['createRule'],
({ rule }) => {
Expand All @@ -39,6 +45,24 @@ export function useCreateRule<Params extends RuleTypeParams = never>() {
}
},
{
onMutate: async () => {
const loadingToast = toasts.addInfo({
title: toMountPoint(
<EuiFlexGroup justifyContent="center" alignItems="center">
<EuiFlexItem grow={false}>
{i18n.translate('xpack.slo.rules.createRule.loadingNotification.descriptionText', {
defaultMessage: 'Creating burn rate rule ...',
})}
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiLoadingSpinner size="s" />
</EuiFlexItem>
</EuiFlexGroup>,
{ i18n: i18nStart, theme }
),
});
return { loadingToastId: loadingToast.id };
},
onError: (_err) => {
toasts.addDanger(
i18n.translate('xpack.slo.rules.createRule.errorNotification.descriptionText', {
Expand All @@ -54,8 +78,11 @@ export function useCreateRule<Params extends RuleTypeParams = never>() {
})
);
},
onSettled: (_d, _err, _res, ctx) => {
if (ctx?.loadingToastId) {
toasts.remove(ctx?.loadingToastId);
}
},
}
);

return createRule;
}
2 changes: 1 addition & 1 deletion x-pack/plugins/observability_solution/slo/public/hooks/use_create_slo.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export function useCreateSlo() {
<RedirectAppLinks coreStart={services} data-test-subj="observabilityMainContainer">
<FormattedMessage
id="xpack.slo.slo.create.successNotification"
defaultMessage="Successfully created {name}. {editSLO}"
defaultMessage='Successfully created SLO: "{name}". {editSLO}'
values={{
name: slo.name,
editSLO: (
Expand Down
8 changes: 3 additions & 5 deletions ...gins/observability_solution/slo/public/pages/slo_edit/components/slo_edit_form_footer.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type { GetSLOResponse } from '@kbn/slo-schema';
import React, { useCallback, useMemo } from 'react';
import { useFormContext } from 'react-hook-form';
import { InPortal } from 'react-reverse-portal';
import { useCreateRule } from '../../../hooks/use_create_rule';
import { useCreateRule } from '../../../hooks/use_create_burn_rate_rule';
import { useKibana } from '../../../utils/kibana_react';
import { sloEditFormFooterPortal } from '../shared_flyout/slo_add_form_flyout';
import { paths } from '../../../../common/locators/paths';
Expand All @@ -32,8 +32,6 @@ export interface Props {
onSave?: () => void;
}

export const maxWidth = 775;

export function SloEditFormFooter({ slo, onSave }: Props) {
const {
application: { navigateToUrl },
Expand All @@ -45,7 +43,7 @@ export function SloEditFormFooter({ slo, onSave }: Props) {

const { mutateAsync: createSlo, isLoading: isCreateSloLoading } = useCreateSlo();
const { mutateAsync: updateSlo, isLoading: isUpdateSloLoading } = useUpdateSlo();
const { mutateAsync: createBurnRateRule, isLoading: isCreateBurnRateRuleLoading } =
const { mutate: createBurnRateRule, isLoading: isCreateBurnRateRuleLoading } =
useCreateRule<BurnRateRuleParams>();

const navigate = useCallback(
Expand All @@ -70,7 +68,7 @@ export function SloEditFormFooter({ slo, onSave }: Props) {
} else {
const processedValues = transformCreateSLOFormToCreateSLOInput(values);
const resp = await createSlo({ slo: processedValues });
await createBurnRateRule({
createBurnRateRule({
rule: createBurnRateRuleRequestBody({ ...processedValues, id: resp.id }),
});
if (onSave) {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/observability_solution/slo/public/pages/slo_edit/slo_edit.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { useFetchApmSuggestions } from '../../hooks/use_fetch_apm_suggestions';
import { useFetchIndices } from '../../hooks/use_fetch_indices';
import { useFetchSloDetails } from '../../hooks/use_fetch_slo_details';
import { usePermissions } from '../../hooks/use_permissions';
import { useCreateRule } from '../../hooks/use_create_rule';
import { useCreateRule } from '../../hooks/use_create_burn_rate_rule';
import { useUpdateSlo } from '../../hooks/use_update_slo';
import { useKibana } from '../../utils/kibana_react';
import { kibanaStartMock } from '../../utils/kibana_react.mock';
Expand All @@ -45,7 +45,7 @@ jest.mock('../../hooks/use_create_slo');
jest.mock('../../hooks/use_update_slo');
jest.mock('../../hooks/use_fetch_apm_suggestions');
jest.mock('../../hooks/use_permissions');
jest.mock('../../hooks/use_create_rule');
jest.mock('../../hooks/use_create_burn_rate_rule');

const mockUseKibanaReturnValue = kibanaStartMock.startContract();

Expand Down
20 changes: 12 additions & 8 deletions x-pack/plugins/observability_solution/slo/server/routes/slo/route.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import {
updateSLOParamsSchema,
} from '@kbn/slo-schema';
import { getOverviewParamsSchema } from '@kbn/slo-schema/src/rest_specs/routes/get_overview';
import { KibanaRequest } from '@kbn/core-http-server';
import { RegisterRoutesDependencies } from '../register_routes';
import { GetSLOsOverview } from '../../services/get_slos_overview';
import type { IndicatorTypes } from '../../domain/models';
import {
Expand Down Expand Up @@ -91,6 +93,11 @@ const assertPlatinumLicense = async (context: SloRequestHandlerContext) => {
}
};

const getSpaceId = async (deps: RegisterRoutesDependencies, request: KibanaRequest) => {
const spaces = await deps.getSpacesStart();
return (await spaces?.spacesService?.getActiveSpace(request))?.id ?? 'default';
};

const createSLORoute = createSloServerRoute({
endpoint: 'POST /api/observability/slos 2023-10-31',
options: {
Expand All @@ -101,18 +108,18 @@ const createSLORoute = createSloServerRoute({
handler: async ({ context, params, logger, dependencies, request }) => {
await assertPlatinumLicense(context);

const spaces = await dependencies.getSpacesStart();
const dataViews = await dependencies.getDataViewsStart();
const spaceId = (await spaces?.spacesService?.getActiveSpace(request))?.id ?? 'default';

const core = await context.core;
const scopedClusterClient = core.elasticsearch.client;
const esClient = core.elasticsearch.client.asCurrentUser;
const basePath = dependencies.pluginsSetup.core.http.basePath;
const soClient = core.savedObjects.client;
const repository = new KibanaSavedObjectsSLORepository(soClient, logger);

const dataViewsService = await dataViews.dataViewsServiceFactory(soClient, esClient);
const [spaceId, dataViewsService] = await Promise.all([
getSpaceId(dependencies, request),
dataViews.dataViewsServiceFactory(soClient, esClient),
]);
const transformManager = new DefaultTransformManager(
transformGenerators,
scopedClusterClient,
Expand All @@ -125,7 +132,6 @@ const createSLORoute = createSloServerRoute({
scopedClusterClient,
logger
);

const createSLO = new CreateSLO(
esClient,
scopedClusterClient,
Expand All @@ -137,9 +143,7 @@ const createSLORoute = createSloServerRoute({
basePath
);

const response = await createSLO.execute(params.body);

return response;
return await createSLO.execute(params.body);
},
});

Expand Down
37 changes: 16 additions & 21 deletions x-pack/plugins/observability_solution/slo/server/services/create_slo.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('CreateSLO', () => {

const response = await createSLO.execute(sloParams);

expect(mockRepository.save).toHaveBeenCalledWith(
expect(mockRepository.create).toHaveBeenCalledWith(
expect.objectContaining({
...sloParams,
id: 'unique-id',
Expand All @@ -80,17 +80,14 @@ describe('CreateSLO', () => {
version: 2,
createdAt: expect.any(Date),
updatedAt: expect.any(Date),
}),
{ throwOnConflict: true }
})
);

expect(mockTransformManager.install).toHaveBeenCalled();
expect(mockTransformManager.start).toHaveBeenCalled();
expect(
mockScopedClusterClient.asSecondaryAuthUser.ingest.putPipeline.mock.calls[0]
).toMatchSnapshot();
expect(mockSummaryTransformManager.install).toHaveBeenCalled();
expect(mockSummaryTransformManager.start).toHaveBeenCalled();
expect(mockEsClient.index.mock.calls[0]).toMatchSnapshot();

expect(response).toEqual(expect.objectContaining({ id: 'unique-id' }));
Expand All @@ -108,7 +105,7 @@ describe('CreateSLO', () => {

await createSLO.execute(sloParams);

expect(mockRepository.save).toHaveBeenCalledWith(
expect(mockRepository.create).toHaveBeenCalledWith(
expect.objectContaining({
...sloParams,
id: expect.any(String),
Expand All @@ -122,8 +119,7 @@ describe('CreateSLO', () => {
enabled: true,
createdAt: expect.any(Date),
updatedAt: expect.any(Date),
}),
{ throwOnConflict: true }
})
);
});

Expand All @@ -141,7 +137,7 @@ describe('CreateSLO', () => {

await createSLO.execute(sloParams);

expect(mockRepository.save).toHaveBeenCalledWith(
expect(mockRepository.create).toHaveBeenCalledWith(
expect.objectContaining({
...sloParams,
id: expect.any(String),
Expand All @@ -155,8 +151,7 @@ describe('CreateSLO', () => {
enabled: true,
createdAt: expect.any(Date),
updatedAt: expect.any(Date),
}),
{ throwOnConflict: true }
})
);
});
});
Expand All @@ -173,16 +168,16 @@ describe('CreateSLO', () => {
expect(mockRepository.deleteById).toHaveBeenCalled();
expect(
mockScopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline
).toHaveBeenCalledTimes(1);
).toHaveBeenCalledTimes(2);

expect(mockSummaryTransformManager.stop).not.toHaveBeenCalled();
expect(mockSummaryTransformManager.uninstall).not.toHaveBeenCalled();
expect(mockTransformManager.stop).not.toHaveBeenCalled();
expect(mockTransformManager.uninstall).not.toHaveBeenCalled();
expect(mockSummaryTransformManager.stop).toHaveBeenCalledTimes(0);
expect(mockSummaryTransformManager.uninstall).toHaveBeenCalledTimes(1);
expect(mockTransformManager.stop).toHaveBeenCalledTimes(0);
expect(mockTransformManager.uninstall).toHaveBeenCalledTimes(1);
});

it('rollbacks completed operations when summary transform start fails', async () => {
mockSummaryTransformManager.start.mockRejectedValue(
it('rollbacks completed operations when summary transform install fails', async () => {
mockSummaryTransformManager.install.mockRejectedValue(
new Error('Summary transform install error')
);
const sloParams = createSLOParams({ indicator: createAPMTransactionErrorRateIndicator() });
Expand All @@ -192,7 +187,7 @@ describe('CreateSLO', () => {
);

expect(mockRepository.deleteById).toHaveBeenCalled();
expect(mockTransformManager.stop).toHaveBeenCalled();
expect(mockTransformManager.stop).not.toHaveBeenCalled();
expect(mockTransformManager.uninstall).toHaveBeenCalled();
expect(
mockScopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline
Expand All @@ -211,12 +206,12 @@ describe('CreateSLO', () => {
);

expect(mockRepository.deleteById).toHaveBeenCalled();
expect(mockTransformManager.stop).toHaveBeenCalled();
expect(mockTransformManager.stop).not.toHaveBeenCalled();
expect(mockTransformManager.uninstall).toHaveBeenCalled();
expect(
mockScopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline
).toHaveBeenCalledTimes(2);
expect(mockSummaryTransformManager.stop).toHaveBeenCalled();
expect(mockSummaryTransformManager.stop).not.toHaveBeenCalled();
expect(mockSummaryTransformManager.uninstall).toHaveBeenCalled();
});
});
Expand Down
Loading

0 comments on commit 9e117c3

Please sign in to comment.