Skip to content

Commit

Permalink
fix: last seen metrics exceeding table limits (#7923)
Browse files Browse the repository at this point in the history
## About the changes
When storing last seen metrics we no longer validate at insert time that
the feature exists. Instead, there's a job cleaning up on a regular
interval.

Metrics for features with more than 255 characters, makes the whole
batch to fail, resulting in metrics being lost.

This PR helps mitigate the issue while also logs long name feature names
  • Loading branch information
gastonfournier authored Aug 19, 2024
1 parent c560bb6 commit 106e1d1
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ function initClientMetrics(flagEnabled = true) {
const lastSeenService = new LastSeenService(
{
lastSeenStore: stores.lastSeenStore,
featureToggleStore: stores.featureToggleStore,
},
config,
);
Expand Down
14 changes: 2 additions & 12 deletions src/lib/features/metrics/last-seen/createLastSeenService.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import FakeFeatureToggleStore from '../../feature-toggle/fakes/fake-feature-toggle-store';
import FeatureToggleStore from '../../feature-toggle/feature-toggle-store';
import type { Db, IUnleashConfig } from '../../../server-impl';
import { FakeLastSeenStore } from './fake-last-seen-store';
import { LastSeenService } from './last-seen-service';
Expand All @@ -15,21 +13,13 @@ export const createLastSeenService = (
config.getLogger,
);

const featureToggleStore = new FeatureToggleStore(
db,
config.eventBus,
config.getLogger,
config.flagResolver,
);

return new LastSeenService({ lastSeenStore, featureToggleStore }, config);
return new LastSeenService({ lastSeenStore }, config);
};

export const createFakeLastSeenService = (
config: IUnleashConfig,
): LastSeenService => {
const lastSeenStore = new FakeLastSeenStore();
const featureToggleStore = new FakeFeatureToggleStore();

return new LastSeenService({ lastSeenStore, featureToggleStore }, config);
return new LastSeenService({ lastSeenStore }, config);
};
37 changes: 17 additions & 20 deletions src/lib/features/metrics/last-seen/last-seen-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ import type { Logger } from '../../../logger';
import type { IUnleashConfig } from '../../../server-impl';
import type { IClientMetricsEnv } from '../client-metrics/client-metrics-store-v2-type';
import type { ILastSeenStore } from './types/last-seen-store-type';
import type {
IFeatureToggleStore,
IFlagResolver,
IUnleashStores,
} from '../../../types';
import type { IUnleashStores } from '../../../types';

export type LastSeenInput = {
featureName: string;
Expand All @@ -20,36 +16,37 @@ export class LastSeenService {

private lastSeenStore: ILastSeenStore;

private featureToggleStore: IFeatureToggleStore;

private config: IUnleashConfig;

private flagResolver: IFlagResolver;

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

async store(): Promise<number> {
const count = this.lastSeenToggles.size;
if (count > 0) {
const lastSeenToggles = Array.from(this.lastSeenToggles.values());
this.lastSeenToggles = new Map<String, LastSeenInput>();
const lastSeenToggles = Array.from(
this.lastSeenToggles.values(),
).filter((lastSeen) => lastSeen.featureName.length <= 255);
if (lastSeenToggles.length < this.lastSeenToggles.size) {
this.logger.warn(
`Toggles with long names ${JSON.stringify(
Array.from(this.lastSeenToggles.values())
.filter(
(lastSeen) => lastSeen.featureName.length > 255,
)
.map((lastSeen) => lastSeen.featureName),
)}`,
);
}
this.logger.debug(
`Updating last seen for ${lastSeenToggles.length} toggles`,
);
this.lastSeenToggles = new Map<String, LastSeenInput>();

await this.lastSeenStore.setLastSeen(lastSeenToggles);
}
Expand Down
1 change: 0 additions & 1 deletion src/lib/features/metrics/last-seen/last-seen-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ export default class LastSeenStore implements ILastSeenStore {
async setLastSeen(data: LastSeenInput[]): Promise<void> {
try {
const inserts = prepareLastSeenInput(data);

const batchSize = 500;

for (let i = 0; i < inserts.length; i += batchSize) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,34 @@ test('should clean unknown feature flag environments from last seen store', asyn

expect(stored.rows.length).toBe(2);
});

test('should not fail with feature names longer than 255 chars', async () => {
const { lastSeenService } = app.services;

const longFeatureNames = [
{ name: 'a'.repeat(254), environment: 'default' },
{ name: 'b'.repeat(255), environment: 'default' },
{ name: 'c'.repeat(256), environment: 'default' }, // this one should be filtered out
];

const inserts = [...longFeatureNames].map((feature) => {
return {
featureName: feature.name,
environment: feature.environment,
yes: 1,
no: 0,
appName: 'test',
timestamp: new Date(),
};
});

lastSeenService.updateLastSeen(inserts);
await lastSeenService.store();

// We have no method to get these from the last seen service or any other service or store
const stored = await db.rawDatabase.raw(
'SELECT * FROM last_seen_at_metrics;',
);

expect(stored.rows.length).toBe(2);
});
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ function initLastSeenService(flagEnabled = true) {
const lastSeenService = new LastSeenService(
{
lastSeenStore: stores.lastSeenStore,
featureToggleStore: stores.featureToggleStore,
},
config,
);
Expand Down

0 comments on commit 106e1d1

Please sign in to comment.