From 96e3be74c212c575c6c593dd4b85e5b280c35fd3 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Thu, 13 May 2021 16:16:42 +0200 Subject: [PATCH 1/6] Minor fixes from last round of review. Signed-off-by: Eric Peterson --- packages/backend-common/src/cache/CacheClient.ts | 3 ++- packages/backend-common/src/cache/CacheManager.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/backend-common/src/cache/CacheClient.ts b/packages/backend-common/src/cache/CacheClient.ts index 49b5c367dd..3b654e248b 100644 --- a/packages/backend-common/src/cache/CacheClient.ts +++ b/packages/backend-common/src/cache/CacheClient.ts @@ -92,7 +92,8 @@ export class DefaultCacheClient implements CacheClient { const wellFormedKey = Buffer.from(candidateKey).toString('base64'); // Memcache in particular doesn't do well with keys > 250 bytes. - if (wellFormedKey.length < 250) { + // Padded because a plugin ID is also prepended to the key. + if (wellFormedKey.length < 200) { return wellFormedKey; } diff --git a/packages/backend-common/src/cache/CacheManager.ts b/packages/backend-common/src/cache/CacheManager.ts index fbd7e3ba0a..50788be56c 100644 --- a/packages/backend-common/src/cache/CacheManager.ts +++ b/packages/backend-common/src/cache/CacheManager.ts @@ -65,7 +65,7 @@ export class CacheManager { } /** - * Generates a CacheManagerInstance for consumption by plugins. + * Generates a PluginCacheManager for consumption by plugins. * * @param pluginId The plugin that the cache manager should be created for. Plugin names should be unique. */ From 3839c2d9bcc9607cb0655a3b354a98c2bb215c7f Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Thu, 13 May 2021 17:38:47 +0200 Subject: [PATCH 2/6] Add an onError handler to getClient options Signed-off-by: Eric Peterson --- .../backend-common/src/cache/CacheManager.test.ts | 11 +++++++++++ packages/backend-common/src/cache/CacheManager.ts | 6 ++++++ packages/backend-common/src/cache/types.ts | 6 ++++++ 3 files changed, 23 insertions(+) diff --git a/packages/backend-common/src/cache/CacheManager.test.ts b/packages/backend-common/src/cache/CacheManager.test.ts index ece4687ee6..05f94d51ec 100644 --- a/packages/backend-common/src/cache/CacheManager.test.ts +++ b/packages/backend-common/src/cache/CacheManager.test.ts @@ -86,6 +86,17 @@ describe('CacheManager', () => { expect(client).toHaveBeenCalledTimes(1); }); + it('attaches error handler to client', () => { + const pluginId = 'error-test'; + const handler = jest.fn(); + manager.forPlugin(pluginId).getClient({ onError: handler }); + + const client = DefaultCacheClient as jest.Mock; + const mockCalls = client.mock.calls.splice(-1); + const realClient = mockCalls[0][0].client as Keyv; + expect(realClient.on).toHaveBeenCalledWith('error', handler); + }); + it('provides different plugins different cache clients', async () => { const plugin1Id = 'test1'; const plugin2Id = 'test2'; diff --git a/packages/backend-common/src/cache/CacheManager.ts b/packages/backend-common/src/cache/CacheManager.ts index 50788be56c..ba736d6121 100644 --- a/packages/backend-common/src/cache/CacheManager.ts +++ b/packages/backend-common/src/cache/CacheManager.ts @@ -73,6 +73,12 @@ export class CacheManager { return { getClient: (opts = {}): CacheClient => { const concreteClient = this.getClientWithTtl(pluginId, opts.defaultTtl); + + // Attach error handler if provided. + if (typeof opts?.onError === 'function') { + concreteClient.on('error', opts.onError); + } + return new DefaultCacheClient({ client: concreteClient, }); diff --git a/packages/backend-common/src/cache/types.ts b/packages/backend-common/src/cache/types.ts index 4dd3cf7d1b..cfd78eddda 100644 --- a/packages/backend-common/src/cache/types.ts +++ b/packages/backend-common/src/cache/types.ts @@ -23,6 +23,12 @@ type ClientOptions = { * can be configured per entry at set-time). */ defaultTtl?: number; + + /** + * An optional handler for connection errors emitted from the underlying data + * store. + */ + onError?: (err: Error) => void }; /** From 6f6ebd248fe9441f5693b4aa1d8a006f8448bbc3 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Thu, 13 May 2021 17:41:39 +0200 Subject: [PATCH 3/6] Prettier Signed-off-by: Eric Peterson --- packages/backend-common/src/cache/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/src/cache/types.ts b/packages/backend-common/src/cache/types.ts index cfd78eddda..87b1580dbb 100644 --- a/packages/backend-common/src/cache/types.ts +++ b/packages/backend-common/src/cache/types.ts @@ -28,7 +28,7 @@ type ClientOptions = { * An optional handler for connection errors emitted from the underlying data * store. */ - onError?: (err: Error) => void + onError?: (err: Error) => void; }; /** From c7dad92187b5c2a762d22149ca09bbae9f77c221 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Thu, 13 May 2021 17:51:51 +0200 Subject: [PATCH 4/6] Changeset. Signed-off-by: Eric Peterson --- .changeset/improbable-word-combination.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 .changeset/improbable-word-combination.md diff --git a/.changeset/improbable-word-combination.md b/.changeset/improbable-word-combination.md new file mode 100644 index 0000000000..7b2e78ba32 --- /dev/null +++ b/.changeset/improbable-word-combination.md @@ -0,0 +1,16 @@ +--- +'@backstage/backend-common': patch +--- + +Plugin developers may now provide handlers for connection errors emitted by cache stores. + +```typescript +// Providing an error handler +const cacheClient = somePluginCache.getClient({ + defaultTtl: 3600000, + onError: e => { + logger.error(`There was a cache connection problem: ${e.message}`); + execOtherErrorHandlingLogic(); + }, +}); +``` From f2d9f5ecb979c8bcbf840aabd48de9efb5cf04fc Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Wed, 19 May 2021 10:49:54 +0200 Subject: [PATCH 5/6] Move error handler to CacheManager from Client. Signed-off-by: Eric Peterson --- .changeset/improbable-word-combination.md | 11 ++-- .../src/cache/CacheManager.test.ts | 54 +++++++++++++++++-- .../backend-common/src/cache/CacheManager.ts | 43 ++++++++++++--- packages/backend-common/src/cache/types.ts | 12 ++++- 4 files changed, 103 insertions(+), 17 deletions(-) diff --git a/.changeset/improbable-word-combination.md b/.changeset/improbable-word-combination.md index 7b2e78ba32..1b1abe2cf1 100644 --- a/.changeset/improbable-word-combination.md +++ b/.changeset/improbable-word-combination.md @@ -2,15 +2,16 @@ '@backstage/backend-common': patch --- -Plugin developers may now provide handlers for connection errors emitted by cache stores. +All cache-related connection errors are now handled and logged by the cache manager. App Integrators may provide an optional error handler when instantiating the cache manager if custom error handling is needed. ```typescript // Providing an error handler -const cacheClient = somePluginCache.getClient({ - defaultTtl: 3600000, +const cacheManager = CacheManager.fromConfig(config, { onError: e => { - logger.error(`There was a cache connection problem: ${e.message}`); - execOtherErrorHandlingLogic(); + if (isSomehowUnrecoverable(e)) { + gracefullyShutThingsDown(); + process.exit(1); + } }, }); ``` diff --git a/packages/backend-common/src/cache/CacheManager.test.ts b/packages/backend-common/src/cache/CacheManager.test.ts index 05f94d51ec..97c1714c53 100644 --- a/packages/backend-common/src/cache/CacheManager.test.ts +++ b/packages/backend-common/src/cache/CacheManager.test.ts @@ -88,13 +88,12 @@ describe('CacheManager', () => { it('attaches error handler to client', () => { const pluginId = 'error-test'; - const handler = jest.fn(); - manager.forPlugin(pluginId).getClient({ onError: handler }); + manager.forPlugin(pluginId).getClient(); const client = DefaultCacheClient as jest.Mock; const mockCalls = client.mock.calls.splice(-1); const realClient = mockCalls[0][0].client as Keyv; - expect(realClient.on).toHaveBeenCalledWith('error', handler); + expect(realClient.on).toHaveBeenCalledWith('error', expect.any(Function)); }); it('provides different plugins different cache clients', async () => { @@ -174,4 +173,53 @@ describe('CacheManager', () => { expect(mockMemcacheCalls[0][0]).toEqual(expectedHost); }); }); + + describe('connection errors', () => { + it('uses provided logger', () => { + // Set up and inject mock logger. + const mockLogger = { child: jest.fn(), error: jest.fn() }; + mockLogger.child.mockImplementation(() => mockLogger as any); + const manager = CacheManager.fromConfig(defaultConfig(), { + logger: mockLogger as any, + }); + + // Set up a cache client using the configured manager. + manager.forPlugin('error-logger-test').getClient(); + + // Retrieve the error handler attached to the cache client. + const client = DefaultCacheClient as jest.Mock; + const mockCalls = client.mock.calls.splice(-1); + const realClient = mockCalls[0][0].client as Keyv; + const realOnError = realClient.on as jest.Mock; + const realHandler = realOnError.mock.calls.splice(-1)[0][1]; + + // Invoke the actual error handler. + const expectedError = new Error('some error'); + realHandler(expectedError); + expect(mockLogger.error).toHaveBeenCalledWith(expectedError); + }); + + it('calls provided handler', () => { + // Set up and inject mock logger. + const mockHandler = jest.fn(); + const manager = CacheManager.fromConfig(defaultConfig(), { + onError: mockHandler, + }); + + // Set up a cache client using the configured manager. + manager.forPlugin('error-handler-test').getClient(); + + // Retrieve the error handler attached to the cache client. + const client = DefaultCacheClient as jest.Mock; + const mockCalls = client.mock.calls.splice(-1); + const realClient = mockCalls[0][0].client as Keyv; + const realOnError = realClient.on as jest.Mock; + const realHandler = realOnError.mock.calls.splice(-1)[0][1]; + + // Invoke the actual error handler. + const expectedError = new Error('some error'); + realHandler(expectedError); + expect(mockHandler).toHaveBeenCalledWith(expectedError); + }); + }); }); diff --git a/packages/backend-common/src/cache/CacheManager.ts b/packages/backend-common/src/cache/CacheManager.ts index ba736d6121..e6699e628e 100644 --- a/packages/backend-common/src/cache/CacheManager.ts +++ b/packages/backend-common/src/cache/CacheManager.ts @@ -18,9 +18,15 @@ import { Config } from '@backstage/config'; import Keyv from 'keyv'; // @ts-expect-error import KeyvMemcache from 'keyv-memcache'; +import { Logger } from 'winston'; +import { getRootLogger } from '../logging'; import { DefaultCacheClient, CacheClient } from './CacheClient'; import { NoStore } from './NoStore'; -import { PluginCacheManager } from './types'; +import { + CacheManagerOptions, + OptionalOnError, + PluginCacheManager, +} from './types'; /** * Implements a Cache Manager which will automatically create new cache clients @@ -38,8 +44,10 @@ export class CacheManager { none: this.getNoneClient, }; + private readonly logger: Logger; private readonly store: keyof CacheManager['storeFactories']; private readonly connection: string; + private readonly errorHandler: OptionalOnError; /** * Creates a new CacheManager instance by reading from the `backend` config @@ -47,21 +55,34 @@ export class CacheManager { * * @param config The loaded application configuration. */ - static fromConfig(config: Config): CacheManager { + static fromConfig( + config: Config, + options: CacheManagerOptions = {}, + ): CacheManager { // If no `backend.cache` config is provided, instantiate the CacheManager // with a "NoStore" cache client. const store = config.getOptionalString('backend.cache.store') || 'none'; const connectionString = config.getOptionalString('backend.cache.connection') || ''; - return new CacheManager(store, connectionString); + const logger = (options.logger || getRootLogger()).child({ + type: 'cacheManager', + }); + return new CacheManager(store, connectionString, logger, options.onError); } - private constructor(store: string, connectionString: string) { + private constructor( + store: string, + connectionString: string, + logger: Logger, + errorHandler: OptionalOnError, + ) { if (!this.storeFactories.hasOwnProperty(store)) { throw new Error(`Unknown cache store: ${store}`); } + this.logger = logger; this.store = store as keyof CacheManager['storeFactories']; this.connection = connectionString; + this.errorHandler = errorHandler; } /** @@ -74,10 +95,16 @@ export class CacheManager { getClient: (opts = {}): CacheClient => { const concreteClient = this.getClientWithTtl(pluginId, opts.defaultTtl); - // Attach error handler if provided. - if (typeof opts?.onError === 'function') { - concreteClient.on('error', opts.onError); - } + // Always provide an error handler to avoid killing the process. + concreteClient.on('error', (err: Error) => { + // In all cases, just log the error. + this.logger.error(err); + + // Invoke any custom error handler if provided. + if (typeof this.errorHandler === 'function') { + this.errorHandler(err); + } + }); return new DefaultCacheClient({ client: concreteClient, diff --git a/packages/backend-common/src/cache/types.ts b/packages/backend-common/src/cache/types.ts index 87b1580dbb..30db53d420 100644 --- a/packages/backend-common/src/cache/types.ts +++ b/packages/backend-common/src/cache/types.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { Logger } from 'winston'; import { CacheClient } from './CacheClient'; type ClientOptions = { @@ -23,12 +24,21 @@ type ClientOptions = { * can be configured per entry at set-time). */ defaultTtl?: number; +}; + +export type OptionalOnError = ((err: Error) => void) | undefined; + +export type CacheManagerOptions = { + /** + * An optional logger for use by the PluginCacheManager. + */ + logger?: Logger; /** * An optional handler for connection errors emitted from the underlying data * store. */ - onError?: (err: Error) => void; + onError?: OptionalOnError; }; /** From 023c75404d37d5b48b958337aae0661b2f2e43cd Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Wed, 19 May 2021 16:57:18 +0200 Subject: [PATCH 6/6] Update Public API Signed-off-by: Eric Peterson --- packages/backend-common/api-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index 23e22fe34b..068220cad3 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -73,7 +73,7 @@ export interface CacheClient { // @public export class CacheManager { forPlugin(pluginId: string): PluginCacheManager; - static fromConfig(config: Config): CacheManager; + static fromConfig(config: Config, options?: CacheManagerOptions): CacheManager; } // @public (undocumented)