[Response Ops][Alerting] Creating global service for fetching and caching rules settings #192404

ymao1 · 2024-09-09T19:13:27Z

Summary

Created a RulesSettingsService that caches flapping and query delay settings per space for 1 minute so rules are not accessing the SO index with every execution.

Also moved all classes related to the rules settings into the rules_settings folder so some of these changes are just renaming changes.

To Verify

Add some logging to x-pack/plugins/alerting/server/rules_settings/rules_settings_service.ts to indicate when settings are being fetched and when they're returning from the cache.
Create and run some rules in different spaces to see that flapping and query delay settings are being returned from the cache when the cache has not expired.

…rules-settings

ymao1 · 2024-09-10T13:43:06Z

x-pack/plugins/alerting/server/plugin.ts

@@ -588,33 +591,36 @@ export class AlertingPlugin {
    };

    taskRunnerFactory.initialize({
-      logger,
+      actionsConfigMap: getActionsConfigMap(this.config.rules.run.actions),


alphabetizing 🙈

…rules-settings

elasticmachine · 2024-09-10T17:01:32Z

Pinging @elastic/response-ops (Team:ResponseOps)

ymao1 · 2024-09-10T20:47:34Z

x-pack/plugins/alerting/server/plugin.ts

+      rulesSettingsService: new RulesSettingsService({
+        getRulesSettingsClientWithRequest,
+        isServerless: !!plugins.serverless,
+      }),


actual changes are to pass this new global rules setting service to the task runner factory and to remove getRulesSettingsClientWithRequest

guskovaue

LGTM! Tested locally, works as expected!

pmuellr

Generally LGTM, but feels like x-pack/plugins/alerting/server/rules_settings/rules_settings_service.test.ts could use some additional tests. Also noted a potential design change to use just one map instead of three, within the service.

pmuellr · 2024-09-13T02:13:13Z

.../alerting_api_integration/spaces_only/tests/alerting/group4/alerts_as_data/alerts_as_data.ts

@@ -86,6 +87,8 @@ export default function createAlertsAsDataInstallResourcesTest({ getService }: F

      it(`should write alert docs during rule execution with flapping.enabled: ${enableFlapping}`, async () => {
        await setFlappingSettings(enableFlapping);
+        // wait so cache expires
+        await setTimeoutAsync(60000);


a minute??? Oh noes ...

I wonder ... could we write a new route for FTR that we could call to modify the value? Doesn't need to be in this PR, but it is running this test twice so we just added 2m to this FTR group, it pains my heart :-)

And there are some more below :-)

Updated in cf77b09

pmuellr · 2024-09-13T02:36:52Z

x-pack/plugins/alerting/server/rules_settings/rules_settings_service.ts

+
+export class RulesSettingsService {
+  private defaultQueryDelaySettings: RulesSettingsQueryDelayProperties;
+  private settingsLastUpdated: Map<string, number> = new Map();


Wondering if it would be better to have one map, that had the lastUpdated, queryDelaySettings, and flappingSettings in it, vs 3 maps. Seems like we'd have a better chance of keeping everything in sync over time. Probably easier to add new settings as well.

Updated in f2aa813

pmuellr · 2024-09-13T02:39:02Z

x-pack/plugins/alerting/server/rules_settings/rules_settings_service.ts

+      this.flappingSettings.has(spaceId) &&
+      this.queryDelaySettings.has(spaceId)
+    ) {
+      const lastUpdated = new Date(this.settingsLastUpdated.get(spaceId)!).getTime();


critical field! You can imagine this getting messed up and then we never refresh the cache, or we refresh it every time. Everything looks good here, more of a note for myself :-)

pmuellr · 2024-09-13T02:39:54Z

x-pack/plugins/alerting/server/rules_settings/rules_settings_service.ts

+      const currentFlappingSettings = this.flappingSettings.get(spaceId)!;
+      const currentQueryDelaySettings = this.queryDelaySettings.get(spaceId)!;
+
+      if (now - lastUpdated >= CACHE_INTERVAL_MS) {


We'll want to parameterize this somehow, to let us do the FTRs without all the 1m timeouts ...

Updated in cf77b09

pmuellr · 2024-09-13T02:40:47Z

x-pack/plugins/alerting/server/rules_settings/rules_settings_service.ts

+          return await this.fetchSettings(request, spaceId, now);
+        } catch (err) {
+          // return cached settings on error
+          return {


It looks like we're logging errors on the inner calls, but seems kinda scary to not have anything here. Feels like maybe having a debug logs might good. The existing comments seem like good locations and text for those logs.

Updated in a45e6a3

pmuellr · 2024-09-13T14:52:04Z

x-pack/plugins/alerting/server/rules_settings/rules_settings_service.test.ts

+        expect(rulesSettingsService.queryDelaySettings.get('default')).toEqual({ delay: 0 });
+        // @ts-ignore - accessing private variable
+        expect(rulesSettingsService.flappingSettings.get('default')).toEqual({
+          enabled: true,


We can make this a constant, save some room.

Also, we don't seem to be testing any other values that this. Seems like we should have a test like the one where we test the changed query delay.

Updated in 60ce3e6

pmuellr · 2024-09-13T14:55:16Z

x-pack/plugins/alerting/server/rules_settings/rules_settings_service.test.ts

+      });
+
+      test('should use cached settings if cache has not expired', async () => {
+        const rulesSettingsClient = rulesSettingsClientMock.create();


Shouldn't we set the values to non-default values, to make sure we end up getting the cached ones, instead of default? Seems appropriate for all the "cached" tests below ...

Updated in 60ce3e6

…rules-settings

pmuellr

LGTM! Thanks for the changes!

ymao1 · 2024-09-16T11:43:43Z

@elasticmachine merge upstream

kibana-ci · 2024-09-16T13:21:06Z

💚 Build Succeeded

Buildkite Build
Commit: c8a4f8a

Metrics [docs]

✅ unchanged

History

💚 Build #234157 succeeded cf77b09
💛 Build #233154 was flaky c680f54
💔 Build #233097 failed 5a2fd2b
💔 Build #232939 failed 95578cb

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ymao1

…hing rules settings (elastic#192404) Resolves elastic#184321 and elastic#149884 ## Summary Created a `RulesSettingsService` that caches flapping and query delay settings per space for 1 minute so rules are not accessing the SO index with every execution. Also moved all classes related to the rules settings into the `rules_settings` folder so some of these changes are just renaming changes. ## To Verify 1. Add some logging to `x-pack/plugins/alerting/server/rules_settings/rules_settings_service.ts` to indicate when settings are being fetched and when they're returning from the cache. 2. Create and run some rules in different spaces to see that flapping and query delay settings are being returned from the cache when the cache has not expired. --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit 7a0fe2e)

kibanamachine · 2024-09-16T13:27:40Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…nd caching rules settings (#192404) (#193011) # Backport This will backport the following commits from `main` to `8.x`: - [[Response Ops][Alerting] Creating global service for fetching and caching rules settings (#192404)](#192404)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Ying Mao <[email protected]>

…tching and caching rules settings (elastic#192404) (elastic#193011)" This reverts commit 7939f22.

ymao1 added 5 commits September 9, 2024 15:13

Moving all existing rules setting files into rules settings folders

9648a30

Adding rules settings service

fc13dfc

Caching rule settings

95578cb

Fixing flapping test

142ce80

Merge branch 'main' of github.com:elastic/kibana into alerting/cache-…

5a2fd2b

…rules-settings

ymao1 changed the title ~~Moving all existing rules setting files into rules settings folders~~ [Response Ops][Alerting] Creating global service for fetching and caching rules settings Sep 10, 2024

ymao1 self-assigned this Sep 10, 2024

ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 labels Sep 10, 2024

ymao1 commented Sep 10, 2024

View reviewed changes

ymao1 added 2 commits September 10, 2024 11:25

Fixing functional test

19bc6a0

Merge branch 'main' of github.com:elastic/kibana into alerting/cache-…

c680f54

…rules-settings

ymao1 marked this pull request as ready for review September 10, 2024 17:01

ymao1 requested a review from a team as a code owner September 10, 2024 17:01

ymao1 requested review from pmuellr and guskovaue September 10, 2024 17:01

ymao1 commented Sep 10, 2024

View reviewed changes

cnasikas mentioned this pull request Sep 11, 2024

[Response Ops][Flapping] Rule Specific Flapping - Create/Update API changes #190019

Merged

1 task

guskovaue approved these changes Sep 13, 2024

View reviewed changes

pmuellr requested changes Sep 13, 2024

View reviewed changes

ymao1 added 5 commits September 13, 2024 13:25

Merge branch 'main' of github.com:elastic/kibana into alerting/cache-…

3f386d1

…rules-settings

PR feedback - combine 3 maps into 1

f2aa813

PR feedback - debug logging on fetch error

a45e6a3

PR feedback - improving unit tests

60ce3e6

PR feedback - configurable cache interval

cf77b09

ymao1 requested a review from pmuellr September 13, 2024 18:17

pmuellr approved these changes Sep 13, 2024

View reviewed changes

Merge branch 'main' into alerting/cache-rules-settings

c8a4f8a

ymao1 added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v9.0.0 labels Sep 16, 2024

ymao1 merged commit 7a0fe2e into elastic:main Sep 16, 2024
38 checks passed

ymao1 deleted the alerting/cache-rules-settings branch September 16, 2024 13:22

ymao1 mentioned this pull request Sep 16, 2024

Optimize access to flapping settings in the task runner #149884

Closed

kibanamachine mentioned this pull request Sep 16, 2024

[8.x] [Response Ops][Alerting] Creating global service for fetching and caching rules settings (#192404) #193011

Merged

mikecote added a commit to mikecote/kibana that referenced this pull request Sep 20, 2024

Revert "[8.x] [Response Ops][Alerting] Creating global service for fe…

7655d92

…tching and caching rules settings (elastic#192404) (elastic#193011)" This reverts commit 7939f22.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Response Ops][Alerting] Creating global service for fetching and caching rules settings #192404

[Response Ops][Alerting] Creating global service for fetching and caching rules settings #192404

ymao1 commented Sep 9, 2024 •

edited by kibanamachine

Loading

ymao1 Sep 10, 2024

elasticmachine commented Sep 10, 2024

ymao1 Sep 10, 2024

guskovaue left a comment

pmuellr left a comment

pmuellr Sep 13, 2024

ymao1 Sep 13, 2024

pmuellr Sep 13, 2024

ymao1 Sep 13, 2024

pmuellr Sep 13, 2024

pmuellr Sep 13, 2024

ymao1 Sep 13, 2024

pmuellr Sep 13, 2024

ymao1 Sep 13, 2024

pmuellr Sep 13, 2024

ymao1 Sep 13, 2024 •

edited

Loading

pmuellr Sep 13, 2024

ymao1 Sep 13, 2024

pmuellr left a comment

ymao1 commented Sep 16, 2024

kibana-ci commented Sep 16, 2024

kibanamachine commented Sep 16, 2024

[Response Ops][Alerting] Creating global service for fetching and caching rules settings #192404

[Response Ops][Alerting] Creating global service for fetching and caching rules settings #192404

Conversation

ymao1 commented Sep 9, 2024 • edited by kibanamachine Loading

Summary

To Verify

Choose a reason for hiding this comment

elasticmachine commented Sep 10, 2024

Choose a reason for hiding this comment

guskovaue left a comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymao1 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

ymao1 commented Sep 16, 2024

kibana-ci commented Sep 16, 2024

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Sep 16, 2024

💚 All backports created successfully

Questions ?

ymao1 commented Sep 9, 2024 •

edited by kibanamachine

Loading

ymao1 Sep 13, 2024 •

edited

Loading