From 149bc8aab272fb951c012d3e67dcc12aedb46327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 5 May 2023 09:18:04 +0200 Subject: [PATCH] chore: remove optimal 304 flag (#3665) ## About the changes This PR removes the optimal304 flag after being tested in production. We're keeping the existing configuration that allows users to disable cache mainly because it's useful for testing. --- .../__snapshots__/create-config.test.ts.snap | 6 +- src/lib/create-config.test.ts | 2 +- src/lib/create-config.ts | 8 +- src/lib/routes/client-api/feature.test.ts | 22 ++- src/lib/routes/client-api/feature.ts | 153 ++++-------------- src/lib/types/experimental.ts | 8 - src/server-dev.ts | 2 - 7 files changed, 50 insertions(+), 151 deletions(-) diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index a35791f59a..82404106d5 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -20,7 +20,7 @@ exports[`should create default config 1`] = ` }, "clientFeatureCaching": { "enabled": true, - "maxAge": 600, + "maxAge": 3600000, }, "db": { "acquireConnectionTimeout": 30000, @@ -79,8 +79,6 @@ exports[`should create default config 1`] = ` "maintenanceMode": false, "messageBanner": false, "migrationLock": false, - "optimal304": false, - "optimal304Differ": false, "personalAccessTokensKillSwitch": false, "proPlanAutoCharge": false, "responseTimeWithAppNameKillSwitch": false, @@ -104,8 +102,6 @@ exports[`should create default config 1`] = ` "maintenanceMode": false, "messageBanner": false, "migrationLock": false, - "optimal304": false, - "optimal304Differ": false, "personalAccessTokensKillSwitch": false, "proPlanAutoCharge": false, "responseTimeWithAppNameKillSwitch": false, diff --git a/src/lib/create-config.test.ts b/src/lib/create-config.test.ts index 7fe34ef082..d7057df9b0 100644 --- a/src/lib/create-config.test.ts +++ b/src/lib/create-config.test.ts @@ -378,7 +378,7 @@ test('Supports multiple domains comma separated in environment variables', () => test('Should enable client feature caching with .6 seconds max age by default', () => { const config = createConfig({}); expect(config.clientFeatureCaching.enabled).toBe(true); - expect(config.clientFeatureCaching.maxAge).toBe(600); + expect(config.clientFeatureCaching.maxAge).toBe(3600000); }); test('Should use overrides from options for client feature caching', () => { diff --git a/src/lib/create-config.ts b/src/lib/create-config.ts index 01d688cdd5..0e12436da0 100644 --- a/src/lib/create-config.ts +++ b/src/lib/create-config.ts @@ -22,7 +22,11 @@ import { import { getDefaultLogProvider, LogLevel, validateLogProvider } from './logger'; import { defaultCustomAuthDenyAll } from './default-custom-auth-deny-all'; import { formatBaseUri } from './util/format-base-uri'; -import { minutesToMilliseconds, secondsToMilliseconds } from 'date-fns'; +import { + hoursToMilliseconds, + minutesToMilliseconds, + secondsToMilliseconds, +} from 'date-fns'; import EventEmitter from 'events'; import { ApiTokenType, @@ -72,7 +76,7 @@ function loadExperimental(options: IUnleashOptions): IExperimentalOptions { const defaultClientCachingOptions: IClientCachingOption = { enabled: true, - maxAge: 600, + maxAge: hoursToMilliseconds(1), }; function loadClientCachingOptions( diff --git a/src/lib/routes/client-api/feature.test.ts b/src/lib/routes/client-api/feature.test.ts index 57b34bb8ae..b4930b1c27 100644 --- a/src/lib/routes/client-api/feature.test.ts +++ b/src/lib/routes/client-api/feature.test.ts @@ -33,8 +33,8 @@ async function getSetup() { const callGetAll = async (controller: FeatureController) => { await controller.getAll( // @ts-expect-error - { query: {}, header: () => undefined }, - { json: () => {} }, + { query: {}, header: () => undefined, headers: {} }, + { json: () => {}, setHeader: () => undefined }, ); }; @@ -82,16 +82,19 @@ test('if caching is enabled should memoize', async () => { const openApiService = { respondWithValidation, validPath }; const featureToggleServiceV2 = { getClientFeatures }; const segmentService = { getActive }; + const eventService = { getMaxRevisionId: () => 1 }; const controller = new FeatureController( { clientSpecService, - // @ts-expect-error + // @ts-expect-error due to partial implementation openApiService, - // @ts-expect-error + // @ts-expect-error due to partial implementation featureToggleServiceV2, - // @ts-expect-error + // @ts-expect-error due to partial implementation segmentService, + // @ts-expect-error due to partial implementation + eventService, }, { getLogger, @@ -117,16 +120,19 @@ test('if caching is not enabled all calls goes to service', async () => { const featureToggleServiceV2 = { getClientFeatures }; const segmentService = { getActive }; const openApiService = { respondWithValidation, validPath }; + const eventService = { getMaxRevisionId: () => 1 }; const controller = new FeatureController( { clientSpecService, - // @ts-expect-error + // @ts-expect-error due to partial implementation openApiService, - // @ts-expect-error + // @ts-expect-error due to partial implementation featureToggleServiceV2, - // @ts-expect-error + // @ts-expect-error due to partial implementation segmentService, + // @ts-expect-error due to partial implementation + eventService, }, { getLogger, diff --git a/src/lib/routes/client-api/feature.ts b/src/lib/routes/client-api/feature.ts index 98a1177cfc..442aa070be 100644 --- a/src/lib/routes/client-api/feature.ts +++ b/src/lib/routes/client-api/feature.ts @@ -2,10 +2,8 @@ import memoizee from 'memoizee'; import { Response } from 'express'; // eslint-disable-next-line import/no-extraneous-dependencies import hashSum from 'hash-sum'; -// eslint-disable-next-line import/no-extraneous-dependencies -import { diff } from 'deep-object-diff'; import Controller from '../controller'; -import { IFlagResolver, IUnleashConfig, IUnleashServices } from '../../types'; +import { IUnleashConfig, IUnleashServices } from '../../types'; import FeatureToggleService from '../../services/feature-toggle-service'; import { Logger } from '../../logger'; import { querySchema } from '../../schema/feature-schema'; @@ -30,9 +28,6 @@ import { } from '../../openapi/spec/client-features-schema'; import { ISegmentService } from 'lib/segments/segment-service-interface'; import { EventService } from 'lib/services'; -import { hoursToMilliseconds } from 'date-fns'; -import { isEmpty } from '../../util/isEmpty'; -import EventEmitter from 'events'; const version = 2; @@ -50,8 +45,6 @@ interface IMeta { export default class FeatureController extends Controller { private readonly logger: Logger; - private eventBus: EventEmitter; - private featureToggleServiceV2: FeatureToggleService; private segmentService: ISegmentService; @@ -62,13 +55,10 @@ export default class FeatureController extends Controller { private eventService: EventService; - private readonly cache: boolean; - - private cachedFeatures: any; - - private cachedFeatures2: any; - - private flagResolver: IFlagResolver; + private featuresAndSegments: ( + query: IFeatureToggleQuery, + etag: string, + ) => Promise<[FeatureConfigurationClient[], ISegment[]]>; constructor( { @@ -88,15 +78,13 @@ export default class FeatureController extends Controller { config: IUnleashConfig, ) { super(config); - const { clientFeatureCaching, flagResolver, eventBus } = config; + const { clientFeatureCaching } = config; this.featureToggleServiceV2 = featureToggleServiceV2; this.segmentService = segmentService; this.clientSpecService = clientSpecService; this.openApiService = openApiService; - this.flagResolver = flagResolver; this.eventService = eventService; this.logger = config.getLogger('client-api/feature.js'); - this.eventBus = eventBus; this.route({ method: 'get', @@ -130,39 +118,28 @@ export default class FeatureController extends Controller { ], }); - if (clientFeatureCaching?.enabled) { - this.cache = true; - this.cachedFeatures = memoizee( - (query) => this.resolveFeaturesAndSegments(query), + if (clientFeatureCaching.enabled) { + this.featuresAndSegments = memoizee( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + (query: IFeatureToggleQuery, etag: string) => + this.resolveFeaturesAndSegments(query), { promise: true, maxAge: clientFeatureCaching.maxAge, normalizer(args) { // args is arguments object as accessible in memoized function - return JSON.stringify(args[0]); + return args[1]; }, }, ); + } else { + this.featuresAndSegments = this.resolveFeaturesAndSegments; } - this.cachedFeatures2 = memoizee( - // eslint-disable-next-line @typescript-eslint/no-unused-vars - (query: IFeatureToggleQuery, etag: string) => - this.resolveFeaturesAndSegments(query), - { - promise: true, - maxAge: hoursToMilliseconds(1), - normalizer(args) { - // args is arguments object as accessible in memoized function - return args[1]; - }, - }, - ); } private async resolveFeaturesAndSegments( query?: IFeatureToggleQuery, ): Promise<[FeatureConfigurationClient[], ISegment[]]> { - this.logger.debug('bypass cache'); return Promise.all([ this.featureToggleServiceV2.getClientFeatures(query), this.segmentService.getActive(), @@ -239,93 +216,6 @@ export default class FeatureController extends Controller { async getAll( req: IAuthRequest, res: Response, - ): Promise { - if (this.flagResolver.isEnabled('optimal304')) { - return this.optimal304(req, res); - } - - const query = await this.resolveQuery(req); - - const [features, segments] = this.cache - ? await this.cachedFeatures(query) - : await this.resolveFeaturesAndSegments(query); - - if (this.flagResolver.isEnabled('optimal304Differ')) { - process.nextTick(async () => - this.doOptimal304Diffing(features, query), - ); - } - - if (this.clientSpecService.requestSupportsSpec(req, 'segments')) { - this.openApiService.respondWithValidation( - 200, - res, - clientFeaturesSchema.$id, - { version, features, query: { ...query }, segments }, - ); - } else { - this.openApiService.respondWithValidation( - 200, - res, - clientFeaturesSchema.$id, - { version, features, query }, - ); - } - } - - /** - * This helper method is used to validate that the new way of calculating - * cache-key based on query hash and revision id, with an internal memoization - * of 1hr still ends up producing the same result. - * - * It's worth to note that it is expected that a diff will occur immediately after - * a toggle changes due to the nature of two individual caches and how fast they - * detect the change. The diffs should however go away as soon as they both have - * the latest feature toggle configuration, which will happen within 600ms on the - * default configurations. - * - * This method is experimental and will only be used to validate our internal state - * to make sure our new way of caching is correct and stable. - * - * @deprecated - */ - async doOptimal304Diffing( - features: FeatureConfigurationClient[], - query: IFeatureToggleQuery, - ): Promise { - try { - const { etag } = await this.calculateMeta(query); - const [featuresNew] = await this.cachedFeatures2(query, etag); - const theDiffedObject = diff(features, featuresNew); - - if (isEmpty(theDiffedObject)) { - this.logger.warn('The diff is: '); - this.eventBus.emit('optimal304Differ', { status: 'equal' }); - } else { - this.logger.warn( - `The diff is: ${JSON.stringify(theDiffedObject)}`, - ); - this.eventBus.emit('optimal304Differ', { status: 'diff' }); - } - } catch (e) { - this.logger.error('The diff checker crashed', e); - this.eventBus.emit('optimal304Differ', { status: 'crash' }); - } - } - - async calculateMeta(query: IFeatureToggleQuery): Promise { - // TODO: We will need to standardize this to be able to implement this a cross languages (Edge in Rust?). - const revisionId = await this.eventService.getMaxRevisionId(); - - // TODO: We will need to standardize this to be able to implement this a cross languages (Edge in Rust?). - const queryHash = hashSum(query); - const etag = `"${queryHash}:${revisionId}"`; - return { revisionId, etag, queryHash }; - } - - async optimal304( - req: IAuthRequest, - res: Response, ): Promise { const query = await this.resolveQuery(req); @@ -345,7 +235,10 @@ export default class FeatureController extends Controller { ); } - const [features, segments] = await this.cachedFeatures2(query, etag); + const [features, segments] = await this.featuresAndSegments( + query, + etag, + ); if (this.clientSpecService.requestSupportsSpec(req, 'segments')) { this.openApiService.respondWithValidation( @@ -370,6 +263,16 @@ export default class FeatureController extends Controller { } } + async calculateMeta(query: IFeatureToggleQuery): Promise { + // TODO: We will need to standardize this to be able to implement this a cross languages (Edge in Rust?). + const revisionId = await this.eventService.getMaxRevisionId(); + + // TODO: We will need to standardize this to be able to implement this a cross languages (Edge in Rust?). + const queryHash = hashSum(query); + const etag = `"${queryHash}:${revisionId}"`; + return { revisionId, etag, queryHash }; + } + async getFeatureToggle( req: IAuthRequest<{ featureName: string }, ClientFeaturesQuerySchema>, res: Response, diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 3ae84ab5f8..60f5a467b5 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -50,14 +50,6 @@ const flags = { false, ), cleanClientApi: parseEnvVarBoolean(process.env.CLEAN_CLIENT_API, false), - optimal304: parseEnvVarBoolean( - process.env.UNLEASH_EXPERIMENTAL_OPTIMAL_304, - false, - ), - optimal304Differ: parseEnvVarBoolean( - process.env.UNLEASH_EXPERIMENTAL_OPTIMAL_304_DIFFER, - false, - ), groupRootRoles: parseEnvVarBoolean( process.env.UNLEASH_EXPERIMENTAL_ROOT_ROLE_GROUPS, false, diff --git a/src/server-dev.ts b/src/server-dev.ts index 325d793efa..a0f2123ef7 100644 --- a/src/server-dev.ts +++ b/src/server-dev.ts @@ -38,8 +38,6 @@ process.nextTick(async () => { embedProxyFrontend: true, anonymiseEventLog: false, responseTimeWithAppNameKillSwitch: false, - optimal304: true, - optimal304Differ: false, }, }, authentication: {