feat: improve slack app addon scalability (#4284)

https://linear.app/unleash/issue/2-1237/explore-slack-app-addon-scalability-and-limitations Relevant document: https://linear.app/unleash/document/894e12b7-802c-4bc5-8c22-75af0e66fa4b - Implements 30s cache layer for Slack channels; - Adds error logging; - Adds respective tests; - Slight refactors and improvements for overall robustness; --------- Co-authored-by: Gastón Fournier <gaston@getunleash.io>
2025-01-11 00:08:30 +01:00 · 2023-07-20 13:37:06 +01:00 · 2023-07-20 13:37:06 +01:00 · bb58a516bd
commit bb58a516bd
parent a53d50148b
6 changed files with 291 additions and 99 deletions
--- a/src/lib/addons/snapshots/slack-app.test.ts.snap
+++ b/src/lib/addons/snapshots/slack-app.test.ts.snap
@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

-exports[`Should not post to unexisting tagged channels 1`] = `
+exports[`SlackAppAddon should not post to unexisting tagged channels 1`] = `
 {
  "attachments": [
    {
@ -21,7 +21,7 @@ exports[`Should not post to unexisting tagged channels 1`] = `
 }
 `;

-exports[`Should post message when feature is toggled 1`] = `
+exports[`SlackAppAddon should post message when feature is toggled 1`] = `
 {
  "attachments": [
    {
@ -42,7 +42,7 @@ exports[`Should post message when feature is toggled 1`] = `
 }
 `;

-exports[`Should post to all channels in tags 1`] = `
+exports[`SlackAppAddon should post to all channels in tags 1`] = `
 {
  "attachments": [
    {
@ -63,7 +63,7 @@ exports[`Should post to all channels in tags 1`] = `
 }
 `;

-exports[`Should post to all channels in tags 2`] = `
+exports[`SlackAppAddon should post to all channels in tags 2`] = `
 {
  "attachments": [
    {
--- a/src/lib/addons/addon.ts
+++ b/src/lib/addons/addon.ts
@ -66,4 +66,6 @@ export default abstract class Addon {

    // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
    abstract handleEvent(event: IEvent, parameters: any): Promise<void>;
+
+    destroy?(): void;
 }
--- a/src/lib/addons/slack-app.test.ts
+++ b/src/lib/addons/slack-app.test.ts
@ -1,16 +1,18 @@
 import { IEvent, FEATURE_ENVIRONMENT_ENABLED } from '../types/events';
-
 import SlackAppAddon from './slack-app';
+import { ChatPostMessageArguments, ErrorCode } from '@slack/web-api';

-import noLogger from '../../test/fixtures/no-logger';
-import { ChatPostMessageArguments } from '@slack/web-api';
-
-const accessToken = 'test-access-token';
 const slackApiCalls: ChatPostMessageArguments[] = [];
+const conversationsList = jest.fn();
+let postMessage = jest.fn().mockImplementation((options) => {
+    slackApiCalls.push(options);
+    return Promise.resolve();
+});
+
 jest.mock('@slack/web-api', () => ({
    WebClient: jest.fn().mockImplementation(() => ({
        conversations: {
-            list: () => ({
+            list: conversationsList.mockImplementation(() => ({
                channels: [
                    {
                        id: 1,
@ -20,27 +22,42 @@ jest.mock('@slack/web-api', () => ({
                        id: 2,
                        name: 'another-channel-1',
                    },
+                    {
+                        id: 3,
+                        name: 'another-channel-2',
+                    },
                ],
-            }),
+            })),
        },
        chat: {
-            postMessage: jest.fn().mockImplementation((options) => {
-                slackApiCalls.push(options);
-                return Promise.resolve();
-            }),
+            postMessage,
        },
+        on: jest.fn(),
    })),
+    ErrorCode: {
+        PlatformError: 'slack_webapi_platform_error',
+    },
+    WebClientEvent: {
+        RATE_LIMITED: 'rate_limited',
+    },
 }));

-beforeEach(() => {
-    slackApiCalls.length = 0;
-});
+describe('SlackAppAddon', () => {
+    let addon;
+    const accessToken = 'test-access-token';
+    const loggerMock = {
+        debug: jest.fn(),
+        info: jest.fn(),
+        warn: jest.fn(),
+        error: jest.fn(),
+        fatal: jest.fn(),
+    };
+    const getLogger = jest.fn(() => loggerMock);
+    const mockError = {
+        code: ErrorCode.PlatformError,
+        data: 'Platform error message',
+    };

-test('Should post message when feature is toggled', async () => {
-    const addon = new SlackAppAddon({
-        getLogger: noLogger,
-        unleashUrl: 'http://some-url.com',
-    });
    const event: IEvent = {
        id: 1,
        createdAt: new Date(),
@ -58,66 +75,120 @@ test('Should post message when feature is toggled', async () => {
        tags: [{ type: 'slack', value: 'general' }],
    };

-    await addon.handleEvent(event, { accessToken });
-    expect(slackApiCalls.length).toBe(1);
-    expect(slackApiCalls[0].channel).toBe(1);
-    expect(slackApiCalls[0]).toMatchSnapshot();
-});
-
-test('Should post to all channels in tags', async () => {
-    const addon = new SlackAppAddon({
-        getLogger: noLogger,
-        unleashUrl: 'http://some-url.com',
+    beforeEach(() => {
+        jest.useFakeTimers();
+        slackApiCalls.length = 0;
+        conversationsList.mockClear();
+        addon = new SlackAppAddon({
+            getLogger,
+            unleashUrl: 'http://some-url.com',
+        });
    });
-    const event: IEvent = {
-        id: 2,
-        createdAt: new Date(),
-        type: FEATURE_ENVIRONMENT_ENABLED,
-        createdBy: 'some@user.com',
-        project: 'default',
-        featureName: 'some-toggle',
-        environment: 'development',
-        data: {
-            name: 'some-toggle',
-        },
-        tags: [
-            { type: 'slack', value: 'general' },
-            { type: 'slack', value: 'another-channel-1' },
-        ],
-    };

-    await addon.handleEvent(event, { accessToken });
-    expect(slackApiCalls.length).toBe(2);
-    expect(slackApiCalls[0].channel).toBe(1);
-    expect(slackApiCalls[0]).toMatchSnapshot();
-    expect(slackApiCalls[1].channel).toBe(2);
-    expect(slackApiCalls[1]).toMatchSnapshot();
-});
-
-test('Should not post to unexisting tagged channels', async () => {
-    const addon = new SlackAppAddon({
-        getLogger: noLogger,
-        unleashUrl: 'http://some-url.com',
+    afterEach(() => {
+        jest.useRealTimers();
+        addon.destroy();
    });
-    const event: IEvent = {
-        id: 3,
-        createdAt: new Date(),
-        type: FEATURE_ENVIRONMENT_ENABLED,
-        createdBy: 'some@user.com',
-        project: 'default',
-        featureName: 'some-toggle',
-        environment: 'development',
-        data: {
-            name: 'some-toggle',
-        },
-        tags: [
-            { type: 'slack', value: 'random' },
-            { type: 'slack', value: 'another-channel-1' },
-        ],
-    };

-    await addon.handleEvent(event, { accessToken });
-    expect(slackApiCalls.length).toBe(1);
-    expect(slackApiCalls[0].channel).toBe(2);
-    expect(slackApiCalls[0]).toMatchSnapshot();
+    it('should post message when feature is toggled', async () => {
+        await addon.handleEvent(event, { accessToken });
+
+        expect(slackApiCalls.length).toBe(1);
+        expect(slackApiCalls[0].channel).toBe(1);
+        expect(slackApiCalls[0]).toMatchSnapshot();
+    });
+
+    it('should post to all channels in tags', async () => {
+        const eventWith2Tags: IEvent = {
+            ...event,
+            tags: [
+                { type: 'slack', value: 'general' },
+                { type: 'slack', value: 'another-channel-1' },
+            ],
+        };
+
+        await addon.handleEvent(eventWith2Tags, { accessToken });
+
+        expect(slackApiCalls.length).toBe(2);
+        expect(slackApiCalls[0].channel).toBe(1);
+        expect(slackApiCalls[0]).toMatchSnapshot();
+        expect(slackApiCalls[1].channel).toBe(2);
+        expect(slackApiCalls[1]).toMatchSnapshot();
+    });
+
+    it('should not post to unexisting tagged channels', async () => {
+        const eventWithUnexistingTaggedChannel: IEvent = {
+            ...event,
+            tags: [
+                { type: 'slack', value: 'random' },
+                { type: 'slack', value: 'another-channel-1' },
+            ],
+        };
+
+        await addon.handleEvent(eventWithUnexistingTaggedChannel, {
+            accessToken,
+        });
+
+        expect(slackApiCalls.length).toBe(1);
+        expect(slackApiCalls[0].channel).toBe(2);
+        expect(slackApiCalls[0]).toMatchSnapshot();
+    });
+
+    it('should cache Slack channels', async () => {
+        await addon.handleEvent(event, { accessToken });
+        await addon.handleEvent(event, { accessToken });
+
+        expect(slackApiCalls.length).toBe(2);
+        expect(conversationsList).toHaveBeenCalledTimes(1);
+    });
+
+    it('should refresh Slack channels cache after 30 seconds', async () => {
+        await addon.handleEvent(event, { accessToken });
+
+        jest.advanceTimersByTime(30000);
+
+        await addon.handleEvent(event, { accessToken });
+
+        expect(slackApiCalls.length).toBe(2);
+        expect(conversationsList).toHaveBeenCalledTimes(2);
+    });
+
+    it('should log error when an API call fails', async () => {
+        postMessage = jest.fn().mockRejectedValue(mockError);
+
+        await addon.handleEvent(event, { accessToken });
+
+        expect(loggerMock.warn).toHaveBeenCalledWith(
+            `Error handling event ${event.type}. A platform error occurred: Platform error message`,
+            expect.any(Object),
+        );
+    });
+
+    it('should handle rejections in chat.postMessage', async () => {
+        const eventWith3Tags: IEvent = {
+            ...event,
+            tags: [
+                { type: 'slack', value: 'general' },
+                { type: 'slack', value: 'another-channel-1' },
+                { type: 'slack', value: 'another-channel-2' },
+            ],
+        };
+
+        postMessage = jest
+            .fn()
+            .mockResolvedValueOnce({ ok: true })
+            .mockResolvedValueOnce({ ok: true })
+            .mockRejectedValueOnce(mockError);
+
+        await addon.handleEvent(eventWith3Tags, { accessToken });
+
+        expect(postMessage).toHaveBeenCalledTimes(3);
+        expect(loggerMock.warn).toHaveBeenCalledWith(
+            `Error handling event ${FEATURE_ENVIRONMENT_ENABLED}. A platform error occurred: Platform error message`,
+            expect.any(Object),
+        );
+        expect(loggerMock.info).toHaveBeenCalledWith(
+            `Handled event ${FEATURE_ENVIRONMENT_ENABLED} dispatching 2 out of 3 messages successfully.`,
+        );
+    });
 });
--- a/src/lib/addons/slack-app.ts
+++ b/src/lib/addons/slack-app.ts
@ -1,4 +1,14 @@
-import { WebClient } from '@slack/web-api';
+import {
+    WebClient,
+    ConversationsListResponse,
+    ErrorCode,
+    WebClientEvent,
+    CodedError,
+    WebAPIPlatformError,
+    WebAPIRequestError,
+    WebAPIRateLimitedError,
+    WebAPIHTTPError,
+} from '@slack/web-api';
 import Addon from './addon';

 import slackAppDefinition from './slack-app-definition';
@ -11,46 +21,86 @@ import {
 } from './feature-event-formatter-md';
 import { IEvent } from '../types/events';

+const CACHE_SECONDS = 30;
+
 interface ISlackAppAddonParameters {
    accessToken: string;
 }
+
 export default class SlackAppAddon extends Addon {
    private msgFormatter: FeatureEventFormatter;

+    private accessToken?: string;
+
    private slackClient?: WebClient;

+    private slackChannels?: ConversationsListResponse['channels'];
+
+    private slackChannelsCacheTimeout?: NodeJS.Timeout;
+
    constructor(args: IAddonConfig) {
        super(slackAppDefinition, args);
        this.msgFormatter = new FeatureEventFormatterMd(
            args.unleashUrl,
            LinkStyle.SLACK,
        );
+        this.startCacheInvalidation();
    }

    async handleEvent(
        event: IEvent,
        parameters: ISlackAppAddonParameters,
    ): Promise<void> {
-        const { accessToken } = parameters;
+        try {
+            const { accessToken } = parameters;
+            if (!accessToken) {
+                this.logger.warn('No access token provided.');
+                return;
+            }

-        if (!accessToken) return;
+            const taggedChannels = this.findTaggedChannels(event);
+            if (!taggedChannels.length) {
+                this.logger.debug(
+                    `No Slack channels tagged for event ${event.type}`,
+                    event,
+                );
+                return;
+            }

-        if (!this.slackClient) {
-            this.slackClient = new WebClient(accessToken);
-        }
+            if (!this.slackClient || this.accessToken !== accessToken) {
+                const client = new WebClient(accessToken);
+                client.on(WebClientEvent.RATE_LIMITED, (numSeconds) => {
+                    this.logger.debug(
+                        `Rate limit reached for event ${event.type}. Retry scheduled after ${numSeconds} seconds`,
+                    );
+                });
+                this.slackClient = client;
+                this.accessToken = accessToken;
+            }

-        const slackChannels = await this.slackClient.conversations.list({
-            types: 'public_channel,private_channel',
-        });
-        const taggedChannels = this.findTaggedChannels(event);
+            if (!this.slackChannels) {
+                const slackConversationsList =
+                    await this.slackClient.conversations.list({
+                        types: 'public_channel,private_channel',
+                    });
+                this.slackChannels = slackConversationsList.channels || [];
+                this.logger.debug(
+                    `Fetched ${this.slackChannels.length} Slack channels`,
+                );
+            }

-        if (slackChannels.channels?.length && taggedChannels.length) {
-            const slackChannelsToPostTo = slackChannels.channels.filter(
-                ({ id, name }) => id && name && taggedChannels.includes(name),
-            );
+            const currentSlackChannels = [...this.slackChannels];
+            if (!currentSlackChannels.length) {
+                this.logger.warn('No Slack channels found.');
+                return;
+            }

            const text = this.msgFormatter.format(event);
-            const featureLink = this.msgFormatter.featureLink(event);
+            const url = this.msgFormatter.featureLink(event);
+
+            const slackChannelsToPostTo = currentSlackChannels.filter(
+                ({ id, name }) => id && name && taggedChannels.includes(name),
+            );

            const requests = slackChannelsToPostTo.map(({ id }) =>
                this.slackClient!.chat.postMessage({
@ -65,7 +115,7 @@ export default class SlackAppAddon extends Addon {
                                    type: 'button',
                                    value: 'featureToggle',
                                    style: 'primary',
-                                    url: featureLink,
+                                    url,
                                },
                            ],
                        },
@ -73,8 +123,22 @@ export default class SlackAppAddon extends Addon {
                }),
            );

-            await Promise.all(requests);
-            this.logger.info(`Handled event ${event.type}.`);
+            const results = await Promise.allSettled(requests);
+
+            results
+                .filter(({ status }) => status === 'rejected')
+                .map(({ reason }: PromiseRejectedResult) =>
+                    this.logError(event, reason),
+                );
+
+            this.logger.info(
+                `Handled event ${event.type} dispatching ${
+                    results.filter(({ status }) => status === 'fulfilled')
+                        .length
+                } out of ${requests.length} messages successfully.`,
+            );
+        } catch (error) {
+            this.logError(event, error);
        }
    }

@ -86,6 +150,54 @@ export default class SlackAppAddon extends Addon {
        }
        return [];
    }
+
+    startCacheInvalidation(): void {
+        this.slackChannelsCacheTimeout = setInterval(() => {
+            this.slackChannels = undefined;
+        }, CACHE_SECONDS * 1000);
+    }
+
+    logError(event: IEvent, error: Error | CodedError): void {
+        if (!('code' in error)) {
+            this.logger.warn(`Error handling event ${event.type}.`, error);
+            return;
+        }
+
+        if (error.code === ErrorCode.PlatformError) {
+            const { data } = error as WebAPIPlatformError;
+            this.logger.warn(
+                `Error handling event ${event.type}. A platform error occurred: ${data}`,
+                error,
+            );
+        } else if (error.code === ErrorCode.RequestError) {
+            const { original } = error as WebAPIRequestError;
+            this.logger.warn(
+                `Error handling event ${event.type}. A request error occurred: ${original}`,
+                error,
+            );
+        } else if (error.code === ErrorCode.RateLimitedError) {
+            const { retryAfter } = error as WebAPIRateLimitedError;
+            this.logger.warn(
+                `Error handling event ${event.type}. A rate limit error occurred: retry after ${retryAfter} seconds`,
+                error,
+            );
+        } else if (error.code === ErrorCode.HTTPError) {
+            const { statusCode } = error as WebAPIHTTPError;
+            this.logger.warn(
+                `Error handling event ${event.type}. An HTTP error occurred: status code ${statusCode}`,
+                error,
+            );
+        } else {
+            this.logger.warn(`Error handling event ${event.type}.`, error);
+        }
+    }
+
+    destroy(): void {
+        if (this.slackChannelsCacheTimeout) {
+            clearInterval(this.slackChannelsCacheTimeout);
+            this.slackChannelsCacheTimeout = undefined;
+        }
+    }
 }

 module.exports = SlackAppAddon;
--- a/src/lib/server-impl.ts
+++ b/src/lib/server-impl.ts
@ -59,6 +59,7 @@ async function createApp(
        stores.clientInstanceStore.destroy();
        services.clientMetricsServiceV2.destroy();
        services.proxyService.destroy();
+        services.addonService.destroy();
        await db.destroy();
    };

--- a/src/lib/services/addon-service.ts
+++ b/src/lib/services/addon-service.ts
@ -301,4 +301,10 @@ export default class AddonService {
        }
        return true;
    }
+
+    destroy(): void {
+        Object.values(this.addonProviders).forEach((addon) =>
+            addon.destroy?.(),
+        );
+    }
 }