From f3cd1be9df2daae7b8a116b203266e44e22a37e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 10 Apr 2024 12:34:05 +0200 Subject: [PATCH] chore: remove response time metrics fix (#6779) ## About the changes The feature `responseTimeMetricsFix` has been enabled for a while. Since it's released in 5.11 this prepares the removal for the next major version. ![image](https://github.com/Unleash/unleash/assets/455064/cc49ba3f-f775-45b2-998c-ef7a02c537b4) --- .../middleware/response-time-metrics.test.ts | 189 +----------------- src/lib/middleware/response-time-metrics.ts | 9 +- 2 files changed, 7 insertions(+), 191 deletions(-) diff --git a/src/lib/middleware/response-time-metrics.test.ts b/src/lib/middleware/response-time-metrics.test.ts index d5fe37c5566..6e8dd541d95 100644 --- a/src/lib/middleware/response-time-metrics.test.ts +++ b/src/lib/middleware/response-time-metrics.test.ts @@ -33,11 +33,6 @@ const flagResolver = { getVariant: jest.fn(), }; -const flagResolverWithResponseTimeMetricsFix = { - ...flagResolver, - isEnabled: (name: string) => name === 'responseTimeMetricsFix', -}; - // Make sure it's always cleaned up let res: any; beforeEach(() => { @@ -47,180 +42,6 @@ beforeEach(() => { }; }); -describe('responseTimeMetrics old behavior', () => { - const instanceStatsService = { - getAppCountSnapshot: jest.fn(), - }; - const eventBus = new EventEmitter(); - afterEach(() => { - eventBus.removeAllListeners(); - }); - - test('uses baseUrl and route path to report metrics', async () => { - let timeInfo: any; - // register a listener - eventBus.on(REQUEST_TIME, (data) => { - timeInfo = data; - }); - const middleware = responseTimeMetrics( - eventBus, - flagResolver, - instanceStatsService, - ); - const req = { - baseUrl: '/api/admin', - route: { - path: '/features', - }, - method: 'GET', - path: 'should-not-be-used', - }; - - // @ts-expect-error req and res doesn't have all properties - middleware(req, res); - - await isDefined(timeInfo); - expect(timeInfo).toMatchObject({ - path: '/api/admin/features', - method: 'GET', - statusCode: 200, - time: 100, - appName: undefined, - }); - }); - - test('uses baseUrl and route path to report metrics even with the new flag enabled', async () => { - let timeInfo: any; - // register a listener - eventBus.on(REQUEST_TIME, (data) => { - timeInfo = data; - }); - const middleware = responseTimeMetrics( - eventBus, - flagResolverWithResponseTimeMetricsFix, - instanceStatsService, - ); - const req = { - baseUrl: '/api/admin', - route: { - path: '/features', - }, - method: 'GET', - path: 'should-not-be-used', - }; - - // @ts-expect-error req and res doesn't have all properties - middleware(req, res); - - await isDefined(timeInfo); - expect(timeInfo).toMatchObject({ - path: '/api/admin/features', - }); - }); - - test('uses baseUrl and route path to report metrics even with res.locals.route but flag disabled', async () => { - let timeInfo: any; - // register a listener - eventBus.on(REQUEST_TIME, (data) => { - timeInfo = data; - }); - const middleware = responseTimeMetrics( - eventBus, - flagResolver, - instanceStatsService, - ); - const req = { - baseUrl: '/api/admin', - route: { - path: '/features', - }, - method: 'GET', - path: 'should-not-be-used', - }; - - // @ts-expect-error req and res doesn't have all properties - middleware(req, { - statusCode: 200, - locals: { route: '/should-not-be-used-eiter' }, - }); - - await isDefined(timeInfo); - expect(timeInfo).toMatchObject({ - path: '/api/admin/features', - }); - }); - - test('reports (hidden) when route is not present', async () => { - let timeInfo: any; - // register a listener - eventBus.on(REQUEST_TIME, (data) => { - timeInfo = data; - }); - const middleware = responseTimeMetrics( - eventBus, - flagResolver, - instanceStatsService, - ); - const req = { - baseUrl: '/api/admin', - method: 'GET', - path: 'should-not-be-used', - }; - - // @ts-expect-error req and res doesn't have all properties - middleware(req, res); - - await isDefined(timeInfo); - expect(timeInfo).toMatchObject({ - path: '(hidden)', - }); - }); - - test.each([ - ['/api/admin/features', '(hidden)'], - ['/api/admin/features/my-feature', '(hidden)'], - ['/api/frontend/client/metrics', '(hidden)'], - ['/api/client/metrics', '(hidden)'], - ['/edge/validate', '(hidden)'], - ['/whatever', '(hidden)'], - ['/healthz', '(hidden)'], - ['/internal-backstage/prometheus', '(hidden)'], - ])( - 'when path is %s and route is undefined, reports %s', - async (path: string, expected: string) => { - let timeInfo: any; - // register a listener - eventBus.on(REQUEST_TIME, (data) => { - timeInfo = data; - }); - const middleware = responseTimeMetrics( - eventBus, - flagResolver, - instanceStatsService, - ); - const req = { - baseUrl: '/api/admin', - method: 'GET', - path: 'should-not-be-used', - }; - const reqWithoutRoute = { - method: 'GET', - path, - }; - - // @ts-expect-error req and res doesn't have all properties - storeRequestedRoute(req, res, () => {}); - // @ts-expect-error req and res doesn't have all properties - middleware(reqWithoutRoute, res); - - await isDefined(timeInfo); - expect(timeInfo).toMatchObject({ - path: expected, - }); - }, - ); -}); - describe('responseTimeMetrics new behavior', () => { const instanceStatsService = { getAppCountSnapshot: jest.fn(), @@ -235,7 +56,7 @@ describe('responseTimeMetrics new behavior', () => { }); const middleware = responseTimeMetrics( eventBus, - flagResolverWithResponseTimeMetricsFix, + flagResolver, instanceStatsService, ); const req = { @@ -264,7 +85,7 @@ describe('responseTimeMetrics new behavior', () => { }); const middleware = responseTimeMetrics( eventBus, - flagResolverWithResponseTimeMetricsFix, + flagResolver, instanceStatsService, ); const req = { @@ -298,7 +119,7 @@ describe('responseTimeMetrics new behavior', () => { }); const middleware = responseTimeMetrics( eventBus, - flagResolverWithResponseTimeMetricsFix, + flagResolver, instanceStatsService, ); const req = { @@ -334,7 +155,7 @@ describe('responseTimeMetrics new behavior', () => { }); const middleware = responseTimeMetrics( eventBus, - flagResolverWithResponseTimeMetricsFix, + flagResolver, instanceStatsService, ); const req = { @@ -378,7 +199,7 @@ describe('responseTimeMetrics new behavior', () => { }); const middleware = responseTimeMetrics( eventBus, - flagResolverWithResponseTimeMetricsFix, + flagResolver, instanceStatsService, ); const req = { diff --git a/src/lib/middleware/response-time-metrics.ts b/src/lib/middleware/response-time-metrics.ts index 87792c1b291..648517c3a67 100644 --- a/src/lib/middleware/response-time-metrics.ts +++ b/src/lib/middleware/response-time-metrics.ts @@ -46,19 +46,14 @@ export function responseTimeMetrics( ): RequestHandler { return _responseTime((req, res, time) => { const { statusCode } = res; - const responseTimeMetricsFix = flagResolver.isEnabled( - 'responseTimeMetricsFix', - ); let pathname: string | undefined = undefined; - if (responseTimeMetricsFix && res.locals.route) { + if (res.locals.route) { pathname = res.locals.route; } else if (req.route) { pathname = req.baseUrl + req.route.path; } // when pathname is undefined use a fallback - pathname = - pathname ?? - (responseTimeMetricsFix ? collapse(req.path) : '(hidden)'); + pathname = pathname ?? collapse(req.path); let appName: string | undefined; if ( !flagResolver.isEnabled('responseTimeWithAppNameKillSwitch') &&