[SLO] Fix SLO burn rate rule to call alertWithLifecycle before `get…

…AlertUuid` (#169004) ## Summary This PR fixes #168962 by calling `alertWithLifecycle` before `getAlertUuid`. When `getAlertUuid` is called first it will generate a UUID which is not associated with the actual alert. When you call `alertWithLifecycle`, it generates the Alert then stores the `alertUuid` that maps the `alertId` to the `alertUuid`; `getAlertUuid` tries to recall this association but falls back to generating a UUID. I had to change the way the test worked by modifying mock's behavior to match the implementation; once I had a test that failed similarly to what we see in the real world, I fixed the issue by changing the implementation to call `alertWithLifecycle` first Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 48bb5ae)
elastic · Oct 16, 2023 · 5514b3f · 5514b3f
1 parent 714189f
commit 5514b3f
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 25 deletions.
diff --git a/x-pack/plugins/observability/server/lib/rules/slo_burn_rate/executor.test.ts b/x-pack/plugins/observability/server/lib/rules/slo_burn_rate/executor.test.ts
@@ -101,6 +101,8 @@ describe('BurnRateRuleExecutor', () => {
   let esClientMock: ElasticsearchClientMock;
   let soClientMock: jest.Mocked<SavedObjectsClientContract>;
   let loggerMock: jest.Mocked<MockedLogger>;
+  let alertUuidMap: Map<string, string>;
+  let alertMock: Partial<Alert>;
   const alertUuid = 'mockedAlertUuid';
   const basePathMock = { publicBaseUrl: 'https://kibana.dev' } as IBasePath;
   const alertsLocatorMock = {
@@ -124,9 +126,17 @@ describe('BurnRateRuleExecutor', () => {
     LifecycleAlertServices<BurnRateAlertState, BurnRateAlertContext, BurnRateAllowedActionGroups>;
 
   beforeEach(() => {
+    alertUuidMap = new Map<string, string>();
+    alertMock = {
+      scheduleActions: jest.fn(),
+      replaceState: jest.fn(),
+    };
     esClientMock = elasticsearchServiceMock.createElasticsearchClient();
     soClientMock = savedObjectsClientMock.create();
-    alertWithLifecycleMock = jest.fn();
+    alertWithLifecycleMock = jest.fn().mockImplementation(({ id }) => {
+      alertUuidMap.set(id, alertUuid);
+      return alertMock as any;
+    });
     alertFactoryMock = {
       create: jest.fn(),
       done: jest.fn(),
@@ -144,7 +154,7 @@ describe('BurnRateRuleExecutor', () => {
       shouldWriteAlerts: jest.fn(),
       shouldStopExecution: jest.fn(),
       getAlertStartedDate: jest.fn(),
-      getAlertUuid: jest.fn().mockImplementation(() => alertUuid),
+      getAlertUuid: jest.fn().mockImplementation((id) => alertUuidMap.get(id) || 'bad-uuid'),
       getAlertByAlertUuid: jest.fn(),
       share: {} as SharePluginStart,
       dataViews: dataViewPluginMocks.createStartContract(),
@@ -312,11 +322,6 @@ describe('BurnRateRuleExecutor', () => {
       esClientMock.search.mockResolvedValueOnce(
         generateEsResponse(ruleParams, [], { instanceId: 'bar' })
       );
-      const alertMock: Partial<Alert> = {
-        scheduleActions: jest.fn(),
-        replaceState: jest.fn(),
-      };
-      alertWithLifecycleMock.mockImplementation(() => alertMock as any);
       alertFactoryMock.done.mockReturnValueOnce({ getRecoveredAlerts: () => [] });
 
       const executor = getRuleExecutor({
@@ -417,11 +422,6 @@ describe('BurnRateRuleExecutor', () => {
       esClientMock.search.mockResolvedValueOnce(
         generateEsResponse(ruleParams, [], { instanceId: 'bar' })
       );
-      const alertMock: Partial<Alert> = {
-        scheduleActions: jest.fn(),
-        replaceState: jest.fn(),
-      };
-      alertWithLifecycleMock.mockImplementation(() => alertMock as any);
       alertFactoryMock.done.mockReturnValueOnce({ getRecoveredAlerts: () => [] });
 
       const executor = getRuleExecutor({ basePath: basePathMock });

diff --git a/x-pack/plugins/observability/server/lib/rules/slo_burn_rate/executor.ts b/x-pack/plugins/observability/server/lib/rules/slo_burn_rate/executor.ts
@@ -120,6 +120,17 @@ export const getRuleExecutor = ({
           );
 
           const alertId = instanceId;
+          const alert = alertWithLifecycle({
+            id: alertId,
+            fields: {
+              [ALERT_REASON]: reason,
+              [ALERT_EVALUATION_THRESHOLD]: windowDef.burnRateThreshold,
+              [ALERT_EVALUATION_VALUE]: Math.min(longWindowBurnRate, shortWindowBurnRate),
+              [SLO_ID_FIELD]: slo.id,
+              [SLO_REVISION_FIELD]: slo.revision,
+              [SLO_INSTANCE_ID_FIELD]: instanceId,
+            },
+          });
           const indexedStartedAt = getAlertStartedDate(alertId) ?? startedAt.toISOString();
           const alertUuid = getAlertUuid(alertId);
           const alertDetailsUrl = await getAlertUrl(
@@ -143,19 +154,6 @@ export const getRuleExecutor = ({
             sloInstanceId: instanceId,
           };
 
-          const alert = alertWithLifecycle({
-            id: alertId,
-
-            fields: {
-              [ALERT_REASON]: reason,
-              [ALERT_EVALUATION_THRESHOLD]: windowDef.burnRateThreshold,
-              [ALERT_EVALUATION_VALUE]: Math.min(longWindowBurnRate, shortWindowBurnRate),
-              [SLO_ID_FIELD]: slo.id,
-              [SLO_REVISION_FIELD]: slo.revision,
-              [SLO_INSTANCE_ID_FIELD]: instanceId,
-            },
-          });
-
           alert.scheduleActions(windowDef.actionGroup, context);
           alert.replaceState({ alertState: AlertStates.ALERT });
         }