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

[8.10] backport SLO fixes #168231

Merged
merged 6 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 10 additions & 6 deletions x-pack/plugins/observability/public/hooks/slo/use_clone_slo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@
* 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 { v1 as uuidv1 } from 'uuid';
import { useKibana } from '../../utils/kibana_react';
import { sloKeys } from './query_key_factory';

type ServerError = IHttpFetchError<ResponseErrorBody>;

export function useCloneSlo() {
const {
http,
Expand All @@ -21,7 +24,7 @@ export function useCloneSlo() {

return useMutation<
CreateSLOResponse,
string,
ServerError,
{ slo: CreateSLOInput; originalSloId?: string },
{ previousData?: FindSLOResponse; queryKey?: QueryKey }
>(
Expand Down Expand Up @@ -58,16 +61,17 @@ export function useCloneSlo() {
return { queryKey, previousData };
},
// If the mutation fails, use the context returned from onMutate to roll back
onError: (_err, { slo }, context) => {
onError: (error, { slo }, context) => {
if (context?.previousData && context?.queryKey) {
queryClient.setQueryData(context.queryKey, context.previousData);
}
toasts.addDanger(
i18n.translate('xpack.observability.slo.clone.errorNotification', {

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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { IHttpFetchError, ResponseErrorBody } from '@kbn/core/public';
import { i18n } from '@kbn/i18n';
import { encode } from '@kbn/rison';
import type { CreateSLOInput, CreateSLOResponse, FindSLOResponse } from '@kbn/slo-schema';
Expand All @@ -14,6 +15,8 @@ import { paths } from '../../../common/locators/paths';
import { useKibana } from '../../utils/kibana_react';
import { sloKeys } from './query_key_factory';

type ServerError = IHttpFetchError<ResponseErrorBody>;

export function useCreateSlo() {
const {
application: { navigateToUrl },
Expand All @@ -24,7 +27,7 @@ export function useCreateSlo() {

return useMutation<
CreateSLOResponse,
string,
ServerError,
{ slo: CreateSLOInput },
{ previousData?: FindSLOResponse; queryKey?: QueryKey }
>(
Expand Down Expand Up @@ -72,7 +75,7 @@ export function useCreateSlo() {
queryClient.setQueryData(context.queryKey, context.previousData);
}

toasts.addError(new Error(String(error)), {
toasts.addError(new Error(error.body?.message ?? error.message), {
title: i18n.translate('xpack.observability.slo.create.errorNotification', {
defaultMessage: 'Something went wrong while creating {name}',
values: { name: slo.name },
Expand Down
15 changes: 9 additions & 6 deletions x-pack/plugins/observability/public/hooks/slo/use_delete_slo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@
import { QueryKey, useMutation, useQueryClient } from '@tanstack/react-query';
import { i18n } from '@kbn/i18n';
import { FindSLOResponse } from '@kbn/slo-schema';
import { IHttpFetchError, ResponseErrorBody } from '@kbn/core/public';
import { useKibana } from '../../utils/kibana_react';
import { sloKeys } from './query_key_factory';

type ServerError = IHttpFetchError<ResponseErrorBody>;

export function useDeleteSlo() {
const {
http,
Expand All @@ -20,7 +23,7 @@ export function useDeleteSlo() {

return useMutation<
string,
string,
ServerError,
{ id: string; name: string },
{ previousData?: FindSLOResponse; queryKey?: QueryKey }
>(
Expand Down Expand Up @@ -56,17 +59,17 @@ export function useDeleteSlo() {
return { previousData, queryKey };
},
// If the mutation fails, use the context returned from onMutate to roll back
onError: (_err, { name }, context) => {
onError: (error, { name }, context) => {
if (context?.previousData && context?.queryKey) {
queryClient.setQueryData(context.queryKey, context.previousData);
}

toasts.addDanger(
i18n.translate('xpack.observability.slo.slo.delete.errorNotification', {
toasts.addError(new Error(error.body?.message ?? error.message), {
title: i18n.translate('xpack.observability.slo.slo.delete.errorNotification', {
defaultMessage: 'Failed to delete {name}',
values: { name },
})
);
}),
});
},
onSuccess: (_data, { name }) => {
toasts.addSuccess(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export function useGetPreviewData(isValid: boolean, indicator: Indicator): UseGe
}
);

return response;
return Array.isArray(response) ? response : [];
},
retry: false,
refetchOnWindowFocus: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@
* 2.0.
*/

import { IHttpFetchError, ResponseErrorBody } from '@kbn/core/public';
import { i18n } from '@kbn/i18n';
import type { FindSLOResponse, UpdateSLOInput, UpdateSLOResponse } from '@kbn/slo-schema';
import { QueryKey, useMutation, useQueryClient } from '@tanstack/react-query';
import { useKibana } from '../../utils/kibana_react';
import { sloKeys } from './query_key_factory';

type ServerError = IHttpFetchError<ResponseErrorBody>;

export function useUpdateSlo() {
const {
http,
Expand All @@ -20,7 +23,7 @@ export function useUpdateSlo() {

return useMutation<
UpdateSLOResponse,
string,
ServerError,
{ sloId: string; slo: UpdateSLOInput },
{ previousData?: FindSLOResponse; queryKey?: QueryKey }
>(
Expand Down Expand Up @@ -69,7 +72,7 @@ export function useUpdateSlo() {
queryClient.setQueryData(context.queryKey, context.previousData);
}

toasts.addError(new Error(String(error)), {
toasts.addError(new Error(error.body?.message ?? error.message), {
title: i18n.translate('xpack.observability.slo.update.errorNotification', {
defaultMessage: 'Something went wrong when updating {name}',
values: { name },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export function SloList({ autoRefresh }: Props) {
const [query, setQuery] = useState('');
const [sort, setSort] = useState<SortField | undefined>('status');

const { isInitialLoading, isLoading, isRefetching, isError, sloList, refetch } = useFetchSloList({
const { isLoading, isRefetching, isError, sloList } = useFetchSloList({
page: activePage + 1,
kqlQuery: query,
sortBy: sort,
Expand All @@ -38,34 +38,23 @@ export function SloList({ autoRefresh }: Props) {

const handlePageClick = (pageNumber: number) => {
setActivePage(pageNumber);
refetch();
};

const handleChangeQuery = (newQuery: string) => {
setActivePage(0);
setQuery(newQuery);
refetch();
};

const handleChangeSort = (newSort: SortField | undefined) => {
setActivePage(0);
setSort(newSort);
refetch();
};

return (
<EuiFlexGroup direction="column" gutterSize="m" data-test-subj="sloList">
<EuiFlexItem grow>
<SloListSearchFilterSortBar
loading={
isInitialLoading ||
isLoading ||
isRefetching ||
isCreatingSlo ||
isCloningSlo ||
isUpdatingSlo ||
isDeletingSlo
}
loading={isLoading || isCreatingSlo || isCloningSlo || isUpdatingSlo || isDeletingSlo}
onChangeQuery={handleChangeQuery}
onChangeSort={handleChangeSort}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
EuiSelectableOption,
} from '@elastic/eui';
import { EuiSelectableOptionCheckedType } from '@elastic/eui/src/components/selectable/selectable_option';
import { Query } from '@kbn/es-query';
import { i18n } from '@kbn/i18n';
import { QueryStringInput } from '@kbn/unified-search-plugin/public';
import React, { useState } from 'react';
Expand Down Expand Up @@ -103,9 +104,13 @@ export function SloListSearchFilterSortBar({
unifiedSearch,
}}
disableAutoFocus
onSubmit={() => onChangeQuery(query)}
onSubmit={(value: Query) => {
setQuery(String(value.query));
onChangeQuery(String(value.query));
}}
disableLanguageSwitcher
isDisabled={loading}
autoSubmit
indexPatterns={dataView ? [dataView] : []}
placeholder={i18n.translate('xpack.observability.slo.list.search', {
defaultMessage: 'Search your SLOs...',
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/observability/server/assets/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

export const SLO_RESOURCES_VERSION = 2;
export const SLO_SUMMARY_TRANSFORMS_VERSION = 3;

export const SLO_COMPONENT_TEMPLATE_MAPPINGS_NAME = '.slo-observability.sli-mappings';
export const SLO_COMPONENT_TEMPLATE_SETTINGS_NAME = '.slo-observability.sli-settings';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const getSLOTransformTemplate = (
dest: destination,
settings: {
deduce_mappings: false,
unattended: true,
},
sync: {
time: {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/observability/server/errors/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,5 @@ export class InternalQueryError extends ObservabilityError {}
export class NotSupportedError extends ObservabilityError {}
export class IllegalArgumentError extends ObservabilityError {}
export class InvalidTransformError extends ObservabilityError {}

export class SecurityException extends ObservabilityError {}
5 changes: 5 additions & 0 deletions x-pack/plugins/observability/server/errors/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
CompositeSLOIdConflict,
CompositeSLONotFound,
ObservabilityError,
SecurityException,
SLOIdConflict,
SLONotFound,
} from './errors';
Expand All @@ -22,5 +23,9 @@ export function getHTTPResponseCode(error: ObservabilityError): number {
return 409;
}

if (error instanceof SecurityException) {
return 403;
}

return 400;
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ describe('CreateSLO', () => {
expect(mockTransformManager.install).toHaveBeenCalledWith(
expect.objectContaining({ ...sloParams, id: 'unique-id' })
);
expect(mockTransformManager.preview).toHaveBeenCalledWith('slo-transform-id');
expect(mockTransformManager.start).toHaveBeenCalledWith('slo-transform-id');
expect(response).toEqual(expect.objectContaining({ id: 'unique-id' }));
expect(esClientMock.index.mock.calls[0]).toMatchSnapshot();
Expand Down Expand Up @@ -100,6 +101,16 @@ describe('CreateSLO', () => {
expect(mockRepository.deleteById).toBeCalled();
});

it('removes the transform and deletes the SLO when transform preview fails', async () => {
mockTransformManager.install.mockResolvedValue('slo-transform-id');
mockTransformManager.preview.mockRejectedValue(new Error('Transform preview error'));
const sloParams = createSLOParams({ indicator: createAPMTransactionErrorRateIndicator() });

await expect(createSLO.execute(sloParams)).rejects.toThrowError('Transform preview error');
expect(mockTransformManager.uninstall).toBeCalledWith('slo-transform-id');
expect(mockRepository.deleteById).toBeCalled();
});

it('removes the transform and deletes the SLO when transform start fails', async () => {
mockTransformManager.install.mockResolvedValue('slo-transform-id');
mockTransformManager.start.mockRejectedValue(new Error('Transform start error'));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export class CreateSLO {
}

try {
await this.transformManager.preview(sloTransformId);
await this.transformManager.start(sloTransformId);
} catch (err) {
await Promise.all([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ import {
KQLCustomIndicator,
MetricCustomIndicator,
} from '@kbn/slo-schema';
import { assertNever } from '@kbn/std';
import { APMTransactionDurationIndicator } from '../../domain/models';
import { computeSLI } from '../../domain/services';
import { InvalidQueryError } from '../../errors';
import {
GetHistogramIndicatorAggregation,
GetCustomMetricIndicatorAggregation,
GetHistogramIndicatorAggregation,
} from './aggregations';

export class GetPreviewData {
Expand Down Expand Up @@ -299,19 +300,24 @@ export class GetPreviewData {
}

public async execute(params: GetPreviewDataParams): Promise<GetPreviewDataResponse> {
switch (params.indicator.type) {
case 'sli.apm.transactionDuration':
return this.getAPMTransactionDurationPreviewData(params.indicator);
case 'sli.apm.transactionErrorRate':
return this.getAPMTransactionErrorPreviewData(params.indicator);
case 'sli.kql.custom':
return this.getCustomKQLPreviewData(params.indicator);
case 'sli.histogram.custom':
return this.getHistogramPreviewData(params.indicator);
case 'sli.metric.custom':
return this.getCustomMetricPreviewData(params.indicator);
default:
return [];
try {
const type = params.indicator.type;
switch (type) {
case 'sli.apm.transactionDuration':
return this.getAPMTransactionDurationPreviewData(params.indicator);
case 'sli.apm.transactionErrorRate':
return this.getAPMTransactionErrorPreviewData(params.indicator);
case 'sli.kql.custom':
return this.getCustomKQLPreviewData(params.indicator);
case 'sli.histogram.custom':
return this.getHistogramPreviewData(params.indicator);
case 'sli.metric.custom':
return this.getCustomMetricPreviewData(params.indicator);
default:
assertNever(type);
}
} catch (err) {
return [];
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ function generateSearchQuery(
window: timeWindowDurationInDays * bucketsPerDay,
shift: 1,
script: 'MovingFunctions.sum(values)',
gap_policy: 'insert_zeros',
},
},
cumulative_total: {
Expand All @@ -287,6 +288,7 @@ function generateSearchQuery(
window: timeWindowDurationInDays * bucketsPerDay,
shift: 1,
script: 'MovingFunctions.sum(values)',
gap_policy: 'insert_zeros',
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const createSummaryTransformInstallerMock = (): jest.Mocked<SummaryTransformInst
const createTransformManagerMock = (): jest.Mocked<TransformManager> => {
return {
install: jest.fn(),
preview: jest.fn(),
uninstall: jest.fn(),
start: jest.fn(),
stop: jest.fn(),
Expand Down
Loading
Loading