From bb58a516bd327f9aff310fb07a0f35e76b6e6350 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 20 Jul 2023 13:37:06 +0100 Subject: [PATCH] feat: improve slack app addon scalability (#4284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__snapshots__/slack-app.test.ts.snap | 8 +- src/lib/addons/addon.ts | 2 + src/lib/addons/slack-app.test.ts | 225 ++++++++++++------ src/lib/addons/slack-app.ts | 148 ++++++++++-- src/lib/server-impl.ts | 1 + src/lib/services/addon-service.ts | 6 + 6 files changed, 291 insertions(+), 99 deletions(-) diff --git a/src/lib/addons/__snapshots__/slack-app.test.ts.snap b/src/lib/addons/__snapshots__/slack-app.test.ts.snap index 796b0787f4..339251ad53 100644 --- 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": [ { diff --git a/src/lib/addons/addon.ts b/src/lib/addons/addon.ts index 907f31b585..740400134f 100644 --- 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; + + destroy?(): void; } diff --git a/src/lib/addons/slack-app.test.ts b/src/lib/addons/slack-app.test.ts index 73880612bd..7bd25c35f5 100644 --- 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.`, + ); + }); }); diff --git a/src/lib/addons/slack-app.ts b/src/lib/addons/slack-app.ts index b44cfd1696..a478167815 100644 --- 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 { - 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; diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index fabbff41b8..678e3c98ac 100644 --- 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(); }; diff --git a/src/lib/services/addon-service.ts b/src/lib/services/addon-service.ts index 97bd695dac..3bbaaec8ab 100644 --- 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?.(), + ); + } }