From 86b5f108d037942d93d28aadc5d1ccf36b97176a Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 18 Jan 2024 11:00:56 +0100 Subject: [PATCH] fix: feature toggle update total needs 4 labels (#5946) So, this was causing a lot of ERROR in our logs, due to the metric having gotten an extra label the last month. Two things for this fix. 1. add the missing label to the two calls that did not have it added 2. update the log line to include the error as another argument to the logger, so we actually get a stacktrace from the error. --- .../scheduler/scheduler-service.test.ts | 45 +++++++------------ .../features/scheduler/scheduler-service.ts | 7 ++- src/lib/metrics.ts | 8 +++- src/lib/services/scheduler-service.test.ts | 22 +++++---- 4 files changed, 40 insertions(+), 42 deletions(-) diff --git a/src/lib/features/scheduler/scheduler-service.test.ts b/src/lib/features/scheduler/scheduler-service.test.ts index 8c8f71c840..5e1b1b7836 100644 --- a/src/lib/features/scheduler/scheduler-service.test.ts +++ b/src/lib/features/scheduler/scheduler-service.test.ts @@ -176,10 +176,15 @@ test('Can handle crash of a async job', async () => { await ms(75); schedulerService.stop(); - expect(getRecords()).toEqual([ - ['initial scheduled job failed | id: test-id-10 | async reason'], - ['interval scheduled job failed | id: test-id-10 | async reason'], - ]); + const records = getRecords(); + expect(records[0][0]).toContain( + 'initial scheduled job failed | id: test-id-10', + ); + expect(records[0][1]).toContain('async reason'); + expect(records[1][0]).toContain( + 'interval scheduled job failed | id: test-id-10', + ); + expect(records[1][1]).toContain('async reason'); }); test('Can handle crash of a sync job', async () => { @@ -196,30 +201,14 @@ test('Can handle crash of a sync job', async () => { await ms(75); schedulerService.stop(); - expect(getRecords()).toEqual([ - ['initial scheduled job failed | id: test-id-11 | Error: sync reason'], - ['interval scheduled job failed | id: test-id-11 | Error: sync reason'], - ]); -}); - -test('Can handle crash of a async job', async () => { - const { logger, getRecords } = getLogger(); - const { schedulerService } = createSchedulerTestService({ - loggerOverride: logger, - }); - - const job = async () => { - await Promise.reject('async reason'); - }; - - await schedulerService.schedule(job, 50, 'test-id-10'); - await ms(75); - - schedulerService.stop(); - expect(getRecords()).toEqual([ - ['initial scheduled job failed | id: test-id-10 | async reason'], - ['interval scheduled job failed | id: test-id-10 | async reason'], - ]); + const records = getRecords(); + expect(records[0][0]).toContain( + 'initial scheduled job failed | id: test-id-11', + ); + expect(records[0][1].message).toContain('sync reason'); + expect(records[1][0]).toContain( + 'interval scheduled job failed | id: test-id-11', + ); }); it('should emit scheduler job time event when scheduled function is run', async () => { diff --git a/src/lib/features/scheduler/scheduler-service.ts b/src/lib/features/scheduler/scheduler-service.ts index 1a6b8dc344..9852d6d17e 100644 --- a/src/lib/features/scheduler/scheduler-service.ts +++ b/src/lib/features/scheduler/scheduler-service.ts @@ -52,7 +52,8 @@ export class SchedulerService { } } catch (e) { this.logger.error( - `interval scheduled job failed | id: ${id} | ${e}`, + `interval scheduled job failed | id: ${id}`, + e, ); } }, timeMs).unref(), @@ -64,9 +65,7 @@ export class SchedulerService { await runScheduledFunctionWithEvent(); } } catch (e) { - this.logger.error( - `initial scheduled job failed | id: ${id} | ${e}`, - ); + this.logger.error(`initial scheduled job failed | id: ${id}`, e); } } diff --git a/src/lib/metrics.ts b/src/lib/metrics.ts index 115e09acbd..32df758e8d 100644 --- a/src/lib/metrics.ts +++ b/src/lib/metrics.ts @@ -456,10 +456,14 @@ export default class MetricsMonitor { }, ); eventStore.on(FEATURE_ARCHIVED, ({ featureName, project }) => { - featureToggleUpdateTotal.labels(featureName, project, 'n/a').inc(); + featureToggleUpdateTotal + .labels(featureName, project, 'n/a', 'n/a') + .inc(); }); eventStore.on(FEATURE_REVIVED, ({ featureName, project }) => { - featureToggleUpdateTotal.labels(featureName, project, 'n/a').inc(); + featureToggleUpdateTotal + .labels(featureName, project, 'n/a', 'n/a') + .inc(); }); eventBus.on(CLIENT_METRICS, (m: ValidatedClientMetrics) => { diff --git a/src/lib/services/scheduler-service.test.ts b/src/lib/services/scheduler-service.test.ts index 7321b0d4dd..64fbd55e59 100644 --- a/src/lib/services/scheduler-service.test.ts +++ b/src/lib/services/scheduler-service.test.ts @@ -100,10 +100,13 @@ test('Can handle crash of a async job', async () => { await ms(75); schedulerService.stop(); - expect(getRecords()).toEqual([ - ['initial scheduled job failed | id: test-id-10 | async reason'], - ['interval scheduled job failed | id: test-id-10 | async reason'], - ]); + const records = getRecords(); + expect(records[0][0]).toContain( + 'initial scheduled job failed | id: test-id-10', + ); + expect(records[1][0]).toContain( + 'interval scheduled job failed | id: test-id-10', + ); }); test('Can handle crash of a sync job', async () => { @@ -115,8 +118,11 @@ test('Can handle crash of a sync job', async () => { await ms(75); schedulerService.stop(); - expect(getRecords()).toEqual([ - ['initial scheduled job failed | id: test-id-11 | Error: sync reason'], - ['interval scheduled job failed | id: test-id-11 | Error: sync reason'], - ]); + const records = getRecords(); + expect(records[0][0]).toContain( + 'initial scheduled job failed | id: test-id-11', + ); + expect(records[1][0]).toContain( + 'interval scheduled job failed | id: test-id-11', + ); });