discard and log error about data items that are too big to store (#26)

* discard and log error about data items that are too big to store

* update size limit calculation to use UTF8 byte length & add index overhead

Author: eli-darkly

README.md

@@ -60,6 +60,22 @@ To reduce traffic to DynamoDB, there is an optional in-memory cache that retains
 
         const store = DynamoDBFeatureStore('YOUR TABLE NAME', { cacheTTL: 0 });
 
+## Data size limitation
+
+DynamoDB has [a 400KB limit](https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/ServiceQuotas.html#limits-items) on the size of any data item. For the LaunchDarkly SDK, a data item consists of the JSON representation of an individual feature flag or segment configuration, plus a few smaller attributes. You can see the format and size of these representations by querying `https://sdk.launchdarkly.com/flags/latest-all` and setting the `Authorization` header to your SDK key.
+
+Most flags and segments won't be nearly as big as 400KB, but they could be if for instance you have a long list of user keys for individual user targeting. If the flag or segment representation is too large, it cannot be stored in DynamoDB. To avoid disrupting storage and evaluation of other unrelated feature flags, the SDK will simply skip storing that individual flag or segment, and will log a message (at ERROR level) describing the problem. For example:
+
+```
+    The item "my-flag-key" in "features" was too large to store in DynamoDB and was dropped
+```
+
+If caching is enabled in your configuration, the flag or segment may still be available in the SDK from the in-memory cache, but do not rely on this. If you see this message, consider redesigning your flag/segment configurations, or else do not use DynamoDB for the environment that contains this data item.
+
+This limitation does not apply to target lists in [Big Segments](https://docs.launchdarkly.com/home/users/big-segments/).
+
+A future version of the LaunchDarkly DynamoDB integration may use different strategies to work around this limitation, such as compressing the data or dividing it into multiple items. However, this integration is required to be interoperable with the DynamoDB integrations used by all the other LaunchDarkly SDKs and by the Relay Proxy, so any such change will only be made as part of a larger cross-platform release.
+
 ## About LaunchDarkly
 
 * LaunchDarkly is a continuous delivery platform that provides feature flags as a service and allows developers to iterate quickly and safely. We allow you to easily flag your features and manage them from the LaunchDarkly dashboard.  With LaunchDarkly, you can:

dynamodb_feature_store.js

@@ -3,6 +3,11 @@ const CachingStoreWrapper = require('launchdarkly-node-server-sdk/caching_store_
 
 const defaultCacheTTLSeconds = 15;
 
+// We won't try to store items whose total size exceeds this. The DynamoDB documentation says
+// only "400KB", which probably means 400*1024, but to avoid any chance of trying to store a
+// too-large item we are rounding it down.
+const dynamoDbMaxItemSize = 400000;
+
 // Note that the format of parameters in this implementation is a bit different than in the
 // LD DynamoDB integrations for some other platforms, because we are using the
 // AWS.DynamoDB.DocumentClient class, which represents values as simple types like
@@ -79,8 +84,11 @@ function dynamoDBFeatureStoreInternal(tableName, options, logger) {
         allData.forEach(function(collection) {
           collection.items.forEach(function(item) {
             var key = item.key;
-            delete existingNamespaceKeys[namespaceForKind(collection.kind) + '$' + key];
-            ops.push({ PutRequest: { Item: marshalItem(collection.kind, item) } });
+            const dbItem = marshalItem(collection.kind, item);
+            if (checkSizeLimit(dbItem)) {
+              delete existingNamespaceKeys[namespaceForKind(collection.kind) + '$' + key];
+              ops.push({ PutRequest: { Item: dbItem } });
+            }
           });
         });
 
@@ -105,6 +113,12 @@ function dynamoDBFeatureStoreInternal(tableName, options, logger) {
 
   store.upsertInternal = function(kind, item, cb) {
     var params = makeVersionedPutRequest(kind, item);
+    if (!checkSizeLimit(params.Item)) {
+      // We deliberately don't report this back to the SDK as an error, because we don't want to trigger any
+      // useless retry behavior. We just won't do the update.
+      cb(null, null);
+      return;
+    }
 
     // testUpdateHook is instrumentation, used only by the unit tests
     var prepare = store.testUpdateHook || function(prepareCb) { prepareCb(); };
@@ -213,6 +227,23 @@ function dynamoDBFeatureStoreInternal(tableName, options, logger) {
     return item.namespace + '$' + item.key;
   }
 
+  function checkSizeLimit(item) {
+    // see: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/CapacityUnitCalculations.html
+    let size = 100; // fixed overhead for index data
+    for (const [key, value] of Object.entries(item)) {
+      size += key.length + Buffer.byteLength(value.toString());
+    }
+    if (size <= dynamoDbMaxItemSize) {
+      return true;
+    }
+    logSizeLimitError(item.namespace, item.key);
+    return false;
+  }
+
+  function logSizeLimitError(namespace, key) {
+    logger.error(`The item "${key}" in "${namespace}" was too large to store in DynamoDB and was dropped`);
+  }
+  
   return store;
 }
 

tests/dynamodb_stores-test.js

@@ -14,7 +14,7 @@ const {
 } = require('launchdarkly-node-server-sdk/sharedtest/store_tests');
 const AWS = require('aws-sdk');
 const { promisify } = require('util');
-const { asyncSleep } = require('launchdarkly-js-test-helpers');
+const { asyncSleep, promisifySingle } = require('launchdarkly-js-test-helpers');
 
 // Runs the standard test suites provided by the SDK's store_tests module, plus some additional
 // tests specific to this package.
@@ -88,57 +88,144 @@ describe('DynamoDBFeatureStore', function() {
       store = DynamoDBFeatureStore(testTableName, { dynamoDBClient: client })({ logger });
     });
 
-    it('error from query in init', done => {
+    it('error from query in init', async () => {
       var data = { features: { flag: { key: 'flag', version: 1 } } };
       client.query = (params, cb) => cb(err);
-      store.init(data, function() {
-        expect(logger.error).toHaveBeenCalled();
-        done();
-      });
+      await promisifySingle(store.init)(data);
+      expect(logger.error).toHaveBeenCalled();
     });
 
-    it('error from batchWrite in init', done => {
+    it('error from batchWrite in init', async () => {
       var data = { features: { flag: { key: 'flag', version: 1 } } };
       client.query = (params, cb) => cb(null, { Items: [] });
       client.batchWrite = (params, cb) => cb(err);
-      store.init(data, function() {
-        expect(logger.error).toHaveBeenCalled();
-        done();
-      });
+      await promisifySingle(store.init)(data);
+      expect(logger.error).toHaveBeenCalled();
     });
 
-    it('error from get', done => {
+    it('error from get', async () => {
       client.get = (params, cb) => cb(err);
-      store.get(dataKind.features, 'flag', function(result) {
-        expect(result).toBe(null);
-        expect(logger.error).toHaveBeenCalled();
-        done();
-      });
+      const result = await promisifySingle(store.get)(dataKind.features, 'flag');
+      expect(result).toBe(null);
+      expect(logger.error).toHaveBeenCalled();
     });
 
-    it('error from get all', done => {
+    it('error from get all', async () => {
       client.query = (params, cb) => cb(err);
-      store.all(dataKind.features, function(result) {
-        expect(result).toBe(null);
-        expect(logger.error).toHaveBeenCalled();
-        done();
-      });
+      const result = await promisifySingle(store.all)(dataKind.features);
+      expect(result).toBe(null);
+      expect(logger.error).toHaveBeenCalled();
     });
 
-    it('error from upsert', done => {
+    it('error from upsert', async () => {
       client.put = (params, cb) => cb(err);
-      store.upsert(dataKind.features, { key: 'flag', version: 1 }, function() {
-        expect(logger.error).toHaveBeenCalled();
-        done();
-      });
+      await promisifySingle(store.upsert)(dataKind.features, { key: 'flag', version: 1 });
+      expect(logger.error).toHaveBeenCalled();
     });
 
-    it('error from initialized', done => {
+    it('error from initialized', async () => {
       client.get = (params, cb) => cb(err);
-      store.initialized(function(result) {
-        expect(result).toBe(false);
-        expect(logger.error).toHaveBeenCalled();
-        done();
+      const result = await promisifySingle(store.initialized)();
+      expect(result).toBe(false);
+      expect(logger.error).toHaveBeenCalled();
+    });
+  });
+
+  describe('items over the data size limit', () => {
+    let logger;
+    let store;
+
+    beforeEach(() => {
+      logger = stubLogger();
+      store = DynamoDBFeatureStore(testTableName, { cacheTTL: 0 })({ logger });
+      // Here we're disabling caching because we want to see if the data is really put into the database. If
+      // there's a cache, data could be stored in and retrieved from the cache even though the data was too
+      // big to go into the database.
+    });
+
+    function makeGoodData() {
+      return {
+        features: {
+          flag1: { key: 'flag1', version: 1 },
+          flag2: { key: 'flag2', version: 1 }
+        },
+        segments: {
+          segment1: { key: 'segment1', version: 1 },
+          segment2: { key: 'segment2', version: 1 },
+        }
+      };
+    }
+
+    async function getAllData() {
+      const flags = await promisifySingle(store.all)(dataKind.features);
+      const segments = await promisifySingle(store.all)(dataKind.segments);
+      return { features: flags, segments: segments };
+    }
+
+    function makeBigKeyList() {
+      const ret = [];
+      for (let i = 0; i < 40000; i++) {
+        ret.push('key' + i);
+      }
+      expect(JSON.stringify(ret).length).toBeGreaterThan(400 * 1024);
+      return ret;
+    }
+
+    function makeTooBigFlag() {
+      return { key: 'flag1a', version: 1, targets: [{ variation: 0, values: makeBigKeyList() }] };
+    }
+
+    function makeTooBigSegment() {
+      return { key: 'segment1a', version: 1, included: makeBigKeyList() };
+    }
+    
+    function expectSizeLimitErrorLog(logger) {
+      expect(logger.error.mock.calls.length).toBe(1);
+      expect(logger.error.mock.calls[0][0]).toContain('was too large to store in DynamoDB and was dropped');
+    }
+
+    describe('skips and logs too-large item in init', () => {
+      async function testInit(kind, item) {
+        await clearData();
+
+        const data = makeGoodData();
+        data[kind.namespace][item.key] = item;  
+        await promisifySingle(store.init)(data);
+
+        expectSizeLimitErrorLog(logger);
+        expect(await getAllData()).toEqual(makeGoodData());
+      }
+
+      it('flag', async () => {
+        await testInit(dataKind.features, makeTooBigFlag());
+      });
+      
+      it('segment', async () => {
+        await testInit(dataKind.segments, makeTooBigSegment());
+      });
+    });
+
+    describe('skips and logs too-large item in upsert', () => {
+      async function testUpsert(kind, item) {
+        await clearData();
+
+        const data = makeGoodData();
+        await promisifySingle(store.init)(data);
+        expect(logger.error).not.toHaveBeenCalled();
+        expect(await getAllData()).toEqual(makeGoodData());
+
+        await promisify(store.upsert)(kind, item);
+
+        expectSizeLimitErrorLog(logger);
+        expect(await getAllData()).toEqual(makeGoodData());
+      }
+
+      it('flag', async () => {
+        await testUpsert(dataKind.features, makeTooBigFlag());
+      });
+      
+      it('segment', async () => {
+        await testUpsert(dataKind.segments, makeTooBigSegment());
       });
     });
   });