diff --git a/x-pack/plugins/observability_solution/slo/public/hooks/use_create_rule.ts b/x-pack/plugins/observability_solution/slo/public/hooks/use_create_burn_rate_rule.tsx similarity index 59% rename from x-pack/plugins/observability_solution/slo/public/hooks/use_create_rule.ts rename to x-pack/plugins/observability_solution/slo/public/hooks/use_create_burn_rate_rule.tsx index a85072d907983..906e844c14806 100644 --- a/x-pack/plugins/observability_solution/slo/public/hooks/use_create_rule.ts +++ b/x-pack/plugins/observability_solution/slo/public/hooks/use_create_burn_rate_rule.tsx @@ -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'; @@ -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() { const { http, + i18n: i18nStart, notifications: { toasts }, + theme, } = useKibana().services; - const createRule = useMutation< + return useMutation< CreateRuleResponse, Error, - { rule: CreateRuleRequestBody } + { rule: CreateRuleRequestBody }, + { loadingToastId?: string } >( ['createRule'], ({ rule }) => { @@ -39,6 +45,24 @@ export function useCreateRule() { } }, { + onMutate: async () => { + const loadingToast = toasts.addInfo({ + title: toMountPoint( + + + {i18n.translate('xpack.slo.rules.createRule.loadingNotification.descriptionText', { + defaultMessage: 'Creating burn rate rule ...', + })} + + + + + , + { i18n: i18nStart, theme } + ), + }); + return { loadingToastId: loadingToast.id }; + }, onError: (_err) => { toasts.addDanger( i18n.translate('xpack.slo.rules.createRule.errorNotification.descriptionText', { @@ -54,8 +78,11 @@ export function useCreateRule() { }) ); }, + onSettled: (_d, _err, _res, ctx) => { + if (ctx?.loadingToastId) { + toasts.remove(ctx?.loadingToastId); + } + }, } ); - - return createRule; } diff --git a/x-pack/plugins/observability_solution/slo/public/hooks/use_create_slo.tsx b/x-pack/plugins/observability_solution/slo/public/hooks/use_create_slo.tsx index fcb02315a847a..e5b6d2c114a84 100644 --- a/x-pack/plugins/observability_solution/slo/public/hooks/use_create_slo.tsx +++ b/x-pack/plugins/observability_solution/slo/public/hooks/use_create_slo.tsx @@ -55,7 +55,7 @@ export function useCreateSlo() { void; } -export const maxWidth = 775; - export function SloEditFormFooter({ slo, onSave }: Props) { const { application: { navigateToUrl }, @@ -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(); const navigate = useCallback( @@ -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) { diff --git a/x-pack/plugins/observability_solution/slo/public/pages/slo_edit/slo_edit.test.tsx b/x-pack/plugins/observability_solution/slo/public/pages/slo_edit/slo_edit.test.tsx index dcd497da34770..f5acb4e964f08 100644 --- a/x-pack/plugins/observability_solution/slo/public/pages/slo_edit/slo_edit.test.tsx +++ b/x-pack/plugins/observability_solution/slo/public/pages/slo_edit/slo_edit.test.tsx @@ -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'; @@ -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(); diff --git a/x-pack/plugins/observability_solution/slo/server/routes/slo/route.ts b/x-pack/plugins/observability_solution/slo/server/routes/slo/route.ts index fffbd7aa1e924..703c8aab5a684 100644 --- a/x-pack/plugins/observability_solution/slo/server/routes/slo/route.ts +++ b/x-pack/plugins/observability_solution/slo/server/routes/slo/route.ts @@ -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 { @@ -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: { @@ -101,10 +108,7 @@ 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; @@ -112,7 +116,10 @@ const createSLORoute = createSloServerRoute({ 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, @@ -125,7 +132,6 @@ const createSLORoute = createSloServerRoute({ scopedClusterClient, logger ); - const createSLO = new CreateSLO( esClient, scopedClusterClient, @@ -137,9 +143,7 @@ const createSLORoute = createSloServerRoute({ basePath ); - const response = await createSLO.execute(params.body); - - return response; + return await createSLO.execute(params.body); }, }); diff --git a/x-pack/plugins/observability_solution/slo/server/services/create_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/create_slo.test.ts index 9499294eeb89b..84edf74f18aa5 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/create_slo.test.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/create_slo.test.ts @@ -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', @@ -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' })); @@ -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), @@ -122,8 +119,7 @@ describe('CreateSLO', () => { enabled: true, createdAt: expect.any(Date), updatedAt: expect.any(Date), - }), - { throwOnConflict: true } + }) ); }); @@ -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), @@ -155,8 +151,7 @@ describe('CreateSLO', () => { enabled: true, createdAt: expect.any(Date), updatedAt: expect.any(Date), - }), - { throwOnConflict: true } + }) ); }); }); @@ -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() }); @@ -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 @@ -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(); }); }); diff --git a/x-pack/plugins/observability_solution/slo/server/services/create_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/create_slo.ts index 0f6885dd44c6c..3845ec2ddbd4f 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/create_slo.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/create_slo.ts @@ -10,6 +10,7 @@ import { ElasticsearchClient, IBasePath, Logger } from '@kbn/core/server'; import { ALL_VALUE, CreateSLOParams, CreateSLOResponse } from '@kbn/slo-schema'; import { asyncForEach } from '@kbn/std'; import { v4 as uuidv4 } from 'uuid'; +import { IngestPutPipelineRequest } from '@elastic/elasticsearch/lib/api/types'; import { getSLOPipelineId, getSLOSummaryPipelineId, @@ -22,7 +23,7 @@ import { getSLOPipelineTemplate } from '../assets/ingest_templates/slo_pipeline_ import { getSLOSummaryPipelineTemplate } from '../assets/ingest_templates/slo_summary_pipeline_template'; import { Duration, DurationUnit, SLODefinition } from '../domain/models'; import { validateSLO } from '../domain/services'; -import { SecurityException } from '../errors'; +import { SecurityException, SLOIdConflict } from '../errors'; import { retryTransientEsErrors } from '../utils/retry'; import { SLORepository } from './slo_repository'; import { createTempSummaryDocument } from './summary_transform_generator/helpers/create_temp_summary'; @@ -47,62 +48,58 @@ export class CreateSLO { const rollbackOperations = []; - await this.repository.save(slo, { throwOnConflict: true }); - rollbackOperations.push(() => this.repository.deleteById(slo.id)); + const sloAlreadyExists = await this.repository.checkIfSLOExists(slo); + + if (sloAlreadyExists) { + throw new SLOIdConflict(`SLO [${slo.id}] already exists`); + } + + const createPromise = this.repository.create(slo); + + rollbackOperations.push(() => this.repository.deleteById(slo.id, true)); const rollupTransformId = getSLOTransformId(slo.id, slo.revision); const summaryTransformId = getSLOSummaryTransformId(slo.id, slo.revision); try { - await retryTransientEsErrors( - () => - this.scopedClusterClient.asSecondaryAuthUser.ingest.putPipeline( - getSLOPipelineTemplate(slo) - ), - { logger: this.logger } - ); - rollbackOperations.push(() => - this.scopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline( - { id: getSLOPipelineId(slo.id, slo.revision) }, - { ignore: [404] } - ) - ); + const sloPipelinePromise = this.createPipeline(getSLOPipelineTemplate(slo)); + rollbackOperations.push(() => this.deletePipeline(getSLOPipelineId(slo.id, slo.revision))); - await this.transformManager.install(slo); + const rollupTransformPromise = this.transformManager.install(slo); rollbackOperations.push(() => this.transformManager.uninstall(rollupTransformId)); - await this.transformManager.start(rollupTransformId); - rollbackOperations.push(() => this.transformManager.stop(rollupTransformId)); - - await retryTransientEsErrors( - () => - this.scopedClusterClient.asSecondaryAuthUser.ingest.putPipeline( - getSLOSummaryPipelineTemplate(slo, this.spaceId, this.basePath) - ), - { logger: this.logger } + const summaryPipelinePromise = this.createPipeline( + getSLOSummaryPipelineTemplate(slo, this.spaceId, this.basePath) ); + rollbackOperations.push(() => - this.scopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline( - { id: getSLOSummaryPipelineId(slo.id, slo.revision) }, - { ignore: [404] } - ) + this.deletePipeline(getSLOSummaryPipelineId(slo.id, slo.revision)) ); - await this.summaryTransformManager.install(slo); + const summaryTransformPromise = this.summaryTransformManager.install(slo); rollbackOperations.push(() => this.summaryTransformManager.uninstall(summaryTransformId)); - await this.summaryTransformManager.start(summaryTransformId); + const tempDocPromise = this.createTempSummaryDocument(slo); + + rollbackOperations.push(() => this.deleteTempSummaryDocument(slo)); + + await Promise.all([ + createPromise, + sloPipelinePromise, + rollupTransformPromise, + summaryPipelinePromise, + summaryTransformPromise, + tempDocPromise, + ]); + + rollbackOperations.push(() => this.transformManager.stop(rollupTransformId)); rollbackOperations.push(() => this.summaryTransformManager.stop(summaryTransformId)); - await retryTransientEsErrors( - () => - this.esClient.index({ - index: SLO_SUMMARY_TEMP_INDEX_NAME, - id: `slo-${slo.id}`, - document: createTempSummaryDocument(slo, this.spaceId, this.basePath), - refresh: true, - }), - { logger: this.logger } - ); + // transforms can only be started after the pipeline is created + + await Promise.all([ + this.transformManager.start(rollupTransformId), + this.summaryTransformManager.start(summaryTransformId), + ]); } catch (err) { this.logger.error( `Cannot install the SLO [id: ${slo.id}, revision: ${slo.revision}]. Rolling back.` @@ -126,6 +123,45 @@ export class CreateSLO { return this.toResponse(slo); } + async createTempSummaryDocument(slo: SLODefinition) { + return await retryTransientEsErrors( + () => + this.esClient.index({ + index: SLO_SUMMARY_TEMP_INDEX_NAME, + id: `slo-${slo.id}`, + document: createTempSummaryDocument(slo, this.spaceId, this.basePath), + refresh: true, + }), + { logger: this.logger } + ); + } + + async deleteTempSummaryDocument(slo: SLODefinition) { + return await retryTransientEsErrors( + () => + this.esClient.delete({ + index: SLO_SUMMARY_TEMP_INDEX_NAME, + id: `slo-${slo.id}`, + refresh: true, + }), + { logger: this.logger } + ); + } + + async createPipeline(params: IngestPutPipelineRequest) { + return await retryTransientEsErrors( + () => this.scopedClusterClient.asSecondaryAuthUser.ingest.putPipeline(params), + { logger: this.logger } + ); + } + + async deletePipeline(id: string) { + return this.scopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline( + { id }, + { ignore: [404] } + ); + } + public async inspect(params: CreateSLOParams): Promise<{ slo: CreateSLOParams; rollUpPipeline: Record; diff --git a/x-pack/plugins/observability_solution/slo/server/services/manage_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/manage_slo.test.ts index bace47b69ff4b..d0fd587f6ed0b 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/manage_slo.test.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/manage_slo.test.ts @@ -38,7 +38,7 @@ describe('ManageSLO', () => { expect(mockTransformManager.start).not.toHaveBeenCalled(); expect(mockSummaryTransformManager.start).not.toHaveBeenCalled(); - expect(mockRepository.save).not.toHaveBeenCalled(); + expect(mockRepository.create).not.toHaveBeenCalled(); }); it('enables the slo when disabled', async () => { @@ -49,7 +49,9 @@ describe('ManageSLO', () => { expect(mockTransformManager.start).toMatchSnapshot(); expect(mockSummaryTransformManager.start).toMatchSnapshot(); - expect(mockRepository.save).toHaveBeenCalledWith(expect.objectContaining({ enabled: true })); + expect(mockRepository.update).toHaveBeenCalledWith( + expect.objectContaining({ enabled: true }) + ); }); }); @@ -62,7 +64,7 @@ describe('ManageSLO', () => { expect(mockTransformManager.stop).not.toHaveBeenCalled(); expect(mockSummaryTransformManager.stop).not.toHaveBeenCalled(); - expect(mockRepository.save).not.toHaveBeenCalled(); + expect(mockRepository.update).not.toHaveBeenCalled(); }); it('disables the slo when enabled', async () => { @@ -73,7 +75,9 @@ describe('ManageSLO', () => { expect(mockTransformManager.stop).toMatchSnapshot(); expect(mockSummaryTransformManager.stop).toMatchSnapshot(); - expect(mockRepository.save).toHaveBeenCalledWith(expect.objectContaining({ enabled: false })); + expect(mockRepository.update).toHaveBeenCalledWith( + expect.objectContaining({ enabled: false }) + ); }); }); }); diff --git a/x-pack/plugins/observability_solution/slo/server/services/manage_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/manage_slo.ts index 84e8e3798598b..65f59832b57a6 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/manage_slo.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/manage_slo.ts @@ -26,7 +26,7 @@ export class ManageSLO { await this.transformManager.start(getSLOTransformId(slo.id, slo.revision)); slo.enabled = true; slo.updatedAt = new Date(); - await this.repository.save(slo); + await this.repository.update(slo); } async disable(sloId: string) { @@ -39,6 +39,6 @@ export class ManageSLO { await this.transformManager.stop(getSLOTransformId(slo.id, slo.revision)); slo.enabled = false; slo.updatedAt = new Date(); - await this.repository.save(slo); + await this.repository.update(slo); } } diff --git a/x-pack/plugins/observability_solution/slo/server/services/mocks/index.ts b/x-pack/plugins/observability_solution/slo/server/services/mocks/index.ts index 4fc9d268f62dd..dc458fcdb813e 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/mocks/index.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/mocks/index.ts @@ -42,11 +42,13 @@ const createSummaryTransformManagerMock = (): jest.Mocked => { const createSLORepositoryMock = (): jest.Mocked => { return { - save: jest.fn(), + create: jest.fn(), + update: jest.fn(), findById: jest.fn(), findAllByIds: jest.fn(), deleteById: jest.fn(), search: jest.fn(), + checkIfSLOExists: jest.fn(), }; }; diff --git a/x-pack/plugins/observability_solution/slo/server/services/reset_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/reset_slo.test.ts index efbf3eedb52e1..4e66d992b46cd 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/reset_slo.test.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/reset_slo.test.ts @@ -63,7 +63,7 @@ describe('ResetSLO', () => { it('resets all associated resources', async () => { const slo = createSLO({ id: 'irrelevant', version: 1 }); mockRepository.findById.mockResolvedValueOnce(slo); - mockRepository.save.mockImplementation((v) => Promise.resolve(v)); + mockRepository.update.mockImplementation((v) => Promise.resolve(v)); await resetSLO.execute(slo.id); @@ -87,7 +87,7 @@ describe('ResetSLO', () => { expect(mockEsClient.index).toMatchSnapshot(); - expect(mockRepository.save).toHaveBeenCalledWith({ + expect(mockRepository.update).toHaveBeenCalledWith({ ...slo, version: SLO_MODEL_VERSION, updatedAt: expect.anything(), diff --git a/x-pack/plugins/observability_solution/slo/server/services/reset_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/reset_slo.ts index f69651ff2ad8a..634f02c8f6f90 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/reset_slo.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/reset_slo.ts @@ -104,7 +104,7 @@ export class ResetSLO { throw err; } - const updatedSlo = await this.repository.save({ + const updatedSlo = await this.repository.update({ ...slo, version: SLO_MODEL_VERSION, updatedAt: new Date(), diff --git a/x-pack/plugins/observability_solution/slo/server/services/slo_repository.test.ts b/x-pack/plugins/observability_solution/slo/server/services/slo_repository.test.ts index 1b6eec0ef4f97..243b2b5e9958b 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/slo_repository.test.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/slo_repository.test.ts @@ -11,7 +11,7 @@ import { MockedLogger } from '@kbn/logging-mocks'; import { sloDefinitionSchema } from '@kbn/slo-schema'; import { SLO_MODEL_VERSION } from '../../common/constants'; import { SLODefinition, StoredSLODefinition } from '../domain/models'; -import { SLOIdConflict, SLONotFound } from '../errors'; +import { SLONotFound } from '../errors'; import { SO_SLO_TYPE } from '../saved_objects'; import { aStoredSLO, createAPMTransactionDurationIndicator, createSLO } from './fixtures/slo'; import { KibanaSavedObjectsSLORepository } from './slo_repository'; @@ -82,43 +82,45 @@ describe('KibanaSavedObjectsSLORepository', () => { }); describe('saving an SLO', () => { - it('saves the new SLO', async () => { + it('checking existing id for slo', async () => { const slo = createSLO({ id: 'my-id' }); soClientMock.find.mockResolvedValueOnce(soFindResponse([])); soClientMock.create.mockResolvedValueOnce(aStoredSLO(slo)); const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock); - const savedSLO = await repository.save(slo); + await repository.checkIfSLOExists(slo); - expect(savedSLO).toEqual(slo); expect(soClientMock.find).toHaveBeenCalledWith({ type: SO_SLO_TYPE, - page: 1, - perPage: 1, + perPage: 0, filter: `slo.attributes.id:(${slo.id})`, }); + }); + + it('saves the new SLO', async () => { + const slo = createSLO({ id: 'my-id' }); + soClientMock.find.mockResolvedValueOnce(soFindResponse([])); + soClientMock.create.mockResolvedValueOnce(aStoredSLO(slo)); + const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock); + + const savedSLO = await repository.create(slo); + + expect(savedSLO).toEqual(slo); expect(soClientMock.create).toHaveBeenCalledWith( SO_SLO_TYPE, - sloDefinitionSchema.encode(slo), - { - id: undefined, - overwrite: true, - } + sloDefinitionSchema.encode(slo) ); }); - it('throws when the SLO id already exists and "throwOnConflict" is true', async () => { + it('checks when the SLO id already exists', async () => { const slo = createSLO({ id: 'my-id' }); soClientMock.find.mockResolvedValueOnce(soFindResponse([slo])); const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock); - await expect(repository.save(slo, { throwOnConflict: true })).rejects.toThrowError( - new SLOIdConflict(`SLO [my-id] already exists`) - ); + await expect(await repository.checkIfSLOExists(slo)).toEqual(true); expect(soClientMock.find).toHaveBeenCalledWith({ type: SO_SLO_TYPE, - page: 1, - perPage: 1, + perPage: 0, filter: `slo.attributes.id:(${slo.id})`, }); }); @@ -129,15 +131,10 @@ describe('KibanaSavedObjectsSLORepository', () => { soClientMock.create.mockResolvedValueOnce(aStoredSLO(slo)); const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock); - const savedSLO = await repository.save(slo); + const savedSLO = await repository.update(slo); expect(savedSLO).toEqual(slo); - expect(soClientMock.find).toHaveBeenCalledWith({ - type: SO_SLO_TYPE, - page: 1, - perPage: 1, - filter: `slo.attributes.id:(${slo.id})`, - }); + expect(soClientMock.create).toHaveBeenCalledWith( SO_SLO_TYPE, sloDefinitionSchema.encode(slo), diff --git a/x-pack/plugins/observability_solution/slo/server/services/slo_repository.ts b/x-pack/plugins/observability_solution/slo/server/services/slo_repository.ts index 9f300a148ac2e..35266ea993bfb 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/slo_repository.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/slo_repository.ts @@ -11,14 +11,16 @@ import { ALL_VALUE, Paginated, Pagination, sloDefinitionSchema } from '@kbn/slo- import { isLeft } from 'fp-ts/lib/Either'; import { SLO_MODEL_VERSION } from '../../common/constants'; import { SLODefinition, StoredSLODefinition } from '../domain/models'; -import { SLOIdConflict, SLONotFound } from '../errors'; +import { SLONotFound } from '../errors'; import { SO_SLO_TYPE } from '../saved_objects'; export interface SLORepository { - save(slo: SLODefinition, options?: { throwOnConflict: boolean }): Promise; + checkIfSLOExists(slo: SLODefinition): Promise; + create(slo: SLODefinition): Promise; + update(slo: SLODefinition): Promise; findAllByIds(ids: string[]): Promise; findById(id: string): Promise; - deleteById(id: string): Promise; + deleteById(id: string, ignoreNotFound?: boolean): Promise; search( search: string, pagination: Pagination, @@ -29,19 +31,30 @@ export interface SLORepository { export class KibanaSavedObjectsSLORepository implements SLORepository { constructor(private soClient: SavedObjectsClientContract, private logger: Logger) {} - async save(slo: SLODefinition, options = { throwOnConflict: false }): Promise { - let existingSavedObjectId; + async checkIfSLOExists(slo: SLODefinition) { + const findResponse = await this.soClient.find({ + type: SO_SLO_TYPE, + perPage: 0, + filter: `slo.attributes.id:(${slo.id})`, + }); + + return findResponse.total > 0; + } + + async create(slo: SLODefinition): Promise { + await this.soClient.create(SO_SLO_TYPE, toStoredSLO(slo)); + return slo; + } + + async update(slo: SLODefinition): Promise { + let existingSavedObjectId: string | undefined; + const findResponse = await this.soClient.find({ type: SO_SLO_TYPE, - page: 1, perPage: 1, filter: `slo.attributes.id:(${slo.id})`, }); if (findResponse.total === 1) { - if (options.throwOnConflict) { - throw new SLOIdConflict(`SLO [${slo.id}] already exists`); - } - existingSavedObjectId = findResponse.saved_objects[0].id; } @@ -73,7 +86,7 @@ export class KibanaSavedObjectsSLORepository implements SLORepository { return slo; } - async deleteById(id: string): Promise { + async deleteById(id: string, ignoreNotFound = false): Promise { const response = await this.soClient.find({ type: SO_SLO_TYPE, page: 1, @@ -82,6 +95,9 @@ export class KibanaSavedObjectsSLORepository implements SLORepository { }); if (response.total === 0) { + if (ignoreNotFound) { + return; + } throw new SLONotFound(`SLO [${id}] not found`); } diff --git a/x-pack/plugins/observability_solution/slo/server/services/update_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/update_slo.test.ts index 37855bd4d8fa4..dccfe5f97d633 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/update_slo.test.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/update_slo.test.ts @@ -204,7 +204,7 @@ describe('UpdateSLO', () => { await updateSLO.execute(slo.id, { settings: newSettings }); expectDeletionOfOriginalSLOResources(slo); - expect(mockRepository.save).toHaveBeenCalledWith( + expect(mockRepository.update).toHaveBeenCalledWith( expect.objectContaining({ ...slo, settings: newSettings, @@ -316,7 +316,7 @@ describe('UpdateSLO', () => { updateSLO.execute(originalSlo.id, { indicator: newIndicator }) ).rejects.toThrowError('Transform install error'); - expect(mockRepository.save).toHaveBeenCalledWith(originalSlo); + expect(mockRepository.update).toHaveBeenCalledWith(originalSlo); expect( mockScopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline ).toHaveBeenCalledTimes(1); // for the sli only @@ -343,7 +343,7 @@ describe('UpdateSLO', () => { updateSLO.execute(originalSlo.id, { indicator: newIndicator }) ).rejects.toThrowError('summary transform start error'); - expect(mockRepository.save).toHaveBeenCalledWith(originalSlo); + expect(mockRepository.update).toHaveBeenCalledWith(originalSlo); expect(mockSummaryTransformManager.uninstall).toHaveBeenCalled(); expect(mockScopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline).toHaveBeenCalled(); expect(mockTransformManager.stop).toHaveBeenCalled(); diff --git a/x-pack/plugins/observability_solution/slo/server/services/update_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/update_slo.ts index 0f1967800d1df..9418bfb1ea91a 100644 --- a/x-pack/plugins/observability_solution/slo/server/services/update_slo.ts +++ b/x-pack/plugins/observability_solution/slo/server/services/update_slo.ts @@ -70,8 +70,8 @@ export class UpdateSLO { const rollbackOperations = []; - await this.repository.save(updatedSlo); - rollbackOperations.push(() => this.repository.save(originalSlo)); + await this.repository.update(updatedSlo); + rollbackOperations.push(() => this.repository.update(originalSlo)); if (!requireRevisionBump) { // At this point, we still need to update the sli and summary pipeline to include the changes (id and revision in the rollup index) and (name, desc, tags, ...) in the summary index diff --git a/x-pack/test/api_integration/apis/slos/delete_slo.ts b/x-pack/test/api_integration/apis/slos/delete_slo.ts index 19480d568a37c..979564f06be55 100644 --- a/x-pack/test/api_integration/apis/slos/delete_slo.ts +++ b/x-pack/test/api_integration/apis/slos/delete_slo.ts @@ -55,13 +55,15 @@ export default function ({ getService }: FtrProviderContext) { const { id } = response.body; - const savedObject = await kibanaServer.savedObjects.find({ - type: SO_SLO_TYPE, - }); + await retry.tryForTime(10000, async () => { + const savedObject = await kibanaServer.savedObjects.find({ + type: SO_SLO_TYPE, + }); - expect(savedObject.saved_objects.length).eql(1); + expect(savedObject.saved_objects.length).eql(1); - expect(savedObject.saved_objects[0].attributes.id).eql(id); + expect(savedObject.saved_objects[0].attributes.id).eql(id); + }); await retry.tryForTime(300 * 1000, async () => { // expect summary and rollup data to exist diff --git a/x-pack/test/api_integration/apis/slos/update_slo.ts b/x-pack/test/api_integration/apis/slos/update_slo.ts index 7bf6967bd26a3..a8f4aa1a334f8 100644 --- a/x-pack/test/api_integration/apis/slos/update_slo.ts +++ b/x-pack/test/api_integration/apis/slos/update_slo.ts @@ -14,7 +14,7 @@ import { loadTestData } from './helper/load_test_data'; import { sloData } from './fixtures/create_slo'; export default function ({ getService }: FtrProviderContext) { - describe('Update SLOs', function () { + describe('UpdateSLOs', function () { this.tags('skipCloud'); const supertestAPI = getService('supertest');