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.

Sign up for GitHub

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

[SLOs] Fix cloning SLO by opening pre filled form #172927

Merged
merged 5 commits into from
Dec 18, 2023
Merged

[SLOs] Fix cloning SLO by opening pre filled form #172927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f1cabe
clone slo
shahzad31 Dec 11, 2023
2783949
Merge branch 'main' into slo-card-clone
shahzad31 Dec 12, 2023
094330f
Merge branch 'main' into slo-card-clone
shahzad31 Dec 12, 2023
5ff095c
Merge branch 'main' into slo-card-clone
shahzad31 Dec 14, 2023
b26412c
Merge branch 'main' into slo-card-clone
shahzad31 Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion x-pack/plugins/observability/public/embeddable/slo/overview/slo_overview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export function SloOverview({

return (
<div ref={containerRef} style={{ width: '100%' }}>
<SloCardChart slo={slo} historicalSliData={historicalSliData ?? []} cardsPerRow={4} />
<SloCardChart slo={slo} historicalSliData={historicalSliData ?? []} />
<SloCardBadgesPortal containerRef={containerRef}>
<SloCardItemBadges
slo={slo}
Expand Down
87 changes: 16 additions & 71 deletions x-pack/plugins/observability/public/hooks/slo/use_clone_slo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,83 +5,28 @@
* 2.0.
*/

import { IHttpFetchError, ResponseErrorBody } from '@kbn/core/public';
import { i18n } from '@kbn/i18n';
import type { CreateSLOInput, CreateSLOResponse, FindSLOResponse } from '@kbn/slo-schema';
import { QueryKey, useMutation, useQueryClient } from '@tanstack/react-query';
import { v4 as uuidv4 } from 'uuid';
import { encode } from '@kbn/rison';
import { useCallback } from 'react';
import { SLOWithSummaryResponse } from '@kbn/slo-schema';
import { paths } from '../../../common/locators/paths';
import { useKibana } from '../../utils/kibana_react';
import { sloKeys } from './query_key_factory';

type ServerError = IHttpFetchError<ResponseErrorBody>;

export function useCloneSlo() {
const {
http,
notifications: { toasts },
http: { basePath },
application: { navigateToUrl },
} = useKibana().services;
const queryClient = useQueryClient();

return useMutation<
CreateSLOResponse,
ServerError,
{ slo: CreateSLOInput; originalSloId?: string },
{ previousData?: FindSLOResponse; queryKey?: QueryKey }
>(
['cloneSlo'],
({ slo }: { slo: CreateSLOInput; originalSloId?: string }) => {
const body = JSON.stringify(slo);
return http.post<CreateSLOResponse>(`/api/observability/slos`, { body });
return useCallback(
(slo: SLOWithSummaryResponse) => {
navigateToUrl(
basePath.prepend(
paths.observability.sloCreateWithEncodedForm(
encode({ ...slo, name: `[Copy] ${slo.name}`, id: undefined })
Copy link
Contributor

Choose a reason for hiding this comment

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

@shahzad31 Just out of curiosity in the previous implementation we were using id: uuidv4(). I guess we are calling uuidv4() when clicking on the Create SLO button, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah because we were actually creating an SLO in memory. In new implementation we are just redirecting user to the pre filled form.

)
)
);
},
{
onMutate: async ({ slo, originalSloId }) => {
await queryClient.cancelQueries({ queryKey: sloKeys.lists(), exact: false });

const queriesData = queryClient.getQueriesData<FindSLOResponse>({
queryKey: sloKeys.lists(),
exact: false,
});
const [queryKey, previousData] = queriesData?.at(0) ?? [];

const originalSlo = previousData?.results?.find((el) => el.id === originalSloId);
const optimisticUpdate = {
page: previousData?.page ?? 1,
perPage: previousData?.perPage ?? 25,
total: previousData?.total ? previousData.total + 1 : 1,
results: [
...(previousData?.results ?? []),
{ ...originalSlo, name: slo.name, id: uuidv4(), summary: undefined },
],
};

if (queryKey) {
queryClient.setQueryData(queryKey, optimisticUpdate);
}

return { queryKey, previousData };
},
// If the mutation fails, use the context returned from onMutate to roll back
onError: (error, { slo }, context) => {
if (context?.previousData && context?.queryKey) {
queryClient.setQueryData(context.queryKey, context.previousData);
}

toasts.addError(new Error(error.body?.message ?? error.message), {
title: i18n.translate('xpack.observability.slo.clone.errorNotification', {
defaultMessage: 'Failed to clone {name}',
values: { name: slo.name },
}),
});
},
onSuccess: (_data, { slo }) => {
toasts.addSuccess(
i18n.translate('xpack.observability.slo.clone.successNotification', {
defaultMessage: 'Successfully created {name}',
values: { name: slo.name },
})
);
queryClient.invalidateQueries({ queryKey: sloKeys.lists(), exact: false });
},
}
[navigateToUrl, basePath]
);
}
18 changes: 4 additions & 14 deletions x-pack/plugins/observability/public/pages/slo_details/components/header_control.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,16 @@ import { i18n } from '@kbn/i18n';
import { EuiButton, EuiContextMenuItem, EuiContextMenuPanel, EuiPopover } from '@elastic/eui';
import { SLOWithSummaryResponse } from '@kbn/slo-schema';

import { useCloneSlo } from '../../../hooks/slo/use_clone_slo';
import { SloDeleteConfirmationModal } from '../../../components/slo/delete_confirmation_modal/slo_delete_confirmation_modal';
import { useCapabilities } from '../../../hooks/slo/use_capabilities';
import { useKibana } from '../../../utils/kibana_react';
import { useCloneSlo } from '../../../hooks/slo/use_clone_slo';
import { useDeleteSlo } from '../../../hooks/slo/use_delete_slo';
import { isApmIndicatorType } from '../../../utils/slo/indicator';
import { convertSliApmParamsToApmAppDeeplinkUrl } from '../../../utils/slo/convert_sli_apm_params_to_apm_app_deeplink_url';
import { SLO_BURN_RATE_RULE_TYPE_ID } from '../../../../common/constants';
import { rulesLocatorID, sloFeatureId } from '../../../../common';
import { paths } from '../../../../common/locators/paths';
import {
transformSloResponseToCreateSloForm,
transformCreateSLOFormToCreateSLOInput,
} from '../../slo_edit/helpers/process_slo_form_values';
import type { RulesParams } from '../../../locators/rules';

export interface Props {
Expand All @@ -47,7 +43,6 @@ export function HeaderControl({ isLoading, slo }: Props) {
const [isRuleFlyoutVisible, setRuleFlyoutVisibility] = useState<boolean>(false);
const [isDeleteConfirmationModalOpen, setDeleteConfirmationModalOpen] = useState(false);

const { mutate: cloneSlo } = useCloneSlo();
const { mutate: deleteSlo } = useDeleteSlo();

const handleActionsClick = () => setIsPopoverOpen((value) => !value);
Expand Down Expand Up @@ -101,17 +96,12 @@ export function HeaderControl({ isLoading, slo }: Props) {
}
};

const navigateToClone = useCloneSlo();

const handleClone = async () => {
if (slo) {
setIsPopoverOpen(false);

const newSlo = transformCreateSLOFormToCreateSLOInput(
transformSloResponseToCreateSloForm({ ...slo, name: `[Copy] ${slo.name}` })!
);

cloneSlo({ slo: newSlo, originalSloId: slo.id });

navigate(basePath.prepend(paths.observability.slos));
navigateToClone(slo);
}
};

Expand Down
33 changes: 6 additions & 27 deletions x-pack/plugins/observability/public/pages/slo_details/slo_details.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { useFetchSloDetails } from '../../hooks/slo/use_fetch_slo_details';
import { useFetchHistoricalSummary } from '../../hooks/slo/use_fetch_historical_summary';
import { useFetchActiveAlerts } from '../../hooks/slo/use_fetch_active_alerts';
import { ActiveAlerts } from '../../hooks/slo/active_alerts';
import { useCloneSlo } from '../../hooks/slo/use_clone_slo';
import { useDeleteSlo } from '../../hooks/slo/use_delete_slo';
import { render } from '../../utils/test_helper';
import { SloDetailsPage } from './slo_details';
Expand All @@ -30,6 +29,7 @@ import {
import { chartPluginMock } from '@kbn/charts-plugin/public/mocks';
import { buildApmAvailabilityIndicator } from '../../data/slo/indicator';
import { ALL_VALUE } from '@kbn/slo-schema';
import { encode } from '@kbn/rison';

jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
Expand All @@ -44,7 +44,6 @@ jest.mock('../../hooks/slo/use_capabilities');
jest.mock('../../hooks/slo/use_fetch_active_alerts');
jest.mock('../../hooks/slo/use_fetch_slo_details');
jest.mock('../../hooks/slo/use_fetch_historical_summary');
jest.mock('../../hooks/slo/use_clone_slo');
jest.mock('../../hooks/slo/use_delete_slo');

const useKibanaMock = useKibana as jest.Mock;
Expand All @@ -55,12 +54,10 @@ const useCapabilitiesMock = useCapabilities as jest.Mock;
const useFetchActiveAlertsMock = useFetchActiveAlerts as jest.Mock;
const useFetchSloDetailsMock = useFetchSloDetails as jest.Mock;
const useFetchHistoricalSummaryMock = useFetchHistoricalSummary as jest.Mock;
const useCloneSloMock = useCloneSlo as jest.Mock;
const useDeleteSloMock = useDeleteSlo as jest.Mock;

const mockNavigate = jest.fn();
const mockLocator = jest.fn();
const mockClone = jest.fn();
const mockDelete = jest.fn();
const mockCapabilities = {
apm: { show: true },
Expand Down Expand Up @@ -120,7 +117,6 @@ describe('SLO Details Page', () => {
data: historicalSummaryData,
});
useFetchActiveAlertsMock.mockReturnValue({ isLoading: false, data: new ActiveAlerts() });
useCloneSloMock.mockReturnValue({ mutate: mockClone });
useDeleteSloMock.mockReturnValue({ mutate: mockDelete });
useLocationMock.mockReturnValue({ search: '' });
});
Expand Down Expand Up @@ -248,29 +244,12 @@ describe('SLO Details Page', () => {

fireEvent.click(button!);

const {
id,
createdAt,
enabled,
revision,
summary,
settings,
updatedAt,
instanceId,
version,
...newSlo
} = slo;

expect(mockClone).toBeCalledWith({
originalSloId: slo.id,
slo: {
...newSlo,
name: `[Copy] ${newSlo.name}`,
},
});

await waitFor(() => {
expect(mockNavigate).toBeCalledWith(paths.observability.slos);
expect(mockNavigate).toBeCalledWith(
paths.observability.sloCreateWithEncodedForm(
encode({ ...slo, name: `[Copy] ${slo.name}`, id: undefined })
)
);
});
});

Expand Down
24 changes: 6 additions & 18 deletions x-pack/plugins/observability/public/pages/slo_edit/components/slo_edit_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ import { i18n } from '@kbn/i18n';
import type { GetSLOResponse } from '@kbn/slo-schema';
import React, { useCallback, useEffect, useState } from 'react';
import { FormProvider, useForm } from 'react-hook-form';
import { sloFeatureId } from '../../../../common';
import { SLO_BURN_RATE_RULE_TYPE_ID } from '../../../../common/constants';
import { BurnRateRuleFlyout } from '../../slos/components/common/burn_rate_rule_flyout';
import { paths } from '../../../../common/locators/paths';
import { useCreateSlo } from '../../../hooks/slo/use_create_slo';
import { useFetchRulesForSlo } from '../../../hooks/slo/use_fetch_rules_for_slo';
Expand Down Expand Up @@ -54,7 +53,6 @@ export function SloEditForm({ slo }: Props) {
const {
application: { navigateToUrl },
http: { basePath },
triggersActionsUi: { getAddRuleFlyout: AddRuleFlyout },
} = useKibana().services;

const isEditMode = slo !== undefined;
Expand Down Expand Up @@ -146,10 +144,6 @@ export function SloEditForm({ slo }: Props) {
setIsCreateRuleCheckboxChecked(!isCreateRuleCheckboxChecked);
};

const handleCloseRuleFlyout = async () => {
navigateToUrl(basePath.prepend(paths.observability.slos));
};

return (
<>
<FormProvider {...methods}>
Expand Down Expand Up @@ -256,17 +250,11 @@ export function SloEditForm({ slo }: Props) {
</EuiFlexGroup>
</FormProvider>

{isAddRuleFlyoutOpen && slo ? (
<AddRuleFlyout
canChangeTrigger={false}
consumer={sloFeatureId}
initialValues={{ name: `${watch('name')} burn rate rule`, params: { sloId: slo.id } }}
ruleTypeId={SLO_BURN_RATE_RULE_TYPE_ID}
onClose={handleCloseRuleFlyout}
onSave={handleCloseRuleFlyout}
useRuleProducer
/>
) : null}
<BurnRateRuleFlyout
slo={slo as GetSLOResponse}
isAddRuleFlyoutOpen={isAddRuleFlyoutOpen}
canChangeTrigger={false}
/>
</>
);
}
6 changes: 1 addition & 5 deletions x-pack/plugins/observability/public/pages/slo_edit/slo_edit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function SloEditPage() {
const { sloId } = useParams<{ sloId: string | undefined }>();
const { hasAtLeast } = useLicense();
const hasRightLicense = hasAtLeast('platinum');
const { data: slo, isInitialLoading } = useFetchSloDetails({ sloId });
Copy link
Contributor

Choose a reason for hiding this comment

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

@shahzad31 Why is this not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was un-necessarily blocking rendering of the form, causing empty page splash before form opens

const { data: slo } = useFetchSloDetails({ sloId });

useBreadcrumbs([
{
Expand Down Expand Up @@ -66,10 +66,6 @@ export function SloEditPage() {
navigateToUrl(basePath.prepend(paths.observability.slos));
}

if (sloId && isInitialLoading) {
return null;
}

return (
<ObservabilityPageTemplate
pageHeader={{
Expand Down
19 changes: 9 additions & 10 deletions x-pack/plugins/observability/public/pages/slos/components/card_view/slo_card_item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { EuiIcon, EuiPanel, useEuiBackgroundColor } from '@elastic/eui';
import { ALL_VALUE, HistoricalSummaryResponse, SLOWithSummaryResponse } from '@kbn/slo-schema';
import { Rule } from '@kbn/triggers-actions-ui-plugin/public';
import { i18n } from '@kbn/i18n';
import { css } from '@emotion/react';
import { SloCardBadgesPortal } from './badges_portal';
import { useSloListActions } from '../../hooks/use_slo_list_actions';
import { BurnRateRuleFlyout } from '../common/burn_rate_rule_flyout';
Expand Down Expand Up @@ -52,7 +53,7 @@ export const useSloCardColor = (status?: SLOWithSummaryResponse['summary']['stat
return colors[status ?? 'NO_DATA'];
};

const getSubTitle = (slo: SLOWithSummaryResponse, cardsPerRow: number) => {
const getSubTitle = (slo: SLOWithSummaryResponse) => {
return slo.groupBy && slo.groupBy !== ALL_VALUE ? `${slo.groupBy}: ${slo.instanceId}` : '';
};

Expand Down Expand Up @@ -88,14 +89,14 @@ export function SloCardItem({ slo, rules, activeAlerts, historicalSummary, cards
}
}}
paddingSize="none"
style={{
height: '182px',
overflow: 'hidden',
position: 'relative',
}}
css={css`
height: 182px;
overflow: hidden;
position: relative;
`}
title={slo.summary.status}
>
<SloCardChart slo={slo} historicalSliData={historicalSliData} cardsPerRow={cardsPerRow} />
<SloCardChart slo={slo} historicalSliData={historicalSliData} />
{(isMouseOver || isActionsPopoverOpen) && (
<SloCardItemActions
slo={slo}
Expand Down Expand Up @@ -135,19 +136,17 @@ export function SloCardItem({ slo, rules, activeAlerts, historicalSummary, cards

export function SloCardChart({
slo,
cardsPerRow,
historicalSliData,
}: {
slo: SLOWithSummaryResponse;
cardsPerRow: number;
historicalSliData?: Array<{ key?: number; value?: number }>;
}) {
const {
application: { navigateToUrl },
} = useKibana().services;

const cardColor = useSloCardColor(slo.summary.status);
const subTitle = getSubTitle(slo, cardsPerRow);
const subTitle = getSubTitle(slo);
const { sliValue, sloTarget, sloDetailsUrl } = useSloFormattedSummary(slo);

return (
Expand Down
Loading
Loading