Skip to content

Commit

Permalink
[RAM] Improve rule interval circuit breaker error message (#168173)
Browse files Browse the repository at this point in the history
## Summary
Improve rule interval circuit breaker error message to the following
endpoints:

- create
- update
- bulk edit
- bulk enable

### Bulk modification
![Screenshot from 2023-10-06
10-11-24](https://github.com/elastic/kibana/assets/74562234/11271221-4d92-41a4-9c0a-f2f8972c452e)

### Modifying a single rule
![Screenshot from 2023-10-06
10-12-16](https://github.com/elastic/kibana/assets/74562234/4ad5f482-6b68-4eef-8989-3f0013c218b2)


### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Xavier Mouligneau <[email protected]>
  • Loading branch information
3 people authored Oct 13, 2023
1 parent e76e589 commit c66a86b
Show file tree
Hide file tree
Showing 27 changed files with 536 additions and 119 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,24 @@ export const RuleStatusDropdownSandbox = ({ triggersActionsUi }: SandboxProps) =
const [isSnoozedUntil, setIsSnoozedUntil] = useState<Date | null>(null);
const [muteAll, setMuteAll] = useState(false);

const onEnableRule: any = async () => {
setEnabled(true);
setMuteAll(false);
setIsSnoozedUntil(null);
};

const onDisableRule: any = async () => {
setEnabled(false);
};

return triggersActionsUi.getRuleStatusDropdown({
rule: {
enabled,
isSnoozedUntil,
muteAll,
},
enableRule: async () => {
setEnabled(true);
setMuteAll(false);
setIsSnoozedUntil(null);
},
disableRule: async () => setEnabled(false),
enableRule: onEnableRule,
disableRule: onDisableRule,
snoozeRule: async (schedule) => {
if (schedule.duration === -1) {
setIsSnoozedUntil(null);
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export * from './rrule_type';
export * from './rule_tags_aggregation';
export * from './iso_weekdays';
export * from './saved_objects/rules/mappings';
export * from './rule_circuit_breaker_error_message';

export type {
MaintenanceWindowModificationMetadata,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import {
getRuleCircuitBreakerErrorMessage,
parseRuleCircuitBreakerErrorMessage,
} from './rule_circuit_breaker_error_message';

describe('getRuleCircuitBreakerErrorMessage', () => {
it('should return the correct message', () => {
expect(
getRuleCircuitBreakerErrorMessage({
name: 'test rule',
action: 'create',
interval: 5,
intervalAvailable: 4,
})
).toMatchInlineSnapshot(
`"Error validating circuit breaker - Rule 'test rule' cannot be created. The maximum number of runs per minute would be exceeded. - The rule has 5 runs per minute; there are only 4 runs per minute available. Before you can modify this rule, you must increase its check interval so that it runs less frequently. Alternatively, disable other rules or change their check intervals."`
);

expect(
getRuleCircuitBreakerErrorMessage({
name: 'test rule',
action: 'update',
interval: 1,
intervalAvailable: 1,
})
).toMatchInlineSnapshot(
`"Error validating circuit breaker - Rule 'test rule' cannot be updated. The maximum number of runs per minute would be exceeded. - The rule has 1 run per minute; there is only 1 run per minute available. Before you can modify this rule, you must increase its check interval so that it runs less frequently. Alternatively, disable other rules or change their check intervals."`
);

expect(
getRuleCircuitBreakerErrorMessage({
name: 'test rule',
action: 'bulkEdit',
interval: 1,
intervalAvailable: 1,
rules: 5,
})
).toMatchInlineSnapshot(
`"Error validating circuit breaker - Rules cannot be bulk edited. The maximum number of runs per minute would be exceeded. - The rules have 1 run per minute; there is only 1 run per minute available. Before you can modify these rules, you must disable other rules or change their check intervals so they run less frequently."`
);
});

it('should parse the error message', () => {
const message = getRuleCircuitBreakerErrorMessage({
name: 'test rule',
action: 'create',
interval: 5,
intervalAvailable: 4,
});

const parsedMessage = parseRuleCircuitBreakerErrorMessage(message);

expect(parsedMessage.summary).toContain("Rule 'test rule' cannot be created");
expect(parsedMessage.details).toContain('The rule has 5 runs per minute');
});

it('should passthrough the message if it is not related to circuit breakers', () => {
const parsedMessage = parseRuleCircuitBreakerErrorMessage('random message');

expect(parsedMessage.summary).toEqual('random message');
expect(parsedMessage.details).toBeUndefined();
});
});
136 changes: 136 additions & 0 deletions x-pack/plugins/alerting/common/rule_circuit_breaker_error_message.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { i18n } from '@kbn/i18n';

const errorMessageHeader = 'Error validating circuit breaker';

const getCreateRuleErrorSummary = (name: string) => {
return i18n.translate('xpack.alerting.ruleCircuitBreaker.error.createSummary', {
defaultMessage: `Rule '{name}' cannot be created. The maximum number of runs per minute would be exceeded.`,
values: {
name,
},
});
};

const getUpdateRuleErrorSummary = (name: string) => {
return i18n.translate('xpack.alerting.ruleCircuitBreaker.error.updateSummary', {
defaultMessage: `Rule '{name}' cannot be updated. The maximum number of runs per minute would be exceeded.`,
values: {
name,
},
});
};

const getEnableRuleErrorSummary = (name: string) => {
return i18n.translate('xpack.alerting.ruleCircuitBreaker.error.enableSummary', {
defaultMessage: `Rule '{name}' cannot be enabled. The maximum number of runs per minute would be exceeded.`,
values: {
name,
},
});
};

const getBulkEditRuleErrorSummary = () => {
return i18n.translate('xpack.alerting.ruleCircuitBreaker.error.bulkEditSummary', {
defaultMessage: `Rules cannot be bulk edited. The maximum number of runs per minute would be exceeded.`,
});
};

const getBulkEnableRuleErrorSummary = () => {
return i18n.translate('xpack.alerting.ruleCircuitBreaker.error.bulkEnableSummary', {
defaultMessage: `Rules cannot be bulk enabled. The maximum number of runs per minute would be exceeded.`,
});
};

const getRuleCircuitBreakerErrorDetail = ({
interval,
intervalAvailable,
rules,
}: {
interval: number;
intervalAvailable: number;
rules: number;
}) => {
if (rules === 1) {
return i18n.translate('xpack.alerting.ruleCircuitBreaker.error.ruleDetail', {
defaultMessage: `The rule has {interval, plural, one {{interval} run} other {{interval} runs}} per minute; there {intervalAvailable, plural, one {is only {intervalAvailable} run} other {are only {intervalAvailable} runs}} per minute available. Before you can modify this rule, you must increase its check interval so that it runs less frequently. Alternatively, disable other rules or change their check intervals.`,
values: {
interval,
intervalAvailable,
},
});
}
return i18n.translate('xpack.alerting.ruleCircuitBreaker.error.multipleRuleDetail', {
defaultMessage: `The rules have {interval, plural, one {{interval} run} other {{interval} runs}} per minute; there {intervalAvailable, plural, one {is only {intervalAvailable} run} other {are only {intervalAvailable} runs}} per minute available. Before you can modify these rules, you must disable other rules or change their check intervals so they run less frequently.`,
values: {
interval,
intervalAvailable,
},
});
};

export const getRuleCircuitBreakerErrorMessage = ({
name = '',
interval,
intervalAvailable,
action,
rules = 1,
}: {
name?: string;
interval: number;
intervalAvailable: number;
action: 'update' | 'create' | 'enable' | 'bulkEdit' | 'bulkEnable';
rules?: number;
}) => {
let errorMessageSummary: string;

switch (action) {
case 'update':
errorMessageSummary = getUpdateRuleErrorSummary(name);
break;
case 'create':
errorMessageSummary = getCreateRuleErrorSummary(name);
break;
case 'enable':
errorMessageSummary = getEnableRuleErrorSummary(name);
break;
case 'bulkEdit':
errorMessageSummary = getBulkEditRuleErrorSummary();
break;
case 'bulkEnable':
errorMessageSummary = getBulkEnableRuleErrorSummary();
break;
}

return `Error validating circuit breaker - ${errorMessageSummary} - ${getRuleCircuitBreakerErrorDetail(
{
interval,
intervalAvailable,
rules,
}
)}`;
};

export const parseRuleCircuitBreakerErrorMessage = (
message: string
): {
summary: string;
details?: string;
} => {
if (!message.includes(errorMessageHeader)) {
return {
summary: message,
};
}
const segments = message.split(' - ');
return {
summary: segments[1],
details: segments[2],
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
convertRuleIdsToKueryNode,
} from '../../../../lib';
import { WriteOperations, AlertingAuthorizationEntity } from '../../../../authorization';
import { parseDuration } from '../../../../../common/parse_duration';
import { parseDuration, getRuleCircuitBreakerErrorMessage } from '../../../../../common';
import { bulkMarkApiKeysForInvalidation } from '../../../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation';
import { ruleAuditEvent, RuleAuditAction } from '../../../../rules_client/common/audit_events';
import {
Expand Down Expand Up @@ -77,7 +77,7 @@ import {
transformRuleDomainToRuleAttributes,
transformRuleDomainToRule,
} from '../../transforms';
import { validateScheduleLimit } from '../get_schedule_frequency';
import { validateScheduleLimit, ValidateScheduleLimitResult } from '../get_schedule_frequency';

const isValidInterval = (interval: string | undefined): interval is string => {
return interval !== undefined;
Expand Down Expand Up @@ -326,23 +326,30 @@ async function bulkEditRulesOcc<Params extends RuleParams>(
.map((rule) => rule.attributes.schedule?.interval)
.filter(isValidInterval);

try {
if (operations.some((operation) => operation.field === 'schedule')) {
await validateScheduleLimit({
context,
prevInterval,
updatedInterval,
});
}
} catch (error) {
let validationPayload: ValidateScheduleLimitResult = null;
if (operations.some((operation) => operation.field === 'schedule')) {
validationPayload = await validateScheduleLimit({
context,
prevInterval,
updatedInterval,
});
}

if (validationPayload) {
return {
apiKeysToInvalidate: Array.from(apiKeysMap.values())
.filter((value) => value.newApiKey)
.map((value) => value.newApiKey as string),
resultSavedObjects: [],
rules: [],
errors: rules.map((rule) => ({
message: `Failed to bulk edit rule - ${error.message}`,
message: getRuleCircuitBreakerErrorMessage({
name: rule.attributes.name || 'n/a',
interval: validationPayload!.interval,
intervalAvailable: validationPayload!.intervalAvailable,
action: 'bulkEdit',
rules: updatedInterval.length,
}),
rule: {
id: rule.id,
name: rule.attributes.name || 'n/a',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import Semver from 'semver';
import Boom from '@hapi/boom';
import { SavedObject, SavedObjectsUtils } from '@kbn/core/server';
import { withSpan } from '@kbn/apm-utils';
import { parseDuration } from '../../../../../common/parse_duration';
import { parseDuration, getRuleCircuitBreakerErrorMessage } from '../../../../../common';
import { WriteOperations, AlertingAuthorizationEntity } from '../../../../authorization';
import {
validateRuleTypeParams,
Expand Down Expand Up @@ -36,7 +36,7 @@ import { RuleAttributes } from '../../../../data/rule/types';
import type { CreateRuleData } from './types';
import { createRuleDataSchema } from './schemas';
import { createRuleSavedObject } from '../../../../rules_client/lib';
import { validateScheduleLimit } from '../get_schedule_frequency';
import { validateScheduleLimit, ValidateScheduleLimitResult } from '../get_schedule_frequency';

export interface CreateRuleOptions {
id?: string;
Expand All @@ -61,16 +61,29 @@ export async function createRule<Params extends RuleParams = never>(

try {
createRuleDataSchema.validate(data);
if (data.enabled) {
await validateScheduleLimit({
context,
updatedInterval: data.schedule.interval,
});
}
} catch (error) {
throw Boom.badRequest(`Error validating create data - ${error.message}`);
}

let validationPayload: ValidateScheduleLimitResult = null;
if (data.enabled) {
validationPayload = await validateScheduleLimit({
context,
updatedInterval: data.schedule.interval,
});
}

if (validationPayload) {
throw Boom.badRequest(
getRuleCircuitBreakerErrorMessage({
name: data.name,
interval: validationPayload!.interval,
intervalAvailable: validationPayload!.intervalAvailable,
action: 'create',
})
);
}

try {
await withSpan({ name: 'authorization.ensureAuthorized', type: 'rules' }, () =>
context.authorization.ensureAuthorized({
Expand Down
Loading

0 comments on commit c66a86b

Please sign in to comment.