Initial PR - Create transport #1

RLRabinowitz · 2021-06-21T18:36:30Z

This PR creates the transport. It's very similar to the implementation of https://github.com/lazywithclass/winston-cloudwatch, with some adjustments for DynamoDB:

Index.js:

Throw exception when region doesn't support DynamoDB (partially taken from https://github.com/jwchang0206/winston-dynamodb)
Remove logGroupName and retentionInDays, which is CW specific, and add tableName instead
Change AWS config format to match that of DynamoDB client
Rename every CW to DynamoDB

util.js:

Rename WINSTON_CLOUDWATCH_DEBUG to WINSTON_DYNAMODB_DEBUG

dynamodb-integration.js:

Remove all unnecessary logic from cloudwatch-integration.js - Managing tokens and related errors, not being able to send multiple batch requests simultaniously...
Keep size limit logic + message truncation
Do not allow more than 25 items in a batch
Creates a batch request. The id is the streamName, timestamp is the timestamp of log addition, message is the log message

avnerenv0

@RLRabinowitz This looks good enough to use, but I reviewed it like 'new code', feel free to apply comments however you like

avnerenv0 · 2021-06-22T19:47:40Z

index.js

+var util = require('util'),
+    winston = require('winston'),
+    AWS = require('aws-sdk'),
+    dynamodbIntegration = require('./lib/dynamodb-integration'),
+    isEmpty = require('lodash.isempty'),
+    assign = require('lodash.assign'),
+    isError = require('lodash.iserror'),
+    stringify = require('./lib/utils').stringify,
+    debug = require('./lib/utils').debug,
+    defaultFlushTimeoutMs = 10000;


put a const in each line?

avnerenv0 · 2021-06-22T19:49:03Z

index.js

+    if (!/^uncaughtException: /.test(info.message)) {
+        // do not wait, just return right away
+        return callback(null, true);
+    }


this one is confusing - maybe reverse the condition

Since I'm not too sure on this code here (Specifically what the callback(null, true) means. It seems that the callback is called immediately with these params on logger.info, but is called again with other params when the batch is sent), I'll probably skip this...

callbacks usually follow the pattern of func(error, result) and in this case cb(null, true) means everything worked. This is the 'happy flow' and will happen for EVERY log call except when the message includes uncaughtException.

It's confusing because the 'normal' case is inside the if and short circuits this function.

If the error message does include uncaughtException then we clear everything and submit the logs and get ready for a 'crash' exit of the process.

avnerenv0 · 2021-06-22T19:50:16Z

index.js

+WinstonDynamoDB.prototype.add = function(log) {
+    debug('add log to queue', log);
+
+    var self = this;


maybe use arrow functions and get rid of this shameful piece of javascript history

Interesting, read online and didn't know about the whole this scope thing that is different between functions and arrow functions.
TIL

avnerenv0 · 2021-06-22T19:51:31Z

index.js

+    );
+};
+
+WinstonDynamoDB.prototype.kthxbye = function(callback) {


kthxbye is unique to that transport right?
Doesn't winston have something more 'built-in' ?

Yep it's unique to the transport

index.js

avnerenv0 · 2021-06-22T19:54:27Z

lib/dynamodb-integration.js

@@ -0,0 +1,83 @@
+var LIMITS = {


prefer const

avnerenv0 · 2021-06-22T19:56:17Z

jest.config.js

@@ -0,0 +1,9 @@
+/*


why is this needed if there aren't any tests?

Right. This isn't need yet. I can remove it and add it later when necessary

avnerenv0 · 2021-06-22T19:57:26Z

lib/dynamodb-integration.js

+    const payload = {
+        RequestItems: {
+            [tableName]: eventsToSend.map(event =>
+                ({


The streamName will be the partition key and the timestamp sort key?

In this PR, yes. I'll change the timestamp to be a sequenceNumber in a next PR, because of this issue

avnerenv0 · 2021-06-22T19:58:10Z

lib/dynamodb-integration.js

+    var entryIndex = 0;
+    var bytes = 0;
+    while (entryIndex < Math.min(logEvents.length, LIMITS.MAX_BATCH_ITEM_NUM)) {
+        var ev = logEvents[entryIndex];
+        // unit tests pass null elements
+        var evSize = ev ? Buffer.byteLength(ev.message, 'utf8') + BASE_EVENT_SIZE_BYTES : 0;
+        if(evSize > LIMITS.MAX_EVENT_MSG_SIZE_BYTES) {
+            evSize = LIMITS.MAX_EVENT_MSG_SIZE_BYTES;
+            ev.message = ev.message.substring(0, evSize);
+            const msgTooBigErr = new Error('Message Truncated because it exceeds the DynamoDB size limit');
+            msgTooBigErr.logEvent = ev;
+            cb(msgTooBigErr);
+        }
+        if (bytes + evSize > LIMITS.MAX_BATCH_SIZE_BYTES) break;
+        bytes += evSize;
+        entryIndex++;
+    }


I would put all this in a different function

avnerenv0 · 2021-06-22T19:59:26Z

lib/dynamodb-integration.js

+            ev.message = ev.message.substring(0, evSize);
+            const msgTooBigErr = new Error('Message Truncated because it exceeds the DynamoDB size limit');
+            msgTooBigErr.logEvent = ev;
+            cb(msgTooBigErr);


this calls the callback but continues the run ? doesn't seem right, maybe missing a return ?
Also maybe instead we can split the large message to several smaller messages ?

Thought about it, but since this is the existent logic in CW transport, I've decided not to touch this.
Also, the size limit for the truncation is bigger on DynamoDB (400KB) than in CW (256KB), so I wouldn't worry about it for our use case

avnerenv0 · 2021-06-22T20:02:38Z

I would also add a Dynamo DB Table definition file

RLRabinowitz · 2021-06-23T06:52:42Z

I would also add a Dynamo DB Table definition file

Add this to the missing README issue

RLRabinowitz · 2021-06-23T14:25:17Z

DynamoDB winston transport

RLRabinowitz self-assigned this Jun 21, 2021

Initial Commit

9736f0d

RLRabinowitz force-pushed the chore-initial-setup branch from 35bb93f to 9736f0d Compare June 22, 2021 13:45

RLRabinowitz changed the title ~~Temp PR~~ Initial PR - Create transport Jun 22, 2021

RLRabinowitz requested a review from a team June 22, 2021 16:35

Some minor changes to align more with CW transport

fcefa8a

avnerenv0 approved these changes Jun 22, 2021

View reviewed changes

Fix transport name

0a1675e

RLRabinowitz added 4 commits June 23, 2021 10:47

Use const instead of vars

7e08222

Remove usage of self, use arrow functions instead

230fc51

Extract methods

e136421

Remove jest, as it's not used (YET)

70d22ef

RLRabinowitz merged commit 7690388 into main Jun 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial PR - Create transport #1

Initial PR - Create transport #1

RLRabinowitz commented Jun 21, 2021 •

edited

Loading

avnerenv0 left a comment

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 Jun 22, 2021

RLRabinowitz Jun 23, 2021

avnerenv0 commented Jun 22, 2021

RLRabinowitz commented Jun 23, 2021 •

edited

Loading

RLRabinowitz commented Jun 23, 2021

Initial PR - Create transport #1

Initial PR - Create transport #1

Conversation

RLRabinowitz commented Jun 21, 2021 • edited Loading

avnerenv0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avnerenv0 commented Jun 22, 2021

RLRabinowitz commented Jun 23, 2021 • edited Loading

RLRabinowitz commented Jun 23, 2021

RLRabinowitz commented Jun 21, 2021 •

edited

Loading

RLRabinowitz commented Jun 23, 2021 •

edited

Loading