Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,19 @@ describe('getEcsGroupsFromFlattenGrouping', () => {
expect(getEcsGroupsFromFlattenGrouping(flattenGrouping)).toEqual({});
});

it('should handle ecs and non-ecs fields properly', () => {
const flattenGrouping = {
['host.name']: 'host-1',
['container.id']: 'container-1',
['non.ecs.field']: 'some-value',
};

expect(getEcsGroupsFromFlattenGrouping(flattenGrouping)).toEqual({
'host.name': 'host-1',
'container.id': 'container-1',
});
});

it('should handle array types assigned non-array values', () => {
const flattenGrouping = { tags: 'abc' };

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,5 @@ export const getEcsGroupsFromFlattenGrouping = (
ecsGroup[flattenGroup] = ecsValue;
}
});

return ecsGroup;
};
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ export const getFormattedGroupBy = (
return groupByKeysObjectMapping;
};

export const getFormattedGroups = (grouping?: Record<string, string>): Group[] | undefined => {
export const getFormattedGroups = (grouping?: Record<string, unknown>): Group[] | undefined => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: Should we give this a type instead of unknown? Especially since we're casting to a string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@JiaweiWu I did this refactoring in this commit to address this PR feedback. I will merge as is and in a follow up PR I can try to use a new getFlattenGrouping instead and revert this change.

const groups: Group[] = [];
if (grouping) {
const groupKeys = Object.keys(grouping);
groupKeys.forEach((group) => {
groups.push({ field: group, value: grouping[group] });
groups.push({ field: group, value: String(grouping[group]) });
});
}
return groups.length ? groups : undefined;
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
ALERT_EVALUATION_THRESHOLD,
ALERT_EVALUATION_VALUE,
ALERT_GROUP,
ALERT_GROUPING,
ALERT_REASON,
SLO_BURN_RATE_RULE_TYPE_ID,
} from '@kbn/rule-registry-plugin/common/technical_rule_data_field_names';
Expand Down Expand Up @@ -346,13 +347,21 @@ describe('BurnRateRuleExecutor', () => {
{ shortWindowBurnRate: 2.1, longWindowBurnRate: 2.3 },
{ shortWindowBurnRate: 0.9, longWindowBurnRate: 1.2 },
],
groupings: {
group: { by: { field: 'foo' } },
client: { geo: { continent_name: 'asia' } },
},
},
{
instanceId: 'bar,asia',
windows: [
{ shortWindowBurnRate: 2.2, longWindowBurnRate: 2.5 },
{ shortWindowBurnRate: 0.9, longWindowBurnRate: 1.2 },
],
groupings: {
group: { by: { field: 'bar' } },
client: { geo: { continent_name: 'asia' } },
},
},
];
esClientMock.search.mockResolvedValueOnce(
Expand Down Expand Up @@ -410,6 +419,10 @@ describe('BurnRateRuleExecutor', () => {
value: 'asia',
},
],
[ALERT_GROUPING]: {
group: { by: { field: 'foo' } },
client: { geo: { continent_name: 'asia' } },
},
'client.geo.continent_name': 'asia',
},
});
Expand Down Expand Up @@ -437,6 +450,10 @@ describe('BurnRateRuleExecutor', () => {
value: 'asia',
},
],
[ALERT_GROUPING]: {
group: { by: { field: 'bar' } },
client: { geo: { continent_name: 'asia' } },
},
'client.geo.continent_name': 'asia',
},
});
Expand Down Expand Up @@ -466,7 +483,10 @@ describe('BurnRateRuleExecutor', () => {
});

it('schedules a suppressed alert when both windows of first window definition burn rate have reached the threshold but the dependency matches', async () => {
const slo = createSLO({ objective: { target: 0.9 }, groupBy: ['group.by.field'] });
const slo = createSLO({
objective: { target: 0.9 },
groupBy: ['group.by.field'],
});
const dependencyRuleParams = someRuleParamsWithWindows({ sloId: slo.id });
const ruleParams = someRuleParamsWithWindows({
sloId: slo.id,
Expand All @@ -480,13 +500,19 @@ describe('BurnRateRuleExecutor', () => {
{ shortWindowBurnRate: 2.1, longWindowBurnRate: 2.3 },
{ shortWindowBurnRate: 0.9, longWindowBurnRate: 1.2 },
],
groupings: {
group: { by: { field: 'foo' } },
},
},
{
instanceId: 'bar',
windows: [
{ shortWindowBurnRate: 2.2, longWindowBurnRate: 2.5 },
{ shortWindowBurnRate: 0.9, longWindowBurnRate: 1.2 },
],
groupings: {
group: { by: { field: 'bar' } },
},
},
];
esClientMock.search.mockResolvedValueOnce(
Expand Down Expand Up @@ -554,6 +580,9 @@ describe('BurnRateRuleExecutor', () => {
value: 'foo',
},
],
[ALERT_GROUPING]: {
group: { by: { field: 'foo' } },
},
},
});
expect(servicesMock.alertsClient?.report).toBeCalledWith({
Expand All @@ -576,6 +605,9 @@ describe('BurnRateRuleExecutor', () => {
value: 'bar',
},
],
[ALERT_GROUPING]: {
group: { by: { field: 'bar' } },
},
},
});
expect(servicesMock.alertsClient?.setAlertData).toHaveBeenNthCalledWith(1, {
Expand All @@ -587,6 +619,9 @@ describe('BurnRateRuleExecutor', () => {
reason:
'SUPPRESSED - CRITICAL: The burn rate for the past 1h is 2.3 and for the past 5m is 2.1 for foo. Alert when above 2 for both windows',
alertDetailsUrl: 'https://kibana.dev/s/irrelevant/app/observability/alerts/uuid-foo',
grouping: {
group: { by: { field: 'foo' } },
},
}),
});
expect(servicesMock.alertsClient?.setAlertData).toHaveBeenNthCalledWith(2, {
Expand All @@ -598,6 +633,9 @@ describe('BurnRateRuleExecutor', () => {
reason:
'SUPPRESSED - CRITICAL: The burn rate for the past 1h is 2.5 and for the past 5m is 2.2 for bar. Alert when above 2 for both windows',
alertDetailsUrl: 'https://kibana.dev/s/irrelevant/app/observability/alerts/uuid-bar',
grouping: {
group: { by: { field: 'bar' } },
},
}),
});
});
Expand All @@ -613,13 +651,19 @@ describe('BurnRateRuleExecutor', () => {
{ shortWindowBurnRate: 1.0, longWindowBurnRate: 2.0 },
{ shortWindowBurnRate: 1.9, longWindowBurnRate: 1.2 },
],
groupings: {
group: { by: { field: 'foo' } },
},
},
{
instanceId: 'bar',
windows: [
{ shortWindowBurnRate: 1.0, longWindowBurnRate: 2.0 },
{ shortWindowBurnRate: 1.5, longWindowBurnRate: 1.1 },
],
groupings: {
group: { by: { field: 'bar' } },
},
},
];
esClientMock.search.mockResolvedValueOnce(
Expand Down Expand Up @@ -675,6 +719,9 @@ describe('BurnRateRuleExecutor', () => {
value: 'foo',
},
],
[ALERT_GROUPING]: {
group: { by: { field: 'foo' } },
},
},
});
expect(servicesMock.alertsClient!.report).toBeCalledWith({
Expand All @@ -697,6 +744,9 @@ describe('BurnRateRuleExecutor', () => {
value: 'bar',
},
],
[ALERT_GROUPING]: {
group: { by: { field: 'bar' } },
},
},
});

Expand Down Expand Up @@ -770,6 +820,7 @@ interface ResponseBucket {
shortWindowBurnRate: number;
longWindowBurnRate: number;
}>;
groupings?: Record<string, unknown> | undefined;
}

interface AfterKey {
Expand Down Expand Up @@ -818,6 +869,19 @@ function generateEsResponse(
{
key: { instanceId: bucket.instanceId },
doc_count: 100,
groupings: {
hits: {
hits: [
{
_source: {
slo: {
groupings: bucket.groupings,
},
},
},
],
},
},
} as EvaluationBucket
);
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,17 @@
*/

import numeral from '@elastic/numeral';
import { flattenObject } from '@kbn/object-utils';
import { AlertsClientError, ExecutorType, RuleExecutorOptions } from '@kbn/alerting-plugin/server';
import { ObservabilitySloAlert } from '@kbn/alerts-as-data-utils';
import { IBasePath } from '@kbn/core/server';
import { i18n } from '@kbn/i18n';
import { getEcsGroups } from '@kbn/alerting-rule-utils';
import { getFormattedGroups, getEcsGroupsFromFlattenGrouping } from '@kbn/alerting-rule-utils';
import { getAlertDetailsUrl } from '@kbn/observability-plugin/common';
import {
ALERT_EVALUATION_THRESHOLD,
ALERT_EVALUATION_VALUE,
ALERT_GROUPING,
ALERT_GROUP,
ALERT_REASON,
} from '@kbn/rule-data-utils';
Expand Down Expand Up @@ -122,14 +124,8 @@ export const getRuleExecutor = (basePath: IBasePath) =>
window: windowDef,
} = result;

const instances = instanceId.split(',');
const groups =
instanceId !== ALL_VALUE
? [slo.groupBy].flat().reduce<Group[]>((resultGroups, groupByItem, index) => {
resultGroups.push({ field: groupByItem, value: instances[index].trim() });
return resultGroups;
}, [])
: undefined;
const groupingsFlattened = flattenObject(groupings ?? {});
const groups = getFormattedGroups(groupingsFlattened);

const urlQuery = instanceId === ALL_VALUE ? '' : `?instanceId=${instanceId}`;
const viewInAppUrl = addSpaceIdToPath(
Expand Down Expand Up @@ -168,17 +164,17 @@ export const getRuleExecutor = (basePath: IBasePath) =>
actionGroup,
state: {
alertState: AlertStates.ALERT,
grouping: groupings,
},
payload: {
[ALERT_REASON]: reason,
[ALERT_EVALUATION_THRESHOLD]: windowDef.burnRateThreshold,
[ALERT_EVALUATION_VALUE]: Math.min(longWindowBurnRate, shortWindowBurnRate),
[ALERT_GROUP]: groups,
[ALERT_GROUPING]: groupings, // Object, example: { host: { name: 'host-0' } }
[SLO_ID_FIELD]: slo.id,
[SLO_REVISION_FIELD]: slo.revision,
[SLO_INSTANCE_ID_FIELD]: instanceId,
Comment thread
benakansara marked this conversation as resolved.
...getEcsGroups(groups),
...getEcsGroupsFromFlattenGrouping(groupingsFlattened),
},
});

Expand Down Expand Up @@ -226,16 +222,14 @@ export const getRuleExecutor = (basePath: IBasePath) =>
`/app/observability/slos/${slo.id}${urlQuery}`
);

const recoveredAlertState = recoveredAlert.alert.getState();

const context = {
timestamp: startedAt.toISOString(),
viewInAppUrl,
alertDetailsUrl,
sloId: slo.id,
sloName: slo.name,
sloInstanceId: alertId,
grouping: recoveredAlertState?.grouping,
grouping: recoveredAlert.hit?.[ALERT_GROUPING],
};

alertsClient.setAlertData({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,24 @@ export function sloBurnRateRuleType(
},
alerts: {
context: SLO_RULE_REGISTRATION_CONTEXT,
mappings: { fieldMap: { ...legacyExperimentalFieldMap, ...sloRuleFieldMap } },
mappings: {
fieldMap: {
...legacyExperimentalFieldMap,
...sloRuleFieldMap,
},
dynamicTemplates: [
{
strings_as_keywords: {
path_match: 'kibana.alert.grouping.*',
match_mapping_type: 'string',
mapping: {
type: 'keyword' as const,
ignore_above: 1024,
},
},
},
],
},
useEcs: true,
useLegacyAlerts: true,
shouldWrite: true,
Expand Down
1 change: 1 addition & 0 deletions x-pack/solutions/observability/plugins/slo/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -107,5 +107,6 @@
"@kbn/fields-metadata-plugin",
"@kbn/apm-sources-access-plugin",
"@kbn/lock-manager",
"@kbn/object-utils",
]
}