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

[RAM] Mark disabled alerts as Untracked in both Stack and o11y #164788

Merged
merged 62 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
a801e1e
[RAM] Mark disabled alerts as Untracked in both Stack and o11y
Zacqary Aug 24, 2023
8ac28c3
Add untracked filter to alerts list
Zacqary Aug 24, 2023
1db7232
Fix types
Zacqary Aug 24, 2023
7ef4cd7
Update registered task type test
Zacqary Aug 25, 2023
8488fe9
Fix disable rule event test
Zacqary Aug 25, 2023
a4ac597
Fix jest and typecheck
Zacqary Aug 25, 2023
5ff37f3
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Aug 25, 2023
3cf2e5a
Fix jest
Zacqary Aug 25, 2023
839d858
Fix jest snapshots
Zacqary Aug 25, 2023
d185d41
Fix registered task type test sorting
Zacqary Aug 25, 2023
9f37561
Fix event validator
Zacqary Aug 25, 2023
f4b61ff
Fix alert summary test
Zacqary Aug 25, 2023
d56ab0c
Fix rule type registry jest
Zacqary Aug 25, 2023
dcb5a77
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Aug 28, 2023
99b644f
Clean up typedef and add jest test for untrack
Zacqary Aug 28, 2023
72b9f60
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Aug 29, 2023
2db2f5f
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Aug 30, 2023
90acc70
Fix bad merge
Zacqary Aug 30, 2023
79d6ac2
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 11, 2023
1f243c6
Remove task spawner and use AAD methods to update alerts
Zacqary Sep 11, 2023
84f707f
Remove comment
Zacqary Sep 11, 2023
1de7e21
Type fix
Zacqary Sep 11, 2023
2903a3b
Fix typecheck
Zacqary Sep 11, 2023
87209f5
Fix Jest
Zacqary Sep 11, 2023
870a131
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 11, 2023
ac26389
Iterate over indicesAlertIdMap
Zacqary Sep 13, 2023
ce89b91
Refactor untrack function to just use ruleId
Zacqary Sep 13, 2023
ddb2cd1
Restore alertUuid
Zacqary Sep 13, 2023
bac60b8
Fix typecheck
Zacqary Sep 13, 2023
a18cf49
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 13, 2023
3ba23a2
Add logging for untrack failures
Zacqary Sep 14, 2023
2fd6a11
Move alertsService to rulesClient
Zacqary Sep 14, 2023
a8b37f2
Fix typecheck
Zacqary Sep 14, 2023
36283a5
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 18, 2023
dd412bd
Use autoRecoverAlerts as isLifecycleAlert
Zacqary Sep 18, 2023
dae99ee
Add test for untrackRuleIdsByIndices
Zacqary Sep 18, 2023
e7f3403
Typecheck fix
Zacqary Sep 18, 2023
0624ea6
Handle un-flattened AAD fields
Zacqary Sep 20, 2023
2d6ffd7
untrackRuleIdsByIndices -> setAlertStatusToUntracked
Zacqary Sep 20, 2023
2867b41
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 20, 2023
6259846
Fix lifecycle executor event log to support untracked alerts
Zacqary Sep 20, 2023
e1e74fa
Fix untracked alert table filter query
Zacqary Sep 20, 2023
7ff8935
Fix typecheck
Zacqary Sep 21, 2023
0b4aba7
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 21, 2023
cbb17c1
Fix typecheck
Zacqary Sep 21, 2023
895e9ac
Move alert state clearing into bulkDisable function
Zacqary Sep 25, 2023
ef6457f
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 25, 2023
5b384e6
Fix typecheck
Zacqary Sep 25, 2023
30d4d88
Fix jest
Zacqary Sep 25, 2023
c2ec732
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 25, 2023
f512be7
Fix tests
Zacqary Sep 26, 2023
f3407f1
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 26, 2023
645e1e6
Revert query test change
Zacqary Sep 26, 2023
e6ee727
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 26, 2023
29d3192
Allow bulkDisable to take array of task ids to remove
Zacqary Sep 26, 2023
b94e238
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 26, 2023
38e29a1
Revert "Allow bulkDisable to take array of task ids to remove"
Zacqary Sep 27, 2023
a669d03
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 27, 2023
f3b85df
Revert "Revert "Allow bulkDisable to take array of task ids to remove""
Zacqary Sep 27, 2023
cda79f1
Add isLifecycleAlert check to disable method
Zacqary Sep 27, 2023
8df838e
Merge remote-tracking branch 'upstream/main' into 164059-untracked-di…
Zacqary Sep 27, 2023
c6c1ef1
Fix jest
Zacqary Sep 27, 2023
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 @@ -9,7 +9,7 @@
import React, { memo } from 'react';
import { i18n } from '@kbn/i18n';
import { EuiBadge, EuiBadgeProps } from '@elastic/eui';
import { AlertStatus, ALERT_STATUS_RECOVERED } from '@kbn/rule-data-utils';
import { AlertStatus, ALERT_STATUS_RECOVERED, ALERT_STATUS_UNTRACKED } from '@kbn/rule-data-utils';

export interface AlertLifecycleStatusBadgeProps {
alertStatus: AlertStatus;
Expand Down Expand Up @@ -37,15 +37,27 @@ const FLAPPING_LABEL = i18n.translate(
}
);

const UNTRACKED_LABEL = i18n.translate(
'alertsUIShared.components.alertLifecycleStatusBadge.untrackedLabel',
{
defaultMessage: 'Untracked',
}
);

interface BadgeProps {
label: string;
color: string;
isDisabled?: boolean;
iconProps?: {
iconType: EuiBadgeProps['iconType'];
};
}

const getBadgeProps = (alertStatus: AlertStatus, flapping: boolean | undefined): BadgeProps => {
if (alertStatus === ALERT_STATUS_UNTRACKED) {
return { label: UNTRACKED_LABEL, color: 'default', isDisabled: true };
}

// Prefer recovered over flapping
if (alertStatus === ALERT_STATUS_RECOVERED) {
return {
Expand Down Expand Up @@ -82,10 +94,15 @@ export const AlertLifecycleStatusBadge = memo((props: AlertLifecycleStatusBadgeP

const castedFlapping = castFlapping(flapping);

const { label, color, iconProps } = getBadgeProps(alertStatus, castedFlapping);
const { label, color, iconProps, isDisabled } = getBadgeProps(alertStatus, castedFlapping);

return (
<EuiBadge data-test-subj="alertLifecycleStatusBadge" color={color} {...iconProps}>
<EuiBadge
data-test-subj="alertLifecycleStatusBadge"
isDisabled={isDisabled}
color={color}
{...iconProps}
>
{label}
</EuiBadge>
);
Expand Down
6 changes: 5 additions & 1 deletion packages/kbn-rule-data-utils/src/alerts_as_data_status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,9 @@

export const ALERT_STATUS_ACTIVE = 'active';
export const ALERT_STATUS_RECOVERED = 'recovered';
export const ALERT_STATUS_UNTRACKED = 'untracked';

export type AlertStatus = typeof ALERT_STATUS_ACTIVE | typeof ALERT_STATUS_RECOVERED;
export type AlertStatus =
| typeof ALERT_STATUS_ACTIVE
| typeof ALERT_STATUS_RECOVERED
| typeof ALERT_STATUS_UNTRACKED;
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/common/alert_summary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,5 @@ export interface AlertStatus {
activeStartDate?: string;
flapping: boolean;
maintenanceWindowIds?: string[];
tracked: boolean;
XavierM marked this conversation as resolved.
Show resolved Hide resolved
}
32 changes: 31 additions & 1 deletion x-pack/plugins/alerting/server/alerts_client/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@
*/

import { ElasticsearchClient } from '@kbn/core/server';
import { ALERT_RULE_UUID, ALERT_UUID } from '@kbn/rule-data-utils';
import {
ALERT_INSTANCE_ID,
ALERT_RULE_UUID,
ALERT_STATUS,
ALERT_STATUS_UNTRACKED,
ALERT_UUID,
} from '@kbn/rule-data-utils';
import { chunk, flatMap, isEmpty, keys } from 'lodash';
import { SearchRequest } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import type { Alert } from '@kbn/alerts-as-data-utils';
Expand Down Expand Up @@ -197,6 +203,30 @@ export class AlertsClient<
return { hits, total };
}

public async untrackAlertIdByIndices(alertId: string, indices: string[]) {
XavierM marked this conversation as resolved.
Show resolved Hide resolved
const esClient = await this.options.elasticsearchClientPromise;
for (const index of indices) {
try {
await esClient.updateByQuery({
XavierM marked this conversation as resolved.
Show resolved Hide resolved
index,
body: {
script: {
source: `ctx._source['${ALERT_STATUS}'] = '${ALERT_STATUS_UNTRACKED}'`,
Copy link
Contributor

Choose a reason for hiding this comment

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

For the index threshold & ES query rule type, the AAD docs are not flattened, so using this script, the resulting doc ends up with:

{
  "kibana.alert.status": "untracked",
  "kibana": {
    "alert": {
      "status": "active"
     }
  }
}

Is there any way to avoid this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it just those two rule types specifically or are there potentially more of them? Is there some kind of flag we can look for to know which rule types aren't flattened?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe anything written by the lifecycle registry is flattened, anything written by the alerting framework is not. I think you should be able to use the alerts definition registered with the rule type. If ruleType.alerts.shouldWrite=true it should be unflattened. If false or undefined, it should be flattened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, figured out a way to handle this in Painless

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better!

lang: 'painless',
},
query: {
term: {
[ALERT_INSTANCE_ID]: { value: alertId },
},
},
},
});
} catch (err) {
this.options.logger.error(`Error marking ${alertId} as untracked - ${err.message}`);
}
}
}

public report(
alert: ReportedAlert<
AlertData,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,4 +232,8 @@ export class LegacyAlertsClient<
}

public async persistAlerts() {}

public async untrackAlertIdByIndices() {
return;
}
}
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/alerts_client/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export interface IAlertsClient<
alertsToReturn: Record<string, RawAlertInstance>;
recoveredAlertsToReturn: Record<string, RawAlertInstance>;
};
untrackAlertIdByIndices(alertId: string, indices: string[]): Promise<void>;
factory(): PublicAlertFactory<
State,
Context,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ const rulesClientParams: jest.Mocked<ConstructorOptions> = {
minimumScheduleInterval: { value: '1m', enforce: false },
isAuthenticationTypeAPIKey: isAuthenticationTypeApiKeyMock,
getAuthenticationAPIKey: getAuthenticationApiKeyMock,
getAlertIndicesAlias: jest.fn(),
};
const paramsModifier = jest.fn();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ const rulesClientParams: jest.Mocked<ConstructorOptions> = {
minimumScheduleInterval: { value: '1m', enforce: false },
isAuthenticationTypeAPIKey: jest.fn(),
getAuthenticationAPIKey: jest.fn(),
getAlertIndicesAlias: jest.fn(),
};

beforeEach(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const rulesClientParams: jest.Mocked<ConstructorOptions> = {
minimumScheduleInterval: { value: '1m', enforce: false },
isAuthenticationTypeAPIKey: jest.fn(),
getAuthenticationAPIKey: jest.fn(),
getAlertIndicesAlias: jest.fn(),
};

const getMockAggregationResult = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": true,
"status": "OK",
"tracked": true,
"uuid": undefined,
},
"alert-2": Object {
Expand All @@ -135,6 +136,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": true,
"status": "OK",
"tracked": true,
"uuid": undefined,
},
},
Expand Down Expand Up @@ -241,6 +243,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "OK",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down Expand Up @@ -283,6 +286,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "OK",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down Expand Up @@ -324,6 +328,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "OK",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down Expand Up @@ -366,6 +371,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "Active",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down Expand Up @@ -408,6 +414,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "Active",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down Expand Up @@ -450,6 +457,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "Active",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down Expand Up @@ -490,6 +498,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "Active",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down Expand Up @@ -534,6 +543,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": true,
"status": "Active",
"tracked": true,
"uuid": "uuid-1",
},
"alert-2": Object {
Expand All @@ -542,6 +552,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": true,
"status": "OK",
"tracked": true,
"uuid": "uuid-2",
},
},
Expand Down Expand Up @@ -593,6 +604,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "Active",
"tracked": true,
"uuid": "uuid-1",
},
"alert-2": Object {
Expand All @@ -601,6 +613,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": false,
"muted": false,
"status": "OK",
"tracked": true,
"uuid": "uuid-2",
},
},
Expand Down Expand Up @@ -639,6 +652,7 @@ describe('alertSummaryFromEventLog', () => {
"flapping": true,
"muted": false,
"status": "Active",
"tracked": true,
"uuid": "uuid-1",
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ export function alertSummaryFromEventLog(params: AlertSummaryFromEventLogParams)
status.activeStartDate = undefined;
status.actionGroupId = undefined;
}

status.tracked = action !== EVENT_LOG_ACTIONS.untrackedInstance;
}

for (const event of executionEvents.reverse()) {
Expand Down Expand Up @@ -169,6 +171,7 @@ function getAlertStatus(
actionGroupId: undefined,
activeStartDate: undefined,
flapping: false,
tracked: true,
};
alerts.set(alertId, status);
return status;
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/alerting/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export const EVENT_LOG_ACTIONS = {
recoveredInstance: 'recovered-instance',
activeInstance: 'active-instance',
executeTimeout: 'execute-timeout',
untrackedInstance: 'untracked-instance',
};
export const LEGACY_EVENT_LOG_ACTIONS = {
resolvedInstance: 'resolved-instance',
Expand Down Expand Up @@ -494,6 +495,7 @@ export class AlertingPlugin {
eventLogger: this.eventLogger,
minimumScheduleInterval: this.config.rules.minimumScheduleInterval,
maxScheduledPerMinute: this.config.rules.maxScheduledPerMinute,
getAlertIndicesAlias: createGetAlertIndicesAliasFn(this.ruleTypeRegistry!),
});

rulesSettingsClientFactory.initialize({
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/rule_type_registry.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const createRuleTypeRegistryMock = () => {
list: jest.fn(),
getAllTypes: jest.fn(),
ensureRuleTypeEnabled: jest.fn(),
createAlertsClient: jest.fn(),
};
return mocked;
};
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/alerting/server/rule_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,10 @@ export class RuleTypeRegistry {
public getAllTypes(): string[] {
return [...this.ruleTypes.keys()];
}

public async createAlertsClient(...args: Parameters<AlertsService['createAlertsClient']>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by comment. I think it would make more sense to pass in the AlertsService to the RulesClientFactory -> RulesClient and use it directly there to create the alerts client rather than adding this pass through function in the rule type registry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. This PR has been ballooning out of control with so many different services and plugins getting added to new things, and core types getting modified, and the number of file changes exploding, that I've been doing everything I can to avoid not adding another thing that's going to trigger a typecheck failure across an unpredictable number of test files, but I guess we've already crossed that threshold. My instinct as a developer to keep PRs small does not work with the reality of the Kibana codebase.

I'll get to it.

return this.alertsService?.createAlertsClient(...args);
}
}

function normalizedActionVariables(actionVariables: RuleType['actionVariables']) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const rulesClientParams: jest.Mocked<RulesClientContext> = {
fieldsToExcludeFromPublicApi: [],
isAuthenticationTypeAPIKey: jest.fn(),
getAuthenticationAPIKey: jest.fn(),
getAlertIndicesAlias: jest.fn(),
};

const username = 'test';
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/alerting/server/rules_client/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export { getAuthorizationFilter } from './get_authorization_filter';
export { checkAuthorizationAndGetTotal } from './check_authorization_and_get_total';
export { scheduleTask } from './schedule_task';
export { createNewAPIKeySet } from './create_new_api_key_set';
export { recoverRuleAlerts } from './recover_rule_alerts';
export { untrackRuleAlerts } from './untrack_rule_alerts';
export { migrateLegacyActions } from './siem_legacy_actions/migrate_legacy_actions';
export { formatLegacyActions } from './siem_legacy_actions/format_legacy_actions';
export { addGeneratedActionValues } from './add_generated_action_values';
Expand Down
Loading