Skip to content

Commit

Permalink
chore: remove response time metrics fix (#6779)
Browse files Browse the repository at this point in the history 
## 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)
  • Loading branch information
@gastonfournier
gastonfournier committed Apr 10, 2024
1 parent 69d06c4 commit f3cd1be
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 191 deletions.
189 changes: 5 additions & 184 deletions src/lib/middleware/response-time-metrics.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand All @@ -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(),
Expand All @@ -235,7 +56,7 @@ describe('responseTimeMetrics new behavior', () => {
});
const middleware = responseTimeMetrics(
eventBus,
flagResolverWithResponseTimeMetricsFix,
flagResolver,
instanceStatsService,
);
const req = {
Expand Down Expand Up @@ -264,7 +85,7 @@ describe('responseTimeMetrics new behavior', () => {
});
const middleware = responseTimeMetrics(
eventBus,
flagResolverWithResponseTimeMetricsFix,
flagResolver,
instanceStatsService,
);
const req = {
Expand Down Expand Up @@ -298,7 +119,7 @@ describe('responseTimeMetrics new behavior', () => {
});
const middleware = responseTimeMetrics(
eventBus,
flagResolverWithResponseTimeMetricsFix,
flagResolver,
instanceStatsService,
);
const req = {
Expand Down Expand Up @@ -334,7 +155,7 @@ describe('responseTimeMetrics new behavior', () => {
});
const middleware = responseTimeMetrics(
eventBus,
flagResolverWithResponseTimeMetricsFix,
flagResolver,
instanceStatsService,
);
const req = {
Expand Down Expand Up @@ -378,7 +199,7 @@ describe('responseTimeMetrics new behavior', () => {
});
const middleware = responseTimeMetrics(
eventBus,
flagResolverWithResponseTimeMetricsFix,
flagResolver,
instanceStatsService,
);
const req = {
Expand Down
9 changes: 2 additions & 7 deletions src/lib/middleware/response-time-metrics.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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') &&
Expand Down

0 comments on commit f3cd1be

Please sign in to comment.