Skip to content

Commit

Permalink
Always bump revision on update (#161897)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kdelemme
kdelemme committed Jul 14, 2023
1 parent cab4c19 commit 887432c
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 164 deletions.
6 changes: 3 additions & 3 deletions x-pack/plugins/observability/server/services/slo/delete_slo.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
* 2.0.
*/

import { RulesClientApi } from '@kbn/alerting-plugin/server/types';
import { rulesClientMock } from '@kbn/alerting-plugin/server/rules_client.mock';
import { RulesClientApi } from '@kbn/alerting-plugin/server/types';
import { ElasticsearchClient } from '@kbn/core/server';
import { elasticsearchServiceMock } from '@kbn/core/server/mocks';
import { getSLOTransformId, SLO_INDEX_TEMPLATE_NAME } from '../../assets/constants';
import { getSLOTransformId, SLO_DESTINATION_INDEX_PATTERN } from '../../assets/constants';
import { DeleteSLO } from './delete_slo';
import { createAPMTransactionErrorRateIndicator, createSLO } from './fixtures/slo';
import { createSLORepositoryMock, createTransformManagerMock } from './mocks';
Expand Down Expand Up @@ -47,7 +47,7 @@ describe('DeleteSLO', () => {
);
expect(mockEsClient.deleteByQuery).toHaveBeenCalledWith(
expect.objectContaining({
index: `${SLO_INDEX_TEMPLATE_NAME}*`,
index: SLO_DESTINATION_INDEX_PATTERN,
query: {
match: {
'slo.id': slo.id,
Expand Down
5 changes: 2 additions & 3 deletions x-pack/plugins/observability/server/services/slo/delete_slo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@

import { RulesClientApi } from '@kbn/alerting-plugin/server/types';
import { ElasticsearchClient } from '@kbn/core/server';
import { getSLOTransformId, SLO_INDEX_TEMPLATE_NAME } from '../../assets/constants';

import { getSLOTransformId, SLO_DESTINATION_INDEX_PATTERN } from '../../assets/constants';
import { SLORepository } from './slo_repository';
import { TransformManager } from './transform_manager';

Expand All @@ -34,7 +33,7 @@ export class DeleteSLO {

private async deleteRollupData(sloId: string): Promise<void> {
await this.esClient.deleteByQuery({
index: `${SLO_INDEX_TEMPLATE_NAME}*`,
index: SLO_DESTINATION_INDEX_PATTERN,
wait_for_completion: false,
query: {
match: {
Expand Down
181 changes: 72 additions & 109 deletions x-pack/plugins/observability/server/services/slo/update_slo.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,131 +34,94 @@ describe('UpdateSLO', () => {
updateSLO = new UpdateSLO(mockRepository, mockTransformManager, mockEsClient);
});

describe('without breaking changes', () => {
it('updates the SLO saved object without revision bump', async () => {
const slo = createSLO({ indicator: createAPMTransactionErrorRateIndicator() });
mockRepository.findById.mockResolvedValueOnce(slo);

const newName = 'new slo name';
const newTags = ['other', 'tags'];
const response = await updateSLO.execute(slo.id, { name: newName, tags: newTags });

expectTransformManagerNeverCalled();
expect(mockEsClient.deleteByQuery).not.toBeCalled();
expect(mockRepository.save).toBeCalledWith(
expect.objectContaining({
...slo,
name: newName,
tags: newTags,
updatedAt: expect.anything(),
})
);
expect(slo.name).not.toEqual(newName);
expect(response.name).toEqual(newName);
expect(response.updatedAt).not.toEqual(slo.updatedAt);
expect(response.revision).toEqual(slo.revision);
expect(response.tags).toEqual(newTags);
expect(slo.tags).not.toEqual(newTags);
});
it('updates the settings correctly', async () => {
const slo = createSLO();
mockRepository.findById.mockResolvedValueOnce(slo);

const newSettings = { ...slo.settings, timestamp_field: 'newField' };
await updateSLO.execute(slo.id, { settings: newSettings });

expectDeletionOfObsoleteSLOData(slo);
expect(mockRepository.save).toBeCalledWith(
expect.objectContaining({
...slo,
settings: newSettings,
revision: 2,
updatedAt: expect.anything(),
})
);
expectInstallationOfNewSLOTransform();
});

describe('with breaking changes', () => {
it('consideres settings as a breaking change', async () => {
const slo = createSLO();
mockRepository.findById.mockResolvedValueOnce(slo);

const newSettings = { ...slo.settings, timestamp_field: 'newField' };
await updateSLO.execute(slo.id, { settings: newSettings });

expectDeletionOfObsoleteSLOData(slo);
expect(mockRepository.save).toBeCalledWith(
expect.objectContaining({
...slo,
settings: newSettings,
revision: 2,
updatedAt: expect.anything(),
})
);
expectInstallationOfNewSLOTransform();
it('updates the budgeting method correctly', async () => {
const slo = createSLO({ budgetingMethod: 'occurrences' });
mockRepository.findById.mockResolvedValueOnce(slo);

await updateSLO.execute(slo.id, {
budgetingMethod: 'timeslices',
objective: {
target: slo.objective.target,
timesliceTarget: 0.9,
timesliceWindow: oneMinute(),
},
});

it('consideres a budgeting method change as a breaking change', async () => {
const slo = createSLO({ budgetingMethod: 'occurrences' });
mockRepository.findById.mockResolvedValueOnce(slo);
expectDeletionOfObsoleteSLOData(slo);
expectInstallationOfNewSLOTransform();
});

await updateSLO.execute(slo.id, {
budgetingMethod: 'timeslices',
objective: {
target: slo.objective.target,
timesliceTarget: 0.9,
timesliceWindow: oneMinute(),
},
});
it('updates the timeslice target correctly', async () => {
const slo = createSLOWithTimeslicesBudgetingMethod();
mockRepository.findById.mockResolvedValueOnce(slo);

expectDeletionOfObsoleteSLOData(slo);
expectInstallationOfNewSLOTransform();
await updateSLO.execute(slo.id, {
objective: {
target: slo.objective.target,
timesliceTarget: 0.1,
timesliceWindow: slo.objective.timesliceWindow,
},
});

it('consideres a timeslice target change as a breaking change', async () => {
const slo = createSLOWithTimeslicesBudgetingMethod();
mockRepository.findById.mockResolvedValueOnce(slo);
expectDeletionOfObsoleteSLOData(slo);
expectInstallationOfNewSLOTransform();
});

await updateSLO.execute(slo.id, {
objective: {
target: slo.objective.target,
timesliceTarget: 0.1,
timesliceWindow: slo.objective.timesliceWindow,
},
});
it('consideres a timeslice window change as a breaking change', async () => {
const slo = createSLOWithTimeslicesBudgetingMethod();
mockRepository.findById.mockResolvedValueOnce(slo);

expectDeletionOfObsoleteSLOData(slo);
expectInstallationOfNewSLOTransform();
await updateSLO.execute(slo.id, {
objective: {
target: slo.objective.target,
timesliceTarget: slo.objective.timesliceTarget,
timesliceWindow: fiveMinute(),
},
});

it('consideres a timeslice window change as a breaking change', async () => {
const slo = createSLOWithTimeslicesBudgetingMethod();
mockRepository.findById.mockResolvedValueOnce(slo);

await updateSLO.execute(slo.id, {
objective: {
target: slo.objective.target,
timesliceTarget: slo.objective.timesliceTarget,
timesliceWindow: fiveMinute(),
},
});
expectDeletionOfObsoleteSLOData(slo);
expectInstallationOfNewSLOTransform();
});

expectDeletionOfObsoleteSLOData(slo);
expectInstallationOfNewSLOTransform();
it('removes the obsolete data from the SLO previous revision', async () => {
const slo = createSLO({
indicator: createAPMTransactionErrorRateIndicator({ environment: 'development' }),
});
mockRepository.findById.mockResolvedValueOnce(slo);

it('removes the obsolete data from the SLO previous revision', async () => {
const slo = createSLO({
indicator: createAPMTransactionErrorRateIndicator({ environment: 'development' }),
});
mockRepository.findById.mockResolvedValueOnce(slo);

const newIndicator = createAPMTransactionErrorRateIndicator({ environment: 'production' });
await updateSLO.execute(slo.id, { indicator: newIndicator });

expectDeletionOfObsoleteSLOData(slo);
expect(mockRepository.save).toBeCalledWith(
expect.objectContaining({
...slo,
indicator: newIndicator,
revision: 2,
updatedAt: expect.anything(),
})
);
expectInstallationOfNewSLOTransform();
});
});
const newIndicator = createAPMTransactionErrorRateIndicator({ environment: 'production' });
await updateSLO.execute(slo.id, { indicator: newIndicator });

function expectTransformManagerNeverCalled() {
expect(mockTransformManager.stop).not.toBeCalled();
expect(mockTransformManager.uninstall).not.toBeCalled();
expect(mockTransformManager.start).not.toBeCalled();
expect(mockTransformManager.install).not.toBeCalled();
}
expectDeletionOfObsoleteSLOData(slo);
expect(mockRepository.save).toBeCalledWith(
expect.objectContaining({
...slo,
indicator: newIndicator,
revision: 2,
updatedAt: expect.anything(),
})
);
expectInstallationOfNewSLOTransform();
});

function expectInstallationOfNewSLOTransform() {
expect(mockTransformManager.start).toBeCalled();
Expand Down
62 changes: 13 additions & 49 deletions x-pack/plugins/observability/server/services/slo/update_slo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@
* 2.0.
*/

import deepEqual from 'fast-deep-equal';
import { ElasticsearchClient } from '@kbn/core/server';
import { UpdateSLOParams, UpdateSLOResponse, updateSLOResponseSchema } from '@kbn/slo-schema';

import { getSLOTransformId, SLO_INDEX_TEMPLATE_NAME } from '../../assets/constants';
import { SLORepository } from './slo_repository';
import { TransformManager } from './transform_manager';
import { getSLOTransformId, SLO_DESTINATION_INDEX_PATTERN } from '../../assets/constants';
import { SLO } from '../../domain/models';
import { validateSLO } from '../../domain/services';
import { SLORepository } from './slo_repository';
import { TransformManager } from './transform_manager';

export class UpdateSLO {
constructor(
Expand All @@ -24,52 +22,18 @@ export class UpdateSLO {

public async execute(sloId: string, params: UpdateSLOParams): Promise<UpdateSLOResponse> {
const originalSlo = await this.repository.findById(sloId);
const { hasBreakingChange, updatedSlo } = this.updateSLO(originalSlo, params);

if (hasBreakingChange) {
await this.deleteObsoleteSLORevisionData(originalSlo);

await this.repository.save(updatedSlo);
await this.transformManager.install(updatedSlo);
await this.transformManager.start(getSLOTransformId(updatedSlo.id, updatedSlo.revision));
} else {
await this.repository.save(updatedSlo);
}

return this.toResponse(updatedSlo);
}

private updateSLO(originalSlo: SLO, params: UpdateSLOParams) {
let hasBreakingChange = false;
const updatedSlo: SLO = Object.assign({}, originalSlo, params, { updatedAt: new Date() });
const updatedSlo: SLO = Object.assign({}, originalSlo, params, {
updatedAt: new Date(),
revision: originalSlo.revision + 1,
});
validateSLO(updatedSlo);

if (!deepEqual(originalSlo.indicator, updatedSlo.indicator)) {
hasBreakingChange = true;
}

if (originalSlo.budgetingMethod !== updatedSlo.budgetingMethod) {
hasBreakingChange = true;
}
await this.deleteObsoleteSLORevisionData(originalSlo);
await this.repository.save(updatedSlo);
await this.transformManager.install(updatedSlo);
await this.transformManager.start(getSLOTransformId(updatedSlo.id, updatedSlo.revision));

if (
originalSlo.budgetingMethod === 'timeslices' &&
updatedSlo.budgetingMethod === 'timeslices' &&
(originalSlo.objective.timesliceTarget !== updatedSlo.objective.timesliceTarget ||
!deepEqual(originalSlo.objective.timesliceWindow, updatedSlo.objective.timesliceWindow))
) {
hasBreakingChange = true;
}

if (!deepEqual(originalSlo.settings, updatedSlo.settings)) {
hasBreakingChange = true;
}

if (hasBreakingChange) {
updatedSlo.revision++;
}

return { hasBreakingChange, updatedSlo };
return this.toResponse(updatedSlo);
}

private async deleteObsoleteSLORevisionData(originalSlo: SLO) {
Expand All @@ -81,7 +45,7 @@ export class UpdateSLO {

private async deleteRollupData(sloId: string, sloRevision: number): Promise<void> {
await this.esClient.deleteByQuery({
index: `${SLO_INDEX_TEMPLATE_NAME}*`,
index: SLO_DESTINATION_INDEX_PATTERN,
wait_for_completion: false,
query: {
bool: {
Expand Down

0 comments on commit 887432c

Please sign in to comment.