mirror of
https://github.com/Unleash/unleash.git
synced 2025-02-09 00:18:00 +01:00
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)
This commit is contained in:
parent
69d06c421f
commit
f3cd1be9df
@ -33,11 +33,6 @@ const flagResolver = {
|
|||||||
getVariant: jest.fn(),
|
getVariant: jest.fn(),
|
||||||
};
|
};
|
||||||
|
|
||||||
const flagResolverWithResponseTimeMetricsFix = {
|
|
||||||
...flagResolver,
|
|
||||||
isEnabled: (name: string) => name === 'responseTimeMetricsFix',
|
|
||||||
};
|
|
||||||
|
|
||||||
// Make sure it's always cleaned up
|
// Make sure it's always cleaned up
|
||||||
let res: any;
|
let res: any;
|
||||||
beforeEach(() => {
|
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', () => {
|
describe('responseTimeMetrics new behavior', () => {
|
||||||
const instanceStatsService = {
|
const instanceStatsService = {
|
||||||
getAppCountSnapshot: jest.fn(),
|
getAppCountSnapshot: jest.fn(),
|
||||||
@ -235,7 +56,7 @@ describe('responseTimeMetrics new behavior', () => {
|
|||||||
});
|
});
|
||||||
const middleware = responseTimeMetrics(
|
const middleware = responseTimeMetrics(
|
||||||
eventBus,
|
eventBus,
|
||||||
flagResolverWithResponseTimeMetricsFix,
|
flagResolver,
|
||||||
instanceStatsService,
|
instanceStatsService,
|
||||||
);
|
);
|
||||||
const req = {
|
const req = {
|
||||||
@ -264,7 +85,7 @@ describe('responseTimeMetrics new behavior', () => {
|
|||||||
});
|
});
|
||||||
const middleware = responseTimeMetrics(
|
const middleware = responseTimeMetrics(
|
||||||
eventBus,
|
eventBus,
|
||||||
flagResolverWithResponseTimeMetricsFix,
|
flagResolver,
|
||||||
instanceStatsService,
|
instanceStatsService,
|
||||||
);
|
);
|
||||||
const req = {
|
const req = {
|
||||||
@ -298,7 +119,7 @@ describe('responseTimeMetrics new behavior', () => {
|
|||||||
});
|
});
|
||||||
const middleware = responseTimeMetrics(
|
const middleware = responseTimeMetrics(
|
||||||
eventBus,
|
eventBus,
|
||||||
flagResolverWithResponseTimeMetricsFix,
|
flagResolver,
|
||||||
instanceStatsService,
|
instanceStatsService,
|
||||||
);
|
);
|
||||||
const req = {
|
const req = {
|
||||||
@ -334,7 +155,7 @@ describe('responseTimeMetrics new behavior', () => {
|
|||||||
});
|
});
|
||||||
const middleware = responseTimeMetrics(
|
const middleware = responseTimeMetrics(
|
||||||
eventBus,
|
eventBus,
|
||||||
flagResolverWithResponseTimeMetricsFix,
|
flagResolver,
|
||||||
instanceStatsService,
|
instanceStatsService,
|
||||||
);
|
);
|
||||||
const req = {
|
const req = {
|
||||||
@ -378,7 +199,7 @@ describe('responseTimeMetrics new behavior', () => {
|
|||||||
});
|
});
|
||||||
const middleware = responseTimeMetrics(
|
const middleware = responseTimeMetrics(
|
||||||
eventBus,
|
eventBus,
|
||||||
flagResolverWithResponseTimeMetricsFix,
|
flagResolver,
|
||||||
instanceStatsService,
|
instanceStatsService,
|
||||||
);
|
);
|
||||||
const req = {
|
const req = {
|
||||||
|
@ -46,19 +46,14 @@ export function responseTimeMetrics(
|
|||||||
): RequestHandler {
|
): RequestHandler {
|
||||||
return _responseTime((req, res, time) => {
|
return _responseTime((req, res, time) => {
|
||||||
const { statusCode } = res;
|
const { statusCode } = res;
|
||||||
const responseTimeMetricsFix = flagResolver.isEnabled(
|
|
||||||
'responseTimeMetricsFix',
|
|
||||||
);
|
|
||||||
let pathname: string | undefined = undefined;
|
let pathname: string | undefined = undefined;
|
||||||
if (responseTimeMetricsFix && res.locals.route) {
|
if (res.locals.route) {
|
||||||
pathname = res.locals.route;
|
pathname = res.locals.route;
|
||||||
} else if (req.route) {
|
} else if (req.route) {
|
||||||
pathname = req.baseUrl + req.route.path;
|
pathname = req.baseUrl + req.route.path;
|
||||||
}
|
}
|
||||||
// when pathname is undefined use a fallback
|
// when pathname is undefined use a fallback
|
||||||
pathname =
|
pathname = pathname ?? collapse(req.path);
|
||||||
pathname ??
|
|
||||||
(responseTimeMetricsFix ? collapse(req.path) : '(hidden)');
|
|
||||||
let appName: string | undefined;
|
let appName: string | undefined;
|
||||||
if (
|
if (
|
||||||
!flagResolver.isEnabled('responseTimeWithAppNameKillSwitch') &&
|
!flagResolver.isEnabled('responseTimeWithAppNameKillSwitch') &&
|
||||||
|
Loading…
Reference in New Issue
Block a user