Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter empty metrics before we collect last seen toggles. #2172

Merged
merged 6 commits into from
Oct 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/lib/routes/client-api/metrics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import getApp from '../../app';
import { createTestConfig } from '../../../test/config/test-config';
import { clientMetricsSchema } from '../../services/client-metrics/schema';
import { createServices } from '../../services';
import { IUnleashOptions, IUnleashStores } from '../../types';
import { IUnleashOptions, IUnleashServices, IUnleashStores } from '../../types';

async function getSetup(opts?: IUnleashOptions) {
const stores = createStores();
Expand All @@ -16,6 +16,7 @@ async function getSetup(opts?: IUnleashOptions) {
return {
request: supertest(app),
stores,
services,
destroy: () => {
services.versionService.destroy();
services.clientInstanceService.destroy();
Expand All @@ -26,13 +27,15 @@ async function getSetup(opts?: IUnleashOptions) {

let request;
let stores: IUnleashStores;
let services: IUnleashServices;
let destroy;

beforeEach(async () => {
const setup = await getSetup();
request = setup.request;
stores = setup.stores;
destroy = setup.destroy;
services = setup.services;
});

afterEach(() => {
Expand Down Expand Up @@ -202,6 +205,7 @@ test('should set lastSeen on toggle', async () => {
})
.expect(202);

await services.lastSeenService.store();
const toggle = await stores.featureToggleStore.get('toggleLastSeen');

expect(toggle.lastSeenAt).toBeTruthy();
Expand Down
58 changes: 58 additions & 0 deletions src/lib/services/client-metrics/last-seen-service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { secondsToMilliseconds } from 'date-fns';
import { Logger } from '../../logger';
import { IUnleashConfig } from '../../server-impl';
import { IUnleashStores } from '../../types';
import { IClientMetricsEnv } from '../../types/stores/client-metrics-store-v2';
import { IFeatureToggleStore } from '../../types/stores/feature-toggle-store';

export class LastSeenService {
private timers: NodeJS.Timeout[] = [];

private lastSeenToggles: Set<string> = new Set();

private logger: Logger;

private featureToggleStore: IFeatureToggleStore;

constructor(
{ featureToggleStore }: Pick<IUnleashStores, 'featureToggleStore'>,
config: IUnleashConfig,
lastSeenInterval = secondsToMilliseconds(30),
) {
this.featureToggleStore = featureToggleStore;
this.logger = config.getLogger(
'/services/client-metrics/last-seen-service.ts',
);

this.timers.push(
setInterval(() => this.store(), lastSeenInterval).unref(),
);
}

async store(): Promise<number> {
const count = this.lastSeenToggles.size;
if (count > 0) {
const lastSeenToggles = [...this.lastSeenToggles];
this.lastSeenToggles = new Set();
this.logger.debug(
`Updating last seen for ${lastSeenToggles.length} toggles`,
);
await this.featureToggleStore.setLastSeen(lastSeenToggles);
}
return count;
}

updateLastSeen(clientMetrics: IClientMetricsEnv[]): void {
clientMetrics
.filter(
(clientMetric) => clientMetric.yes > 0 || clientMetric.no > 0,
)
.forEach((clientMetric) =>
this.lastSeenToggles.add(clientMetric.featureName),
);
}

destroy(): void {
this.timers.forEach(clearInterval);
}
}
39 changes: 25 additions & 14 deletions src/lib/services/client-metrics/metrics-service-v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import ApiUser from '../../types/api-user';
import { ALL } from '../../types/models/api-token';
import User from '../../types/user';
import { collapseHourlyMetrics } from '../../util/collapseHourlyMetrics';
import { LastSeenService } from './last-seen-service';

export default class ClientMetricsServiceV2 {
private config: IUnleashConfig;
Expand All @@ -27,6 +28,8 @@ export default class ClientMetricsServiceV2 {

private featureToggleStore: IFeatureToggleStore;

private lastSeenService: LastSeenService;

private logger: Logger;

constructor(
Expand All @@ -35,10 +38,12 @@ export default class ClientMetricsServiceV2 {
clientMetricsStoreV2,
}: Pick<IUnleashStores, 'featureToggleStore' | 'clientMetricsStoreV2'>,
config: IUnleashConfig,
lastSeenService: LastSeenService,
bulkInterval = secondsToMilliseconds(5),
) {
this.featureToggleStore = featureToggleStore;
this.clientMetricsStoreV2 = clientMetricsStoreV2;
this.lastSeenService = lastSeenService;
this.config = config;
this.logger = config.getLogger(
'/services/client-metrics/client-metrics-service-v2.ts',
Expand All @@ -62,30 +67,35 @@ export default class ClientMetricsServiceV2 {
clientIp: string,
): Promise<void> {
const value = await clientMetricsSchema.validateAsync(data);
const toggleNames = Object.keys(value.bucket.toggles);
if (toggleNames.length > 0) {
await this.featureToggleStore.setLastSeen(toggleNames);
}
const toggleNames = Object.keys(value.bucket.toggles).filter(
(name) =>
!(
value.bucket.toggles[name].yes === 0 &&
value.bucket.toggles[name].no === 0
),
);

this.logger.debug(`got metrics from ${clientIp}`);

const clientMetrics: IClientMetricsEnv[] = toggleNames
.map((name) => ({
featureName: name,
appName: value.appName,
environment: value.environment,
timestamp: value.bucket.start, //we might need to approximate between start/stop...
yes: value.bucket.toggles[name].yes,
no: value.bucket.toggles[name].no,
}))
.filter((item) => !(item.yes === 0 && item.no === 0));
const clientMetrics: IClientMetricsEnv[] = toggleNames.map((name) => ({
featureName: name,
appName: value.appName,
environment: value.environment,
timestamp: value.bucket.start, //we might need to approximate between start/stop...
yes: value.bucket.toggles[name].yes,
no: value.bucket.toggles[name].no,
}));

if (this.config.flagResolver.isEnabled('batchMetrics')) {
this.unsavedMetrics = collapseHourlyMetrics([
...this.unsavedMetrics,
...clientMetrics,
]);
this.lastSeenService.updateLastSeen(clientMetrics);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be the default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have it behind a flag, and the batch flag seems natural.

} else {
if (toggleNames.length > 0) {
await this.featureToggleStore.setLastSeen(toggleNames);
}
await this.clientMetricsStoreV2.batchInsertMetrics(clientMetrics);
}

Expand Down Expand Up @@ -161,5 +171,6 @@ export default class ClientMetricsServiceV2 {

destroy(): void {
this.timers.forEach(clearInterval);
this.lastSeenService.destroy();
}
}
9 changes: 8 additions & 1 deletion src/lib/services/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { ProxyService } from './proxy-service';
import EdgeService from './edge-service';
import PatService from './pat-service';
import { PublicSignupTokenService } from './public-signup-token-service';
import { LastSeenService } from './client-metrics/last-seen-service';
export const createServices = (
stores: IUnleashStores,
config: IUnleashConfig,
Expand All @@ -43,7 +44,12 @@ export const createServices = (
const accessService = new AccessService(stores, config, groupService);
const apiTokenService = new ApiTokenService(stores, config);
const clientInstanceService = new ClientInstanceService(stores, config);
const clientMetricsServiceV2 = new ClientMetricsServiceV2(stores, config);
const lastSeenService = new LastSeenService(stores, config);
const clientMetricsServiceV2 = new ClientMetricsServiceV2(
stores,
config,
lastSeenService,
);
const contextService = new ContextService(stores, config);
const emailService = new EmailService(config.email, config.getLogger);
const eventService = new EventService(stores, config);
Expand Down Expand Up @@ -147,6 +153,7 @@ export const createServices = (
edgeService,
patService,
publicSignupTokenService,
lastSeenService,
};
};

Expand Down
2 changes: 2 additions & 0 deletions src/lib/types/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { ProxyService } from '../services/proxy-service';
import EdgeService from '../services/edge-service';
import PatService from '../services/pat-service';
import { PublicSignupTokenService } from '../services/public-signup-token-service';
import { LastSeenService } from '../services/client-metrics/last-seen-service';

export interface IUnleashServices {
accessService: AccessService;
Expand Down Expand Up @@ -71,4 +72,5 @@ export interface IUnleashServices {
openApiService: OpenApiService;
clientSpecService: ClientSpecService;
patService: PatService;
lastSeenService: LastSeenService;
}
50 changes: 50 additions & 0 deletions src/test/e2e/api/client/metricsV2.e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,53 @@ test('should pick up environment from token', async () => {
expect(metrics[0].environment).toBe('test');
expect(metrics[0].appName).toBe('some-fancy-app');
});

test('should set lastSeen for toggles with metrics', async () => {
const start = Date.now();
await app.services.featureToggleServiceV2.createFeatureToggle(
'default',
{ name: 't1' },
'tester',
);
await app.services.featureToggleServiceV2.createFeatureToggle(
'default',
{ name: 't2' },
'tester',
);
const token = await app.services.apiTokenService.createApiToken({
type: ApiTokenType.CLIENT,
project: 'default',
environment: 'default',
username: 'tester',
});

await app.request
.post('/api/client/metrics')
.set('Authorization', token.secret)
.send({
appName: 'some-fancy-app',
instanceId: '1',
bucket: {
start: Date.now(),
stop: Date.now(),
toggles: {
t1: {
yes: 100,
no: 50,
},
t2: {
yes: 0,
no: 0,
},
},
},
})
.expect(202);

await app.services.clientMetricsServiceV2.bulkAdd();
await app.services.lastSeenService.store();
const t1 = await db.stores.featureToggleStore.get('t1');
const t2 = await db.stores.featureToggleStore.get('t2');
expect(t1.lastSeenAt.getTime()).toBeGreaterThanOrEqual(start);
expect(t2.lastSeenAt).toBeDefined();
});
Loading