From f0895cf653799741e4f539639976b22f941daab6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivar=20Conradi=20=C3=98sthus?= Date: Thu, 11 Nov 2021 16:05:29 +0100 Subject: [PATCH] fix: prevent deadlock for batchinserting usage metrics (#1100) * fix: prevent deadlock for batchinserting usage metrics In client metrics v2 we utilize postgres to count the usage across a few dimentions (featureName, app_name, environment). It turns out that if the UPDATE values are not executed in a predictable order we can end up in a deadlock scenario with postgresql. In this fix we thus sort the metrics on the feature_name, app_name and envrionment, to make sure they always are executed in a predictabel order, and thus avoiding independent inserts colliding in to a deadlock waiting for eachother. * fix: tests cannot assume order --- src/lib/db/client-metrics-store-v2.ts | 10 +++++- .../e2e/api/admin/client-metrics.e2e.test.ts | 34 ++++++++++++------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/lib/db/client-metrics-store-v2.ts b/src/lib/db/client-metrics-store-v2.ts index dfc8494f52..357457697b 100644 --- a/src/lib/db/client-metrics-store-v2.ts +++ b/src/lib/db/client-metrics-store-v2.ts @@ -116,9 +116,17 @@ export class ClientMetricsStoreV2 implements IClientMetricsStoreV2 { return prev; }, {}); + // Sort the rows to avoid deadlocks + const batchRow = Object.values(batch).sort( + (a, b) => + a.feature_name.localeCompare(b.feature_name) || + a.app_name.localeCompare(b.app_name) || + a.environment.localeCompare(b.environment), + ); + // Consider rewriting to SQL batch! const insert = this.db(TABLE) - .insert(Object.values(batch)) + .insert(batchRow) .toQuery(); const query = `${insert.toString()} ON CONFLICT (feature_name, app_name, environment, timestamp) DO UPDATE SET "yes" = "client_metrics_env"."yes" + EXCLUDED.yes, "no" = "client_metrics_env"."no" + EXCLUDED.no`; diff --git a/src/test/e2e/api/admin/client-metrics.e2e.test.ts b/src/test/e2e/api/admin/client-metrics.e2e.test.ts index 6558c076a8..f9f2ad7680 100644 --- a/src/test/e2e/api/admin/client-metrics.e2e.test.ts +++ b/src/test/e2e/api/admin/client-metrics.e2e.test.ts @@ -155,14 +155,19 @@ test('should return toggle summary', async () => { .expect('Content-Type', /json/) .expect(200); + const test = demo.lastHourUsage.find((u) => u.environment === 'test'); + const defaultEnv = demo.lastHourUsage.find( + (u) => u.environment === 'default', + ); + expect(demo.featureName).toBe('demo'); expect(demo.lastHourUsage).toHaveLength(2); - expect(demo.lastHourUsage[0].environment).toBe('default'); - expect(demo.lastHourUsage[0].yes).toBe(5); - expect(demo.lastHourUsage[0].no).toBe(4); - expect(demo.lastHourUsage[1].environment).toBe('test'); - expect(demo.lastHourUsage[1].yes).toBe(2); - expect(demo.lastHourUsage[1].no).toBe(6); + expect(test.environment).toBe('test'); + expect(test.yes).toBe(2); + expect(test.no).toBe(6); + expect(defaultEnv.environment).toBe('default'); + expect(defaultEnv.yes).toBe(5); + expect(defaultEnv.no).toBe(4); expect(demo.seenApplications).toStrictEqual(['backend-api', 'web']); }); @@ -219,13 +224,18 @@ test('should only include last hour of metrics return toggle summary', async () .expect('Content-Type', /json/) .expect(200); + const test = demo.lastHourUsage.find((u) => u.environment === 'test'); + const defaultEnv = demo.lastHourUsage.find( + (u) => u.environment === 'default', + ); + expect(demo.featureName).toBe('demo'); expect(demo.lastHourUsage).toHaveLength(2); - expect(demo.lastHourUsage[0].environment).toBe('default'); - expect(demo.lastHourUsage[0].yes).toBe(5); - expect(demo.lastHourUsage[0].no).toBe(4); - expect(demo.lastHourUsage[1].environment).toBe('test'); - expect(demo.lastHourUsage[1].yes).toBe(2); - expect(demo.lastHourUsage[1].no).toBe(6); + expect(defaultEnv.environment).toBe('default'); + expect(defaultEnv.yes).toBe(5); + expect(defaultEnv.no).toBe(4); + expect(test.environment).toBe('test'); + expect(test.yes).toBe(2); + expect(test.no).toBe(6); expect(demo.seenApplications).toStrictEqual(['backend-api', 'web']); });