Pass through logs if already in JSON format #3

kwessel · 2023-02-16T18:36:54Z

No description provided.

splunk-cloudwatch-logs-processor/index.js

ddriddle

We made some suggestions to simplify the code. I have have some design questions below.

splunk-cloudwatch-logs-processor/index.js

ddriddle

The code looks good but it could be simplified per my comments and spacing could be improved to be more in line with what is standard for Javascript.

kwessel · 2023-03-09T02:30:27Z

The code looks good but it could be simplified per my comments and spacing could be improved to be more in line with what is standard for Javascript.

I will freely admit that my knowledge of Javascript formatting and styling is sadly lacking. If you have some time tomorrow to talk me through that, I'll happily correct it. If there were other code comments here besides the comment about what to do or not do based on the presence of the message key, or if you want to discuss that one further, I'm also game for that. Let's plan to talk through the rest of these, and I'll make any other commits, trivial or otherwise, so we can wrap this up. Thank you.

JonRoma · 2023-03-09T03:24:48Z

I'm not particularly enthused about supporting a module in Javascript, a language that neither I nor anyone in SDG is remotely fluent with. I thought we were to going to replace this with Python if any significant changes were made.

edthedev · 2023-03-09T14:07:17Z

@JonRoma Yes. We need to plan a Python re-write for this, since getting this kind of update means it's coming out of semi-retirement. But we won't delay these changes behind a rewrite. I would like to see how @kwessel 's planned testing pans out so we know what we need when we're ready to rewrite in Python.

JonRoma · 2023-03-09T18:01:17Z

I should talk with @kwessel to better understand this use case, because I've been flitting from one thing to another, and have been a bit inattentive to this PR (because I have so many other things going on). Also, I simply haven't talked with @kwessel in a long time, so we should catch up. :-)

This is Javascript simply because Splunk had a sample lambda function already written, and we decided to use it rather than reinvent the wheel. He knew enough JS to get us through this. When we do this, we should aim for making the python replacement be a minimum viable product, but also bear in mind what other service owners might need. The whole build and test framework for this module should be replaced with something better too.

@JonRoma Yes. We need to plan a Python re-write for this, since getting this kind of update means it's coming out of semi-retirement. But we won't delay these changes behind a rewrite. I would like to see how @kwessel 's planned testing pans out so we know what we need when we're ready to rewrite in Python.

ddriddle

The code looks great. Please tell us how testing goes. It would be nice if the SSM param splunk_json was treated as optional and defaulting to false.

kwessel · 2023-03-10T19:20:57Z

It would be nice if the SSM param splunk_json was treated as optional and defaulting to false.

This is done. I'd welcome any suggestions for more efficient or readable code on lines 117-118 and a sanity check of my turnary operator usage on line 162.

ddriddle

The code on 116-120 looks good but it is hard to tell if it is correct without testing it since I don't know the SSM API well. I'm a little confused why you are checking for != -1 then setting to 1. I would have expected you to set to -1 instead. Can you explain in a comment how the logic works? A link to relevant documentation would be great.

I don't understand the intention for the ternary operator on 162. Are you also trying to make sourcetype optional or allow empty values? Some context would be appreciated.

kwessel · 2023-03-10T20:20:19Z

I'm a little confused why you are checking for != -1 then setting to 1. I would have expected you to set to -1 instead. Can you explain in a comment how the logic works?

I'm not setting it to -1 or 1. I'm checking for the return value of indexOf. In NodeJS, indexOf returns -1 if it doesn't find the given balue in the array. So, my code tests to see if InvalidParameters is a list and, if it is, if it contains 'splunk_json'. If it does, it then uses the splice method on the array to remove that particular item from the array. Splice takes the index where it should start splicing as the first paramenter and the number of elements to remove as the second. So, it starts at the index where splunk_json lives and removes one item. I was afraid those lines might not be clear. I welcome suggestions to make them more readable.

I don't understand the intention for the ternary operator on 162. Are you also trying to make sourcetype optional or allow empty values? Some context would be appreciated.

The context is I'm half asleep. That was supposed to be on line 163. The operator should have been on splunkJSON, not sourceType. 162has been restored, and the change has been applied to 163. Please review and comment or approve as appropriate. Thank you, all!

ddriddle

@kwessel Thank you for the clarification. I strongly suggest adding a better comment on line 116:

            // Remove optional splunk_json from InvalidParameters if it exists

My JS foo is not strong enough to suggest a refactor but I'm fine with the code as is as long as you improve the comment. I have asked @mpitcel to take a look and see if she can suggest a refactor.

If you could also remove the blank line on 216 that would be great.

Otherwise everything looks good. Please test the code with both splunk_json present and absent in an AWS account to be sure everything works as expected.

ddriddle · 2023-03-13T18:35:43Z

splunk-cloudwatch-logs-processor/index.js

@@ -112,6 +113,11 @@ const getSplunkLogger = (parsed, context, callback) => {

            console.log('Data retrieved from SSM:', JSON.stringify(ssmData, null, 2));

+            // Make splunk_json optional


Please update comment for folks who are not JS coders:

Suggested change

// Make splunk_json optional

// Remove optional splunk_json from InvalidParameters if it exists

ddriddle · 2023-03-13T18:36:49Z

splunk-cloudwatch-logs-processor/index.js

+            if (InvalidParameters in ssmData && ssmData.InvalidParameters.indexOf('splunk_json') != -1) {
+                ssmData.InvalidParameters.splice(ssmData.InvalidParameters.indexOf('splunk_json'), 1);
+            }
+


@mpitcel any refactor suggestions to make this code clearer for folks who are not JS coders?

ddriddle · 2023-03-13T18:51:50Z

splunk-cloudwatch-logs-processor/index.js


+            }


Remove unnecessary blank line:

Suggested change

}

}

kwessel added the enhancement New feature or request label Feb 16, 2023

kwessel requested review from edthedev, JonRoma, ddriddle and eccoleman February 16, 2023 18:36

kwessel self-assigned this Feb 16, 2023

edthedev previously approved these changes Feb 16, 2023

View reviewed changes

mpitcel previously approved these changes Feb 16, 2023

View reviewed changes

tzturner previously approved these changes Feb 16, 2023

View reviewed changes

tzturner reviewed Feb 16, 2023

View reviewed changes

splunk-cloudwatch-logs-processor/index.js Outdated Show resolved Hide resolved

Pass through logs if already in JSON format

cfee6c6

kwessel dismissed stale reviews from tzturner, mpitcel, and edthedev via cfee6c6 February 16, 2023 20:22

kwessel force-pushed the feature/allow-json-logging branch from aa0cc4d to cfee6c6 Compare February 16, 2023 20:22

ddriddle approved these changes Feb 16, 2023

View reviewed changes

edthedev previously approved these changes Feb 20, 2023

View reviewed changes

ddriddle previously approved these changes Mar 8, 2023

View reviewed changes

Simplify flow

96969d0

kwessel dismissed stale reviews from ddriddle and edthedev via 96969d0 March 9, 2023 21:42

ddriddle previously approved these changes Mar 10, 2023

View reviewed changes

kwessel dismissed ddriddle’s stale review via 44ebeaf March 10, 2023 18:51

kwessel force-pushed the feature/allow-json-logging branch 2 times, most recently from 44ebeaf to 968cfc8 Compare March 10, 2023 19:19

ddriddle previously approved these changes Mar 10, 2023

View reviewed changes

Use optional SSM variable to determine JSON-formatted splunk payload

3e46306

kwessel dismissed ddriddle’s stale review via 3e46306 March 10, 2023 20:14

kwessel force-pushed the feature/allow-json-logging branch from 968cfc8 to 3e46306 Compare March 10, 2023 20:14

ddriddle requested review from tzturner, mpitcel, edthedev and ddriddle and removed request for tzturner and mpitcel March 13, 2023 17:57

ddriddle approved these changes Mar 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass through logs if already in JSON format #3

Pass through logs if already in JSON format #3

kwessel commented Feb 16, 2023

ddriddle left a comment

ddriddle left a comment

kwessel commented Mar 9, 2023

JonRoma commented Mar 9, 2023

edthedev commented Mar 9, 2023

JonRoma commented Mar 9, 2023

ddriddle left a comment

kwessel commented Mar 10, 2023

ddriddle left a comment

kwessel commented Mar 10, 2023

ddriddle left a comment

ddriddle Mar 13, 2023

ddriddle Mar 13, 2023

ddriddle Mar 13, 2023

		@@ -112,6 +113,11 @@ const getSplunkLogger = (parsed, context, callback) => {

		console.log('Data retrieved from SSM:', JSON.stringify(ssmData, null, 2));

		// Make splunk_json optional

	// Make splunk_json optional
	// Remove optional splunk_json from InvalidParameters if it exists

Pass through logs if already in JSON format #3

Are you sure you want to change the base?

Pass through logs if already in JSON format #3

Conversation

kwessel commented Feb 16, 2023

ddriddle left a comment

Choose a reason for hiding this comment

ddriddle left a comment

Choose a reason for hiding this comment

kwessel commented Mar 9, 2023

JonRoma commented Mar 9, 2023

edthedev commented Mar 9, 2023

JonRoma commented Mar 9, 2023

ddriddle left a comment

Choose a reason for hiding this comment

kwessel commented Mar 10, 2023

ddriddle left a comment

Choose a reason for hiding this comment

kwessel commented Mar 10, 2023

ddriddle left a comment

Choose a reason for hiding this comment

ddriddle Mar 13, 2023

Choose a reason for hiding this comment

ddriddle Mar 13, 2023

Choose a reason for hiding this comment

ddriddle Mar 13, 2023

Choose a reason for hiding this comment