Fix to better handle lambda responses when they are empty or null or not a valid json #5211

san81 · 2024-11-21T20:51:52Z

Description

In the cases where the Lambda response is not a proper Json, we are better handling the logic now as described below

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…not a valid json Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

dlvenable · 2024-11-21T22:54:21Z

...ambda/src/main/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessor.java

@@ -72,6 +73,7 @@ public class LambdaProcessor extends AbstractProcessor<Record<Event>, Record<Eve
    private final Counter numberOfRecordsFailedCounter;
    private final Counter numberOfRequestsSuccessCounter;
    private final Counter numberOfRequestsFailedCounter;
+    private final Counter lambdaResponseRecordsCounter;


Should we use a DistributionSummary here? This will let us both count and understand the sizes per request.

@dlvenable let's do this in the follow up PR

dlvenable · 2024-11-21T22:55:01Z

...ambda/src/main/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessor.java

-                    "Lambda response payload is null or empty, dropping the original events");
-            return responseStrategy.handleEvents(parsedEvents, originalRecords);
+        // Considering "null" payload as empty response from lambda and not parsing it.
+        if (!("null".equals(payload.asUtf8String()))) {


Can you explain the "null" string situation?

Like then Lambda response is a plain one word "null" then we are considering it as empty response and not trying to parse it. It is basically like no response from lambda so we are going to drop all the records in the case of AggregateMode but fail in the case of StrictMode

Is this a Lambda response or a response from the Lambda function?

This is lambda response.

Let's make this a constant with a useful name.

e.g.

private static final NO_RETURN_RESPONSE = "null"

dlvenable · 2024-11-21T22:57:16Z

...a/src/test/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessorTest.java

-        assertThrows(RuntimeException.class, () -> localLambdaProcessor.convertLambdaResponseToEvent(buffer, invokeResponse),
-                "For Strict mode request and response size from lambda should match");
-
+        if (null != expectedException) {


It's better not to have conditionals in tests. This can be flaky and hard to understand.

Can we split this into two tests with two different ArgumentsSources methods/providers?

Oh in this case, it is basically like, is there an exception to assert or no. The check is only based on the arguments but not based on the logic. I think we do have such pattern else where.

I think it is a pattern best avoided.

But, we can keep for now.

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

dlvenable · 2024-11-22T00:05:15Z

...egrationTest/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessorIT.java

-        lambdaRegion = System.getProperty("tests.lambda.processor.region");
-        functionName = System.getProperty("tests.lambda.processor.functionName");
-        role = System.getProperty("tests.lambda.processor.sts_role_arn");
+//        lambdaRegion = System.getProperty("tests.lambda.processor.region");


I think you meant to revert this comment. This will not work in our CI.

dlvenable · 2024-11-22T00:05:42Z

...ambda/src/main/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessor.java

-                    "Lambda response payload is null or empty, dropping the original events");
-            return responseStrategy.handleEvents(parsedEvents, originalRecords);
+        // Considering "null" payload as empty response from lambda and not parsing it.
+        if (!("null".equals(payload.asUtf8String()))) {


Is this a Lambda response or a response from the Lambda function?

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

…-response-fix

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

…-response-fix

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

dlvenable · 2024-11-22T01:09:24Z

...a/src/test/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessorTest.java

+                arguments("lambda-processor-success-config.yaml", thirdSample, "random string",thirdSample, true),
+                arguments("lambda-processor-success-config.yaml", fourthSample, SdkBytes.fromByteArray("[]".getBytes()), fourthSample, true),
+                arguments("lambda-processor-success-config.yaml", fifthSample, SdkBytes.fromByteArray(fifthSampleJsonString.getBytes()), fifthSample, false)/*,
+                arguments("lambda-processor-success-config.yaml", getSampleEventRecords(1), SdkBytes.fromByteArray("[{\"key\":\"val\"}, {\"key\":\"val\"}]".getBytes()),Collections.emptyList()),


Please remove commented out code

dlvenable · 2024-11-22T01:09:47Z

...a/src/test/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessorTest.java

-        assertThrows(RuntimeException.class, () -> localLambdaProcessor.convertLambdaResponseToEvent(buffer, invokeResponse),
-                "For Strict mode request and response size from lambda should match");
-
+        if (null != expectedException) {


I think it is a pattern best avoided.

...a/src/test/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessorTest.java

dlvenable · 2024-11-22T01:10:04Z

...a/src/test/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessorTest.java

@@ -73,6 +97,8 @@
 import static org.opensearch.dataprepper.plugins.lambda.utils.LambdaTestSetupUtil.getSampleEventRecords;
 import static org.opensearch.dataprepper.plugins.lambda.utils.LambdaTestSetupUtil.getSampleRecord;

+=======


Merge conflicts

dlvenable · 2024-11-22T01:11:44Z

...ambda/src/main/java/org/opensearch/dataprepper/plugins/lambda/processor/LambdaProcessor.java

-                    "Lambda response payload is null or empty, dropping the original events");
-            return responseStrategy.handleEvents(parsedEvents, originalRecords);
+        // Considering "null" payload as empty response from lambda and not parsing it.
+        if (!("null".equals(payload.asUtf8String()))) {


Let's make this a constant with a useful name.

e.g.

private static final NO_RETURN_RESPONSE = "null"

dlvenable · 2024-11-22T01:15:46Z

...org/opensearch/dataprepper/plugins/lambda/processor/StrictResponseEventHandlingStrategy.java

-            throw new RuntimeException(
-                    "Response Processing Mode is configured as Strict mode but behavior is aggregate mode. Event count mismatch.");
+            throw new StrictResponseModeNotRespectedException(
+                    "Response Processing Mode is configured as Strict mode but behavior is aggregate mode. " +


"The aws_lambda processor is configured with response_events_match set to true. The Lambda function responded with a different number of events. Either set response_events_match to false or investigate your Lambda function to ensure that it returns the same number of events and provided as input. parsedEvents size = . Original events size = "

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

san81 added 4 commits November 21, 2024 12:50

Fix to better handle lambda responses when they are empty or null or …

c1023b3

…not a valid json Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

UTs for strict mode response comparison

705ee78

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

Additional UTs for strict mode and aggregate mode

928f5f3

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

removed unused method and better method naming

f496648

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

dlvenable requested changes Nov 21, 2024

View reviewed changes

san81 and others added 3 commits November 21, 2024 15:28

doExecute method testing

c85e196

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

better exception message

e3d2d61

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

Add IT for aggregate mode cases

422fd64

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

dlvenable reviewed Nov 22, 2024

View reviewed changes

san81 and others added 6 commits November 21, 2024 16:24

Testing with presence of tags

a328a28

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

Merge remote-tracking branch 'origin/lambda-response-fix' into lambda…

4d3872c

…-response-fix

Add IT to test behaviour for different lambda responses

158f2ce

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

removed unused imports

065ef32

Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com>

Merge remote-tracking branch 'origin/lambda-response-fix' into lambda…

066168a

…-response-fix

fix checkstyle

e7f0cea

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

san81 marked this pull request as ready for review November 22, 2024 01:05

san81 requested review from sb2k16, chenqi0805, engechas, graytaylor0, dinujoh, KarstenSchnitter and oeyh as code owners November 22, 2024 01:05

fix checkstyle

88edadf

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

dlvenable requested changes Nov 22, 2024

View reviewed changes

Address comments

97d5155

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

dlvenable approved these changes Nov 22, 2024

View reviewed changes

kkondaka approved these changes Nov 22, 2024

View reviewed changes

kkondaka merged commit 539d599 into opensearch-project:main Nov 22, 2024
46 of 47 checks passed

dlvenable mentioned this pull request Jan 14, 2025

Welcoming Santhosh Gandhe (san81) to the Data Prepper maintainers. #5329

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to better handle lambda responses when they are empty or null or not a valid json #5211

Fix to better handle lambda responses when they are empty or null or not a valid json #5211

san81 commented Nov 21, 2024

dlvenable Nov 21, 2024

kkondaka Nov 22, 2024

dlvenable Nov 21, 2024

san81 Nov 21, 2024

dlvenable Nov 22, 2024

kkondaka Nov 22, 2024

dlvenable Nov 22, 2024

dlvenable Nov 21, 2024

san81 Nov 21, 2024

dlvenable Nov 22, 2024

dlvenable Nov 22, 2024

dlvenable Nov 22, 2024

dlvenable Nov 22, 2024

dlvenable Nov 22, 2024

kkondaka Nov 22, 2024

dlvenable Nov 22, 2024

dlvenable Nov 22, 2024

dlvenable Nov 22, 2024

dlvenable Nov 22, 2024

Fix to better handle lambda responses when they are empty or null or not a valid json #5211

Fix to better handle lambda responses when they are empty or null or not a valid json #5211

Conversation

san81 commented Nov 21, 2024

Description

Issues Resolved

Check List

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