From a186c508354f198fbe21d60905d18ba99b122633 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 4 Jul 2024 16:33:00 +0700 Subject: [PATCH 01/38] [feat] Add CEL evaluator and add it to grpc sink --- build.gradle | 7 ++- .../firehose/config/GrpcSinkConfig.java | 4 ++ .../evaluator/CELPayloadEvaluator.java | 46 +++++++++++++++++++ .../firehose/evaluator/PayloadEvaluator.java | 5 ++ .../firehose/sink/grpc/GrpcSink.java | 28 ++++++++--- .../firehose/sink/grpc/GrpcSinkFactory.java | 2 +- 6 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 src/main/java/com/gotocompany/firehose/evaluator/CELPayloadEvaluator.java create mode 100644 src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java diff --git a/build.gradle b/build.gradle index ad2892f84..c4a192312 100644 --- a/build.gradle +++ b/build.gradle @@ -103,6 +103,9 @@ dependencies { implementation 'org.apache.logging.log4j:log4j-core:2.20.0' implementation group: 'com.gotocompany', name: 'depot', version: '0.9.1' implementation group: 'com.networknt', name: 'json-schema-validator', version: '1.0.59' exclude group: 'org.slf4j' + implementation 'dev.cel:cel:0.5.2' + implementation(enforcedPlatform("org.projectnessie.cel:cel-bom:0.4.4")) + implementation("org.projectnessie.cel:cel-tools") testImplementation group: 'junit', name: 'junit', version: '4.11' testImplementation 'org.hamcrest:hamcrest-all:1.3' @@ -117,11 +120,11 @@ dependencies { protobuf { generatedFilesBaseDir = "$projectDir/src/generated" protoc { - artifact = "com.google.protobuf:protoc:3.1.0" + artifact = "com.google.protobuf:protoc:3.17.3" } plugins { grpc { - artifact = "io.grpc:protoc-gen-grpc-java:1.0.3" + artifact = "io.grpc:protoc-gen-grpc-java:1.60.1" } } generateProtoTasks { diff --git a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java index 38d58d4e9..da5286b1d 100644 --- a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java +++ b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java @@ -30,6 +30,10 @@ public interface GrpcSinkConfig extends AppConfig { @Config.Key("SINK_GRPC_ARG_DEADLINE_MS") Long getSinkGrpcArgDeadlineMS(); + @Config.Key("SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION") + @DefaultValue("GenericResponse.success == false") + String getSinkGrpcResponseRetryCELExpression(); + @Key("SINK_GRPC_METADATA") @DefaultValue("") @ConverterClass(GrpcMetadataConverter.class) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/CELPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/CELPayloadEvaluator.java new file mode 100644 index 000000000..f932a00a6 --- /dev/null +++ b/src/main/java/com/gotocompany/firehose/evaluator/CELPayloadEvaluator.java @@ -0,0 +1,46 @@ +package com.gotocompany.firehose.evaluator; + +import com.google.protobuf.Descriptors; +import com.google.protobuf.DynamicMessage; +import com.google.protobuf.Message; +import lombok.extern.slf4j.Slf4j; +import org.projectnessie.cel.checker.Decls; +import org.projectnessie.cel.tools.Script; +import org.projectnessie.cel.tools.ScriptCreateException; +import org.projectnessie.cel.tools.ScriptException; +import org.projectnessie.cel.tools.ScriptHost; + +import java.util.HashMap; +import java.util.Map; + +@Slf4j +public class CELPayloadEvaluator implements PayloadEvaluator { + + private final Script script; + + public CELPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { + try { + script = ScriptHost.newBuilder() + .build() + .buildScript(celExpression) + .withDeclarations(Decls.newVar(descriptor.getFullName(), Decls.newObjectType(descriptor.getFullName()))) + .withTypes(DynamicMessage.newBuilder(descriptor).getDefaultInstanceForType()) + .build(); + } catch (ScriptCreateException e) { + throw new IllegalArgumentException("Failed to build CEL Script with reason: " + e.getMessage(), e); + } + } + + @Override + public boolean evaluate(Message payload) { + try { + Map arguments = new HashMap<>(); + arguments.put(payload.getDescriptorForType().getFullName(), payload); + return script.execute(Boolean.class, arguments); + } catch (ScriptException e) { + throw new IllegalArgumentException( + "Failed to evaluate payload with CEL Expression with reason: " + e.getMessage(), e); + } + } + +} diff --git a/src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java new file mode 100644 index 000000000..e558b5705 --- /dev/null +++ b/src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java @@ -0,0 +1,5 @@ +package com.gotocompany.firehose.evaluator; + +public interface PayloadEvaluator { + boolean evaluate(T payload); +} diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index c08312844..a881ddbb0 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -1,7 +1,11 @@ package com.gotocompany.firehose.sink.grpc; - +import com.gotocompany.depot.error.ErrorInfo; +import com.gotocompany.depot.error.ErrorType; +import com.gotocompany.firehose.config.GrpcSinkConfig; +import com.gotocompany.firehose.evaluator.CELPayloadEvaluator; +import com.gotocompany.firehose.exception.DefaultException; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; import com.gotocompany.firehose.metrics.FirehoseInstrumentation; @@ -9,6 +13,7 @@ import com.gotocompany.firehose.sink.grpc.client.GrpcClient; import com.google.protobuf.DynamicMessage; import com.gotocompany.stencil.client.StencilClient; +import org.apache.commons.lang3.StringUtils; import java.io.IOException; import java.util.ArrayList; @@ -21,13 +26,24 @@ public class GrpcSink extends AbstractSink { private final GrpcClient grpcClient; + private final StencilClient stencilClient; + private final GrpcSinkConfig grpcSinkConfig; private List messages; - private StencilClient stencilClient; + private CELPayloadEvaluator retryEvaluator; - public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, GrpcClient grpcClient, StencilClient stencilClient) { + public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, + GrpcClient grpcClient, + StencilClient stencilClient, + GrpcSinkConfig grpcSinkConfig) { super(firehoseInstrumentation, "grpc"); this.grpcClient = grpcClient; this.stencilClient = stencilClient; + this.grpcSinkConfig = grpcSinkConfig; + if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { + this.retryEvaluator = new CELPayloadEvaluator( + stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), + grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); + } } @Override @@ -38,11 +54,9 @@ protected List execute() throws Exception { DynamicMessage response = grpcClient.execute(message.getLogMessage(), message.getHeaders()); getFirehoseInstrumentation().logDebug("Response: {}", response); Object m = response.getField(response.getDescriptorForType().findFieldByName("success")); - boolean success = (m != null) ? Boolean.valueOf(String.valueOf(m)) : false; - if (!success) { - getFirehoseInstrumentation().logWarn("Grpc Service returned error"); - failedMessages.add(message); + if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()) && retryEvaluator.evaluate(response)) { + message.setErrorInfo(new ErrorInfo(new DefaultException("DEFAULT"), ErrorType.SINK_RETRYABLE_ERROR)); } } getFirehoseInstrumentation().logDebug("Failed messages count: {}", failedMessages.size()); diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java index a405a5f57..163968168 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java @@ -38,7 +38,7 @@ public static AbstractSink create(Map configuration, StatsDRepor GrpcClient grpcClient = new GrpcClient(new FirehoseInstrumentation(statsDReporter, GrpcClient.class), grpcConfig, managedChannel, stencilClient); firehoseInstrumentation.logInfo("GRPC connection established"); - return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient); + return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcConfig); } } From c9a6ac02b182bb0283106b29b1f5e6527d9ee831 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 4 Jul 2024 16:36:14 +0700 Subject: [PATCH 02/38] Remove blank check --- src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index a881ddbb0..b492a3986 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -55,7 +55,7 @@ protected List execute() throws Exception { getFirehoseInstrumentation().logDebug("Response: {}", response); Object m = response.getField(response.getDescriptorForType().findFieldByName("success")); - if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()) && retryEvaluator.evaluate(response)) { + if (retryEvaluator.evaluate(response)) { message.setErrorInfo(new ErrorInfo(new DefaultException("DEFAULT"), ErrorType.SINK_RETRYABLE_ERROR)); } } From 0cbe4fa9bee04a8b70d6a910358f7ee5987cf8d4 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 4 Jul 2024 16:37:51 +0700 Subject: [PATCH 03/38] remove unintended change --- src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index b492a3986..81d3a6546 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -53,10 +53,10 @@ protected List execute() throws Exception { for (Message message : this.messages) { DynamicMessage response = grpcClient.execute(message.getLogMessage(), message.getHeaders()); getFirehoseInstrumentation().logDebug("Response: {}", response); - Object m = response.getField(response.getDescriptorForType().findFieldByName("success")); if (retryEvaluator.evaluate(response)) { message.setErrorInfo(new ErrorInfo(new DefaultException("DEFAULT"), ErrorType.SINK_RETRYABLE_ERROR)); + failedMessages.add(message); } } getFirehoseInstrumentation().logDebug("Failed messages count: {}", failedMessages.size()); From 2b46bbb5d2d35e03683aa4a08cb4c59d869a6fe2 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 5 Jul 2024 10:29:53 +0700 Subject: [PATCH 04/38] [feat] add success field checking --- .../java/com/gotocompany/firehose/sink/grpc/GrpcSink.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index 81d3a6546..7489fb3de 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -53,10 +53,15 @@ protected List execute() throws Exception { for (Message message : this.messages) { DynamicMessage response = grpcClient.execute(message.getLogMessage(), message.getHeaders()); getFirehoseInstrumentation().logDebug("Response: {}", response); + Object m = response.getField(response.getDescriptorForType().findFieldByName("success")); + boolean success = (m != null) ? Boolean.valueOf(String.valueOf(m)) : false; + if (!success) { + getFirehoseInstrumentation().logWarn("Grpc Service returned error"); + failedMessages.add(message); + } if (retryEvaluator.evaluate(response)) { message.setErrorInfo(new ErrorInfo(new DefaultException("DEFAULT"), ErrorType.SINK_RETRYABLE_ERROR)); - failedMessages.add(message); } } getFirehoseInstrumentation().logDebug("Failed messages count: {}", failedMessages.size()); From 7cf9caf0d9f9d5e551ab36748f7ee48d7322dc28 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 5 Jul 2024 11:27:09 +0700 Subject: [PATCH 05/38] [feat] add evaluator method --- .../gotocompany/firehose/sink/grpc/GrpcSink.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index 7489fb3de..65e799b05 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -60,8 +60,8 @@ protected List execute() throws Exception { getFirehoseInstrumentation().logWarn("Grpc Service returned error"); failedMessages.add(message); } - if (retryEvaluator.evaluate(response)) { - message.setErrorInfo(new ErrorInfo(new DefaultException("DEFAULT"), ErrorType.SINK_RETRYABLE_ERROR)); + if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { + setRetryEvaluatorErrorInfo(message, response); } } getFirehoseInstrumentation().logDebug("Failed messages count: {}", failedMessages.size()); @@ -79,4 +79,13 @@ public void close() throws IOException { this.messages = new ArrayList<>(); stencilClient.close(); } + + private void setRetryEvaluatorErrorInfo(Message message, DynamicMessage dynamicMessage) { + boolean eligibleToRetry = retryEvaluator.evaluate(dynamicMessage); + if (eligibleToRetry) { + message.setErrorInfo(new ErrorInfo(new DefaultException("Retryable gRPC Error"), ErrorType.SINK_RETRYABLE_ERROR)); + return; + } + message.setErrorInfo(new ErrorInfo(new DefaultException("Non Retryable gRPC Error"), ErrorType.SINK_NON_RETRYABLE_ERROR)); + } } From 75a5b4ada52871b3690154c5f1675a849c8ec0ae Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 5 Jul 2024 12:15:33 +0700 Subject: [PATCH 06/38] [feat] handle descriptor update --- ...a => GRPCResponseCELPayloadEvaluator.java} | 45 +++++++++++++------ .../firehose/sink/grpc/GrpcSink.java | 6 +-- 2 files changed, 34 insertions(+), 17 deletions(-) rename src/main/java/com/gotocompany/firehose/evaluator/{CELPayloadEvaluator.java => GRPCResponseCELPayloadEvaluator.java} (55%) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/CELPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GRPCResponseCELPayloadEvaluator.java similarity index 55% rename from src/main/java/com/gotocompany/firehose/evaluator/CELPayloadEvaluator.java rename to src/main/java/com/gotocompany/firehose/evaluator/GRPCResponseCELPayloadEvaluator.java index f932a00a6..f67a39f55 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/CELPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GRPCResponseCELPayloadEvaluator.java @@ -14,21 +14,16 @@ import java.util.Map; @Slf4j -public class CELPayloadEvaluator implements PayloadEvaluator { +public class GRPCResponseCELPayloadEvaluator implements PayloadEvaluator { - private final Script script; + private final String celExpression; + private Script script; + private Descriptors.Descriptor descriptor; - public CELPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { - try { - script = ScriptHost.newBuilder() - .build() - .buildScript(celExpression) - .withDeclarations(Decls.newVar(descriptor.getFullName(), Decls.newObjectType(descriptor.getFullName()))) - .withTypes(DynamicMessage.newBuilder(descriptor).getDefaultInstanceForType()) - .build(); - } catch (ScriptCreateException e) { - throw new IllegalArgumentException("Failed to build CEL Script with reason: " + e.getMessage(), e); - } + public GRPCResponseCELPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { + this.celExpression = celExpression; + this.descriptor = descriptor; + this.script = buildScript(descriptor); } @Override @@ -36,11 +31,33 @@ public boolean evaluate(Message payload) { try { Map arguments = new HashMap<>(); arguments.put(payload.getDescriptorForType().getFullName(), payload); - return script.execute(Boolean.class, arguments); + return getScript(payload.getDescriptorForType()).execute(Boolean.class, arguments); } catch (ScriptException e) { throw new IllegalArgumentException( "Failed to evaluate payload with CEL Expression with reason: " + e.getMessage(), e); } } + private Script getScript(Descriptors.Descriptor descriptor) throws ScriptCreateException { + if (!descriptor.equals(this.descriptor)) { + synchronized (this) { + this.script = buildScript(descriptor); + this.descriptor = descriptor; + } + } + return this.script; + } + + private Script buildScript(Descriptors.Descriptor descriptor) { + try { + return ScriptHost.newBuilder() + .build() + .buildScript(this.celExpression) + .withDeclarations(Decls.newVar(descriptor.getFullName(), Decls.newObjectType(descriptor.getFullName()))) + .withTypes(DynamicMessage.newBuilder(descriptor).getDefaultInstanceForType()) + .build(); + } catch (ScriptCreateException e) { + throw new IllegalArgumentException("Failed to build CEL Script due to : " + e.getMessage(), e); + } + } } diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index 65e799b05..62047d643 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -4,7 +4,7 @@ import com.gotocompany.depot.error.ErrorInfo; import com.gotocompany.depot.error.ErrorType; import com.gotocompany.firehose.config.GrpcSinkConfig; -import com.gotocompany.firehose.evaluator.CELPayloadEvaluator; +import com.gotocompany.firehose.evaluator.GRPCResponseCELPayloadEvaluator; import com.gotocompany.firehose.exception.DefaultException; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; @@ -29,7 +29,7 @@ public class GrpcSink extends AbstractSink { private final StencilClient stencilClient; private final GrpcSinkConfig grpcSinkConfig; private List messages; - private CELPayloadEvaluator retryEvaluator; + private GRPCResponseCELPayloadEvaluator retryEvaluator; public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, GrpcClient grpcClient, @@ -40,7 +40,7 @@ public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, this.stencilClient = stencilClient; this.grpcSinkConfig = grpcSinkConfig; if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - this.retryEvaluator = new CELPayloadEvaluator( + this.retryEvaluator = new GRPCResponseCELPayloadEvaluator( stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); } From a0a9e7b5166b0dca834c68dfbe2a893d20af1fee Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 5 Jul 2024 16:17:44 +0700 Subject: [PATCH 07/38] Add test for evaluator --- build.gradle | 1 - ...a => GrpcResponseCELPayloadEvaluator.java} | 11 ++- .../firehose/sink/grpc/GrpcSink.java | 6 +- .../GrpcResponseCELPayloadEvaluatorTest.java | 94 +++++++++++++++++++ src/test/proto/GenericResponse.proto | 17 ++++ 5 files changed, 121 insertions(+), 8 deletions(-) rename src/main/java/com/gotocompany/firehose/evaluator/{GRPCResponseCELPayloadEvaluator.java => GrpcResponseCELPayloadEvaluator.java} (85%) create mode 100644 src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java create mode 100644 src/test/proto/GenericResponse.proto diff --git a/build.gradle b/build.gradle index c4a192312..abf18aeb3 100644 --- a/build.gradle +++ b/build.gradle @@ -103,7 +103,6 @@ dependencies { implementation 'org.apache.logging.log4j:log4j-core:2.20.0' implementation group: 'com.gotocompany', name: 'depot', version: '0.9.1' implementation group: 'com.networknt', name: 'json-schema-validator', version: '1.0.59' exclude group: 'org.slf4j' - implementation 'dev.cel:cel:0.5.2' implementation(enforcedPlatform("org.projectnessie.cel:cel-bom:0.4.4")) implementation("org.projectnessie.cel:cel-tools") diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GRPCResponseCELPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java similarity index 85% rename from src/main/java/com/gotocompany/firehose/evaluator/GRPCResponseCELPayloadEvaluator.java rename to src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java index f67a39f55..1a09f813d 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GRPCResponseCELPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java @@ -14,13 +14,13 @@ import java.util.Map; @Slf4j -public class GRPCResponseCELPayloadEvaluator implements PayloadEvaluator { +public class GrpcResponseCELPayloadEvaluator implements PayloadEvaluator { private final String celExpression; private Script script; private Descriptors.Descriptor descriptor; - public GRPCResponseCELPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { + public GrpcResponseCELPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { this.celExpression = celExpression; this.descriptor = descriptor; this.script = buildScript(descriptor); @@ -41,8 +41,10 @@ public boolean evaluate(Message payload) { private Script getScript(Descriptors.Descriptor descriptor) throws ScriptCreateException { if (!descriptor.equals(this.descriptor)) { synchronized (this) { - this.script = buildScript(descriptor); - this.descriptor = descriptor; + if (!descriptor.equals(this.descriptor)) { + this.script = buildScript(descriptor); + this.descriptor = descriptor; + } } } return this.script; @@ -50,6 +52,7 @@ private Script getScript(Descriptors.Descriptor descriptor) throws ScriptCreateE private Script buildScript(Descriptors.Descriptor descriptor) { try { + log.info("Building new CEL Script"); return ScriptHost.newBuilder() .build() .buildScript(this.celExpression) diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index 62047d643..47dc17865 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -4,7 +4,7 @@ import com.gotocompany.depot.error.ErrorInfo; import com.gotocompany.depot.error.ErrorType; import com.gotocompany.firehose.config.GrpcSinkConfig; -import com.gotocompany.firehose.evaluator.GRPCResponseCELPayloadEvaluator; +import com.gotocompany.firehose.evaluator.GrpcResponseCELPayloadEvaluator; import com.gotocompany.firehose.exception.DefaultException; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; @@ -29,7 +29,7 @@ public class GrpcSink extends AbstractSink { private final StencilClient stencilClient; private final GrpcSinkConfig grpcSinkConfig; private List messages; - private GRPCResponseCELPayloadEvaluator retryEvaluator; + private GrpcResponseCELPayloadEvaluator retryEvaluator; public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, GrpcClient grpcClient, @@ -40,7 +40,7 @@ public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, this.stencilClient = stencilClient; this.grpcSinkConfig = grpcSinkConfig; if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - this.retryEvaluator = new GRPCResponseCELPayloadEvaluator( + this.retryEvaluator = new GrpcResponseCELPayloadEvaluator( stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); } diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java new file mode 100644 index 000000000..773a3c2c0 --- /dev/null +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java @@ -0,0 +1,94 @@ +package com.gotocompany.firehose.evaluator; + +import com.google.protobuf.DescriptorProtos; +import com.google.protobuf.Descriptors; +import com.google.protobuf.DynamicMessage; +import com.google.protobuf.Message; +import com.gotocompany.firehose.consumer.GenericError; +import com.gotocompany.firehose.consumer.GenericResponse; +import org.junit.Before; +import org.junit.Test; +import org.junit.jupiter.api.Assertions; + +public class GrpcResponseCELPayloadEvaluatorTest { + + private static final String CEL_EXPRESSION = "GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"400\")"; + private PayloadEvaluator grpcPayloadEvaluator; + + @Before + public void setup() { + this.grpcPayloadEvaluator = new GrpcResponseCELPayloadEvaluator(GenericResponse.getDescriptor(), CEL_EXPRESSION); + } + + @Test + public void shouldEvaluateResponseToTrueWhenCELExpressionMatchesPayload() { + GenericResponse genericResponse = GenericResponse.newBuilder() + .setSuccess(false) + .setDetail("Detail Message") + .addErrors(GenericError.newBuilder() + .setCode("400") + .setEntity("GoFin") + .setCause("Unknown") + .build()) + .build(); + + boolean result = grpcPayloadEvaluator.evaluate(genericResponse); + + Assertions.assertTrue(result); + } + + @Test + public void shouldEvaluateResponseToFalseWhenCELExpressionDoesntMatchPayload() { + GenericResponse genericResponse = GenericResponse.newBuilder() + .setSuccess(false) + .setDetail("Detail Message") + .addErrors(GenericError.newBuilder() + .setCode("50000") + .setEntity("GoFin") + .setCause("Unknown") + .build()) + .build(); + + boolean result = grpcPayloadEvaluator.evaluate(genericResponse); + + Assertions.assertFalse(result); + } + + @Test + public void shouldEvaluateResponseWhenDescriptorUpdated() throws Descriptors.DescriptorValidationException { + Descriptors.Descriptor baseDescriptor = GenericResponse.getDescriptor(); + DescriptorProtos.FieldDescriptorProto newFieldProto = DescriptorProtos.FieldDescriptorProto.newBuilder() + .setName("new_field") + .setNumber(4) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_STRING) + .build(); + DescriptorProtos.DescriptorProto newDescriptorProto = baseDescriptor.toProto().toBuilder() + .addField(newFieldProto) + .build(); + Descriptors.FileDescriptor newFileDescriptor = Descriptors.FileDescriptor.buildFrom(DescriptorProtos.FileDescriptorProto + .newBuilder() + .setName("new.proto") + .addMessageType(newDescriptorProto) + .addMessageType(GenericError.getDescriptor().toProto()) + .build(), new Descriptors.FileDescriptor[]{}); + Descriptors.Descriptor genericResponseDescriptor = newFileDescriptor.findMessageTypeByName("GenericResponse"); + DynamicMessage dynamicMessage = DynamicMessage.newBuilder(genericResponseDescriptor) + .setField(genericResponseDescriptor.findFieldByName("success"), false) + .setField(genericResponseDescriptor.findFieldByName("new_field"), "new_field") + .build(); + + boolean result = grpcPayloadEvaluator.evaluate(dynamicMessage); + + Assertions.assertFalse(result); + } + + @Test + public void shouldThrowIllegalArgumentExceptionWhenPayloadNotMatchingDescriptor() { + Assertions.assertThrows(IllegalArgumentException.class, + () -> grpcPayloadEvaluator.evaluate(GenericError.newBuilder() + .setCause("Unknown") + .setCode("500") + .setEntity("GoFin") + .build())); + } +} diff --git a/src/test/proto/GenericResponse.proto b/src/test/proto/GenericResponse.proto new file mode 100644 index 000000000..dc16cd30c --- /dev/null +++ b/src/test/proto/GenericResponse.proto @@ -0,0 +1,17 @@ +syntax = "proto3"; + +option java_multiple_files = true; +option java_package = "com.gotocompany.firehose.consumer"; +option java_outer_classname = "SampleGenericResponse"; + +message GenericResponse { + bool success = 1; + repeated GenericError errors = 2; + string detail = 3; +} + +message GenericError { + string code = 1; + string entity = 2; + string cause = 3; +} From 9c0eafa8943dbec23117ba83b95f4734a5d11b70 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 5 Jul 2024 16:34:20 +0700 Subject: [PATCH 08/38] Update test --- .../gotocompany/firehose/sink/grpc/GrpcSinkTest.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index 0e66daa2a..881bdc571 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -1,5 +1,6 @@ package com.gotocompany.firehose.sink.grpc; +import com.gotocompany.firehose.config.GrpcSinkConfig; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; import com.gotocompany.firehose.metrics.FirehoseInstrumentation; @@ -40,10 +41,13 @@ public class GrpcSinkTest { @Mock private FirehoseInstrumentation firehoseInstrumentation; + @Mock + private GrpcSinkConfig grpcSinkConfig; + @Before public void setUp() { initMocks(this); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); } @Test @@ -85,7 +89,7 @@ public void shouldReturnBackListOfFailedMessages() throws IOException, Deseriali @Test public void shouldCloseStencilClient() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); sink.close(); verify(stencilClient, times(1)).close(); @@ -93,7 +97,7 @@ public void shouldCloseStencilClient() throws IOException { @Test public void shouldLogWhenClosingConnection() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); sink.close(); verify(firehoseInstrumentation, times(1)).logInfo("GRPC connection closing"); From 9c56c64c568efca8b13d995a99dd07b97e8c2d58 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 5 Jul 2024 16:38:13 +0700 Subject: [PATCH 09/38] Update SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION default value --- .../java/com/gotocompany/firehose/config/GrpcSinkConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java index da5286b1d..bda2bc286 100644 --- a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java +++ b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java @@ -31,7 +31,7 @@ public interface GrpcSinkConfig extends AppConfig { Long getSinkGrpcArgDeadlineMS(); @Config.Key("SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION") - @DefaultValue("GenericResponse.success == false") + @DefaultValue("") String getSinkGrpcResponseRetryCELExpression(); @Key("SINK_GRPC_METADATA") From 764d0dde916270f085ee3757e9cfcb0f25e929d8 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Sun, 7 Jul 2024 18:02:19 +0700 Subject: [PATCH 10/38] Add test for GrpcSink --- .../firehose/sink/grpc/GrpcSinkTest.java | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index 881bdc571..3afb239e6 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -1,6 +1,9 @@ package com.gotocompany.firehose.sink.grpc; +import com.google.protobuf.InvalidProtocolBufferException; import com.gotocompany.firehose.config.GrpcSinkConfig; +import com.gotocompany.firehose.consumer.GenericError; +import com.gotocompany.firehose.consumer.GenericResponse; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; import com.gotocompany.firehose.metrics.FirehoseInstrumentation; @@ -13,7 +16,9 @@ import org.apache.kafka.common.header.internals.RecordHeaders; import org.junit.Before; import org.junit.Test; +import org.junit.jupiter.api.Assertions; import org.mockito.Mock; +import org.mockito.Mockito; import java.io.IOException; import java.util.Collections; @@ -102,4 +107,64 @@ public void shouldLogWhenClosingConnection() throws IOException { sink.close(); verify(firehoseInstrumentation, times(1)).logInfo("GRPC connection closing"); } + + @Test + public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatches() throws InvalidProtocolBufferException { + Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); + GenericResponse response = GenericResponse.newBuilder() + .setSuccess(false) + .setDetail("detail") + .addErrors(GenericError.newBuilder() + .setCode("4000") + .setCause("cause") + .setEntity("gtf") + .build()) + .build(); + DynamicMessage dynamicMessage = DynamicMessage.parseFrom( + response.getDescriptorForType(), + response.toByteArray() + ); + when(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()) + .thenReturn("GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"4000\")"); + when(grpcClient.execute(any(), any())) + .thenReturn(dynamicMessage); + when(stencilClient.get(any())) + .thenReturn(GenericResponse.getDescriptor()); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); + + List result = sink.pushMessage(Collections.singletonList(payload)); + + Assertions.assertEquals(1, result.size()); + Assertions.assertEquals(result.get(0).getErrorInfo().getErrorType(), ErrorType.SINK_RETRYABLE_ERROR); + } + + @Test + public void shouldReturnFailedMessagesWithNonRetryableErrorsWhenCELExpressionDoesntMatch() throws InvalidProtocolBufferException { + Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); + GenericResponse response = GenericResponse.newBuilder() + .setSuccess(false) + .setDetail("detail") + .addErrors(GenericError.newBuilder() + .setCode("4000") + .setCause("cause") + .setEntity("gtf") + .build()) + .build(); + DynamicMessage dynamicMessage = DynamicMessage.parseFrom( + response.getDescriptorForType(), + response.toByteArray() + ); + when(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()) + .thenReturn("GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"5000\")"); + when(grpcClient.execute(any(), any())) + .thenReturn(dynamicMessage); + when(stencilClient.get(any())) + .thenReturn(GenericResponse.getDescriptor()); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); + + List result = sink.pushMessage(Collections.singletonList(payload)); + + Assertions.assertEquals(1, result.size()); + Assertions.assertEquals(result.get(0).getErrorInfo().getErrorType(), ErrorType.SINK_NON_RETRYABLE_ERROR); + } } From 91ebabc55825d8a281bda5a898a178236429683c Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Sun, 7 Jul 2024 18:11:32 +0700 Subject: [PATCH 11/38] Rename descriptor to payloadDescriptor --- .../GrpcResponseCELPayloadEvaluator.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java index 1a09f813d..17b2f54c0 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java @@ -38,26 +38,26 @@ public boolean evaluate(Message payload) { } } - private Script getScript(Descriptors.Descriptor descriptor) throws ScriptCreateException { - if (!descriptor.equals(this.descriptor)) { + private Script getScript(Descriptors.Descriptor payloadDescriptor) throws ScriptCreateException { + if (!payloadDescriptor.equals(this.descriptor)) { synchronized (this) { - if (!descriptor.equals(this.descriptor)) { - this.script = buildScript(descriptor); - this.descriptor = descriptor; + if (!payloadDescriptor.equals(this.descriptor)) { + this.script = buildScript(payloadDescriptor); + this.descriptor = payloadDescriptor; } } } return this.script; } - private Script buildScript(Descriptors.Descriptor descriptor) { + private Script buildScript(Descriptors.Descriptor payloadDescriptor) { try { log.info("Building new CEL Script"); return ScriptHost.newBuilder() .build() .buildScript(this.celExpression) - .withDeclarations(Decls.newVar(descriptor.getFullName(), Decls.newObjectType(descriptor.getFullName()))) - .withTypes(DynamicMessage.newBuilder(descriptor).getDefaultInstanceForType()) + .withDeclarations(Decls.newVar(payloadDescriptor.getFullName(), Decls.newObjectType(payloadDescriptor.getFullName()))) + .withTypes(DynamicMessage.newBuilder(payloadDescriptor).getDefaultInstanceForType()) .build(); } catch (ScriptCreateException e) { throw new IllegalArgumentException("Failed to build CEL Script due to : " + e.getMessage(), e); From 2b4c9843fd10232ac3c956fca912b8711f9b473c Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Sun, 7 Jul 2024 18:12:13 +0700 Subject: [PATCH 12/38] Checkstyle update --- .../java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index 3afb239e6..f9fcc2d07 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -18,7 +18,6 @@ import org.junit.Test; import org.junit.jupiter.api.Assertions; import org.mockito.Mock; -import org.mockito.Mockito; import java.io.IOException; import java.util.Collections; From 927ccc39198c074c4967cad162f934b2bfc63e18 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 8 Jul 2024 13:28:51 +0700 Subject: [PATCH 13/38] Refactor instantiation logic to separate method --- .../firehose/sink/grpc/GrpcSink.java | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index 47dc17865..0091b471a 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -39,11 +39,7 @@ public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, this.grpcClient = grpcClient; this.stencilClient = stencilClient; this.grpcSinkConfig = grpcSinkConfig; - if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - this.retryEvaluator = new GrpcResponseCELPayloadEvaluator( - stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), - grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); - } + instantiateRetryEvaluator(); } @Override @@ -59,9 +55,9 @@ protected List execute() throws Exception { if (!success) { getFirehoseInstrumentation().logWarn("Grpc Service returned error"); failedMessages.add(message); - } - if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - setRetryEvaluatorErrorInfo(message, response); + if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { + setRetryableErrorInfo(message, response); + } } } getFirehoseInstrumentation().logDebug("Failed messages count: {}", failedMessages.size()); @@ -80,7 +76,15 @@ public void close() throws IOException { stencilClient.close(); } - private void setRetryEvaluatorErrorInfo(Message message, DynamicMessage dynamicMessage) { + private void instantiateRetryEvaluator() { + if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { + this.retryEvaluator = new GrpcResponseCELPayloadEvaluator( + stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), + grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); + } + } + + private void setRetryableErrorInfo(Message message, DynamicMessage dynamicMessage) { boolean eligibleToRetry = retryEvaluator.evaluate(dynamicMessage); if (eligibleToRetry) { message.setErrorInfo(new ErrorInfo(new DefaultException("Retryable gRPC Error"), ErrorType.SINK_RETRYABLE_ERROR)); From b350110d08660a5c6dfd925bb7313f58ec40ce6c Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 8 Jul 2024 16:42:29 +0700 Subject: [PATCH 14/38] Remove schema refresh --- .../GrpcResponseCELPayloadEvaluator.java | 16 +--------- .../GrpcResponseCELPayloadEvaluatorTest.java | 31 ------------------- 2 files changed, 1 insertion(+), 46 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java index 17b2f54c0..94f101ed2 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java @@ -18,11 +18,9 @@ public class GrpcResponseCELPayloadEvaluator implements PayloadEvaluator arguments = new HashMap<>(); arguments.put(payload.getDescriptorForType().getFullName(), payload); - return getScript(payload.getDescriptorForType()).execute(Boolean.class, arguments); + return this.script.execute(Boolean.class, arguments); } catch (ScriptException e) { throw new IllegalArgumentException( "Failed to evaluate payload with CEL Expression with reason: " + e.getMessage(), e); } } - private Script getScript(Descriptors.Descriptor payloadDescriptor) throws ScriptCreateException { - if (!payloadDescriptor.equals(this.descriptor)) { - synchronized (this) { - if (!payloadDescriptor.equals(this.descriptor)) { - this.script = buildScript(payloadDescriptor); - this.descriptor = payloadDescriptor; - } - } - } - return this.script; - } - private Script buildScript(Descriptors.Descriptor payloadDescriptor) { try { log.info("Building new CEL Script"); diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java index 773a3c2c0..cfa839ec7 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java @@ -1,8 +1,5 @@ package com.gotocompany.firehose.evaluator; -import com.google.protobuf.DescriptorProtos; -import com.google.protobuf.Descriptors; -import com.google.protobuf.DynamicMessage; import com.google.protobuf.Message; import com.gotocompany.firehose.consumer.GenericError; import com.gotocompany.firehose.consumer.GenericResponse; @@ -54,34 +51,6 @@ public void shouldEvaluateResponseToFalseWhenCELExpressionDoesntMatchPayload() { Assertions.assertFalse(result); } - @Test - public void shouldEvaluateResponseWhenDescriptorUpdated() throws Descriptors.DescriptorValidationException { - Descriptors.Descriptor baseDescriptor = GenericResponse.getDescriptor(); - DescriptorProtos.FieldDescriptorProto newFieldProto = DescriptorProtos.FieldDescriptorProto.newBuilder() - .setName("new_field") - .setNumber(4) - .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_STRING) - .build(); - DescriptorProtos.DescriptorProto newDescriptorProto = baseDescriptor.toProto().toBuilder() - .addField(newFieldProto) - .build(); - Descriptors.FileDescriptor newFileDescriptor = Descriptors.FileDescriptor.buildFrom(DescriptorProtos.FileDescriptorProto - .newBuilder() - .setName("new.proto") - .addMessageType(newDescriptorProto) - .addMessageType(GenericError.getDescriptor().toProto()) - .build(), new Descriptors.FileDescriptor[]{}); - Descriptors.Descriptor genericResponseDescriptor = newFileDescriptor.findMessageTypeByName("GenericResponse"); - DynamicMessage dynamicMessage = DynamicMessage.newBuilder(genericResponseDescriptor) - .setField(genericResponseDescriptor.findFieldByName("success"), false) - .setField(genericResponseDescriptor.findFieldByName("new_field"), "new_field") - .build(); - - boolean result = grpcPayloadEvaluator.evaluate(dynamicMessage); - - Assertions.assertFalse(result); - } - @Test public void shouldThrowIllegalArgumentExceptionWhenPayloadNotMatchingDescriptor() { Assertions.assertThrows(IllegalArgumentException.class, From 8485e9d056a6ab262424c9274460312d0d960779 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 8 Jul 2024 21:31:18 +0700 Subject: [PATCH 15/38] Remove projectnessie and use implementation from cel-java --- build.gradle | 3 +- .../GrpcResponseCELPayloadEvaluator.java | 60 +++++++++++-------- .../GrpcResponseCELPayloadEvaluatorTest.java | 10 +--- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/build.gradle b/build.gradle index abf18aeb3..56f04593c 100644 --- a/build.gradle +++ b/build.gradle @@ -103,8 +103,7 @@ dependencies { implementation 'org.apache.logging.log4j:log4j-core:2.20.0' implementation group: 'com.gotocompany', name: 'depot', version: '0.9.1' implementation group: 'com.networknt', name: 'json-schema-validator', version: '1.0.59' exclude group: 'org.slf4j' - implementation(enforcedPlatform("org.projectnessie.cel:cel-bom:0.4.4")) - implementation("org.projectnessie.cel:cel-tools") + implementation 'dev.cel:cel:0.5.2' testImplementation group: 'junit', name: 'junit', version: '4.11' testImplementation 'org.hamcrest:hamcrest-all:1.3' diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java index 94f101ed2..749ec8cdd 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java @@ -1,52 +1,60 @@ package com.gotocompany.firehose.evaluator; import com.google.protobuf.Descriptors; -import com.google.protobuf.DynamicMessage; import com.google.protobuf.Message; +import com.gotocompany.firehose.exception.DeserializerException; +import dev.cel.common.CelAbstractSyntaxTree; +import dev.cel.common.CelValidationException; +import dev.cel.common.types.StructTypeReference; +import dev.cel.compiler.CelCompiler; +import dev.cel.compiler.CelCompilerFactory; +import dev.cel.parser.CelStandardMacro; +import dev.cel.runtime.CelEvaluationException; +import dev.cel.runtime.CelRuntime; +import dev.cel.runtime.CelRuntimeFactory; import lombok.extern.slf4j.Slf4j; -import org.projectnessie.cel.checker.Decls; -import org.projectnessie.cel.tools.Script; -import org.projectnessie.cel.tools.ScriptCreateException; -import org.projectnessie.cel.tools.ScriptException; -import org.projectnessie.cel.tools.ScriptHost; import java.util.HashMap; import java.util.Map; @Slf4j -public class GrpcResponseCELPayloadEvaluator implements PayloadEvaluator { +public class GrpcResponseCELPayloadEvaluator implements PayloadEvaluator{ - private final String celExpression; - private Script script; + private CelRuntime.Program celProgram; public GrpcResponseCELPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { - this.celExpression = celExpression; - this.script = buildScript(descriptor); + buildCelEnvironment(descriptor, celExpression); } + @Override public boolean evaluate(Message payload) { try { - Map arguments = new HashMap<>(); - arguments.put(payload.getDescriptorForType().getFullName(), payload); - return this.script.execute(Boolean.class, arguments); - } catch (ScriptException e) { - throw new IllegalArgumentException( - "Failed to evaluate payload with CEL Expression with reason: " + e.getMessage(), e); + Map args = new HashMap<>(); + args.put(payload.getDescriptorForType().getFullName(), payload); + return (boolean) celProgram.eval(args); + } catch (CelEvaluationException e) { + throw new DeserializerException("Failed to evaluate payload", e); } } - private Script buildScript(Descriptors.Descriptor payloadDescriptor) { + private void buildCelEnvironment(Descriptors.Descriptor descriptor, String celExpression) { try { - log.info("Building new CEL Script"); - return ScriptHost.newBuilder() - .build() - .buildScript(this.celExpression) - .withDeclarations(Decls.newVar(payloadDescriptor.getFullName(), Decls.newObjectType(payloadDescriptor.getFullName()))) - .withTypes(DynamicMessage.newBuilder(payloadDescriptor).getDefaultInstanceForType()) + CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() + .setStandardMacros(CelStandardMacro.EXISTS) + .addVar(descriptor.getFullName(), StructTypeReference.create(descriptor.getFullName())) + .addMessageTypes(descriptor) + .build(); + CelRuntime celRuntime = CelRuntimeFactory.standardCelRuntimeBuilder() .build(); - } catch (ScriptCreateException e) { - throw new IllegalArgumentException("Failed to build CEL Script due to : " + e.getMessage(), e); + CelAbstractSyntaxTree celAbstractSyntaxTree = celCompiler.compile(celExpression) + .getAst(); + this.celProgram = celRuntime.createProgram(celAbstractSyntaxTree); + } catch (CelValidationException e) { + throw new IllegalArgumentException("Invalid CEL expression: " + celExpression, e); + } catch (CelEvaluationException e) { + throw new IllegalArgumentException("Failed to construct CEL evaluator: " + e.getMessage(), e); } } + } diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java index cfa839ec7..645878155 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java @@ -52,12 +52,8 @@ public void shouldEvaluateResponseToFalseWhenCELExpressionDoesntMatchPayload() { } @Test - public void shouldThrowIllegalArgumentExceptionWhenPayloadNotMatchingDescriptor() { - Assertions.assertThrows(IllegalArgumentException.class, - () -> grpcPayloadEvaluator.evaluate(GenericError.newBuilder() - .setCause("Unknown") - .setCode("500") - .setEntity("GoFin") - .build())); + public void construct_shouldThrowIllegalArgumentExceptionWhenCelValidationFailed() { + Assertions.assertThrows(IllegalArgumentException.class, () -> new GrpcResponseCELPayloadEvaluator( + GenericResponse.getDescriptor(), "GenericResponse.nonExistField == true")); } } From 949dab936eccdd4e532bde96f6cde708a0e75fbb Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 8 Jul 2024 21:33:34 +0700 Subject: [PATCH 16/38] update checkstyle --- ...valuator.java => GrpcResponseCelPayloadEvaluator.java} | 4 ++-- .../java/com/gotocompany/firehose/sink/grpc/GrpcSink.java | 6 +++--- ...Test.java => GrpcResponseCelPayloadEvaluatorTest.java} | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) rename src/main/java/com/gotocompany/firehose/evaluator/{GrpcResponseCELPayloadEvaluator.java => GrpcResponseCelPayloadEvaluator.java} (93%) rename src/test/java/com/gotocompany/firehose/evaluator/{GrpcResponseCELPayloadEvaluatorTest.java => GrpcResponseCelPayloadEvaluatorTest.java} (88%) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java similarity index 93% rename from src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java rename to src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java index 749ec8cdd..8d8bc5cfb 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java @@ -18,11 +18,11 @@ import java.util.Map; @Slf4j -public class GrpcResponseCELPayloadEvaluator implements PayloadEvaluator{ +public class GrpcResponseCelPayloadEvaluator implements PayloadEvaluator { private CelRuntime.Program celProgram; - public GrpcResponseCELPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { + public GrpcResponseCelPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { buildCelEnvironment(descriptor, celExpression); } diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index 0091b471a..db2f459de 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -4,7 +4,7 @@ import com.gotocompany.depot.error.ErrorInfo; import com.gotocompany.depot.error.ErrorType; import com.gotocompany.firehose.config.GrpcSinkConfig; -import com.gotocompany.firehose.evaluator.GrpcResponseCELPayloadEvaluator; +import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; import com.gotocompany.firehose.exception.DefaultException; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; @@ -29,7 +29,7 @@ public class GrpcSink extends AbstractSink { private final StencilClient stencilClient; private final GrpcSinkConfig grpcSinkConfig; private List messages; - private GrpcResponseCELPayloadEvaluator retryEvaluator; + private GrpcResponseCelPayloadEvaluator retryEvaluator; public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, GrpcClient grpcClient, @@ -78,7 +78,7 @@ public void close() throws IOException { private void instantiateRetryEvaluator() { if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - this.retryEvaluator = new GrpcResponseCELPayloadEvaluator( + this.retryEvaluator = new GrpcResponseCelPayloadEvaluator( stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); } diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java similarity index 88% rename from src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java rename to src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java index 645878155..c8a59be96 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCELPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java @@ -7,14 +7,14 @@ import org.junit.Test; import org.junit.jupiter.api.Assertions; -public class GrpcResponseCELPayloadEvaluatorTest { +public class GrpcResponseCelPayloadEvaluatorTest { private static final String CEL_EXPRESSION = "GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"400\")"; private PayloadEvaluator grpcPayloadEvaluator; @Before public void setup() { - this.grpcPayloadEvaluator = new GrpcResponseCELPayloadEvaluator(GenericResponse.getDescriptor(), CEL_EXPRESSION); + this.grpcPayloadEvaluator = new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), CEL_EXPRESSION); } @Test @@ -52,8 +52,8 @@ public void shouldEvaluateResponseToFalseWhenCELExpressionDoesntMatchPayload() { } @Test - public void construct_shouldThrowIllegalArgumentExceptionWhenCelValidationFailed() { - Assertions.assertThrows(IllegalArgumentException.class, () -> new GrpcResponseCELPayloadEvaluator( + public void shouldThrowIllegalArgumentExceptionWhenCelValidationFailed() { + Assertions.assertThrows(IllegalArgumentException.class, () -> new GrpcResponseCelPayloadEvaluator( GenericResponse.getDescriptor(), "GenericResponse.nonExistField == true")); } } From e4f4080cc982c30f02dc90e00e13191222540640 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 8 Jul 2024 21:43:02 +0700 Subject: [PATCH 17/38] Add comment --- .../GrpcResponseCelPayloadEvaluator.java | 24 ++++++++++++++++++- .../firehose/evaluator/PayloadEvaluator.java | 11 +++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java index 8d8bc5cfb..576f6328f 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java @@ -17,16 +17,31 @@ import java.util.HashMap; import java.util.Map; +/** + * Implementation of PayloadEvaluator that evaluates gRPC responses using CEL (Common Expression Language). + */ @Slf4j public class GrpcResponseCelPayloadEvaluator implements PayloadEvaluator { private CelRuntime.Program celProgram; + /** + * Constructs a GrpcResponseCelPayloadEvaluator with the specified descriptor and CEL expression. + * + * @param descriptor the descriptor of the gRPC message + * @param celExpression the CEL expression to evaluate against the message + */ public GrpcResponseCelPayloadEvaluator(Descriptors.Descriptor descriptor, String celExpression) { buildCelEnvironment(descriptor, celExpression); } - + /** + * Evaluates the given gRPC message payload using the CEL program. + * + * @param payload the gRPC message to be evaluated + * @return true if the payload passes the evaluation, false otherwise + * @throws DeserializerException if the evaluation fails + */ @Override public boolean evaluate(Message payload) { try { @@ -38,6 +53,13 @@ public boolean evaluate(Message payload) { } } + /** + * Builds the CEL environment required to evaluate the CEL expression. + * + * @param descriptor the descriptor of the gRPC message + * @param celExpression the CEL expression to evaluate against the message + * @throws IllegalArgumentException if the CEL expression is invalid or if the evaluator cannot be constructed + */ private void buildCelEnvironment(Descriptors.Descriptor descriptor, String celExpression) { try { CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() diff --git a/src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java index e558b5705..7b8ba480b 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/PayloadEvaluator.java @@ -1,5 +1,16 @@ package com.gotocompany.firehose.evaluator; +/** + * A generic interface for evaluating payloads. + * + * @param the type of payload to be evaluated + */ public interface PayloadEvaluator { + /** + * Evaluates the given payload. + * + * @param payload the payload to be evaluated + * @return true if the payload passes the evaluation, false otherwise + */ boolean evaluate(T payload); } From 70d3a5eab2428bc9d40587ffb3d8a5618ff00df1 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 8 Jul 2024 21:46:03 +0700 Subject: [PATCH 18/38] Update docs --- docs/docs/sinks/grpc-sink.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/docs/sinks/grpc-sink.md b/docs/docs/sinks/grpc-sink.md index 6830dff8a..a4b5f5584 100644 --- a/docs/docs/sinks/grpc-sink.md +++ b/docs/docs/sinks/grpc-sink.md @@ -65,3 +65,11 @@ Defines the amount of time (in milliseconds) gRPC clients are willing to wait fo - Example value: `1000` - Type: `optional` + +### `SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION` + +Defines the CEL expression used to evaluate whether gRPC sink call should be retried or not based on the gRPC response + +- Example value: `com.goto.entity.GrpcResponse.success == false && com.goto.entity.GrpcResponse.errors.exists(e, e.code == "4000")` +- Type: `optional` +- Default value: `` \ No newline at end of file From 44e3ee60c385f206f07c69810c9ee82041656f79 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 8 Jul 2024 22:55:29 +0700 Subject: [PATCH 19/38] Move the evaluator instantiation to factory method --- .../DefaultGrpcPayloadEvaluator.java | 10 +++++++ .../firehose/sink/grpc/GrpcSink.java | 22 ++++---------- .../firehose/sink/grpc/GrpcSinkFactory.java | 16 +++++++++- .../firehose/sink/grpc/GrpcSinkTest.java | 30 ++++++++++--------- 4 files changed, 47 insertions(+), 31 deletions(-) create mode 100644 src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java diff --git a/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java new file mode 100644 index 000000000..bc2ac7ad8 --- /dev/null +++ b/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java @@ -0,0 +1,10 @@ +package com.gotocompany.firehose.evaluator; + +import com.google.protobuf.Message; + +public class DefaultGrpcPayloadEvaluator implements PayloadEvaluator { + @Override + public boolean evaluate(Message payload) { + return false; + } +} diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index db2f459de..c8063f4aa 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -4,7 +4,7 @@ import com.gotocompany.depot.error.ErrorInfo; import com.gotocompany.depot.error.ErrorType; import com.gotocompany.firehose.config.GrpcSinkConfig; -import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; +import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.exception.DefaultException; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; @@ -13,7 +13,6 @@ import com.gotocompany.firehose.sink.grpc.client.GrpcClient; import com.google.protobuf.DynamicMessage; import com.gotocompany.stencil.client.StencilClient; -import org.apache.commons.lang3.StringUtils; import java.io.IOException; import java.util.ArrayList; @@ -29,17 +28,18 @@ public class GrpcSink extends AbstractSink { private final StencilClient stencilClient; private final GrpcSinkConfig grpcSinkConfig; private List messages; - private GrpcResponseCelPayloadEvaluator retryEvaluator; + private PayloadEvaluator retryEvaluator; public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, GrpcClient grpcClient, StencilClient stencilClient, - GrpcSinkConfig grpcSinkConfig) { + GrpcSinkConfig grpcSinkConfig, + PayloadEvaluator retryEvaluator) { super(firehoseInstrumentation, "grpc"); this.grpcClient = grpcClient; this.stencilClient = stencilClient; this.grpcSinkConfig = grpcSinkConfig; - instantiateRetryEvaluator(); + this.retryEvaluator = retryEvaluator; } @Override @@ -55,9 +55,7 @@ protected List execute() throws Exception { if (!success) { getFirehoseInstrumentation().logWarn("Grpc Service returned error"); failedMessages.add(message); - if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - setRetryableErrorInfo(message, response); - } + setRetryableErrorInfo(message, response); } } getFirehoseInstrumentation().logDebug("Failed messages count: {}", failedMessages.size()); @@ -76,14 +74,6 @@ public void close() throws IOException { stencilClient.close(); } - private void instantiateRetryEvaluator() { - if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - this.retryEvaluator = new GrpcResponseCelPayloadEvaluator( - stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), - grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); - } - } - private void setRetryableErrorInfo(Message message, DynamicMessage dynamicMessage) { boolean eligibleToRetry = retryEvaluator.evaluate(dynamicMessage); if (eligibleToRetry) { diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java index 163968168..304fe9f5e 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java @@ -1,7 +1,11 @@ package com.gotocompany.firehose.sink.grpc; +import com.google.protobuf.Message; import com.gotocompany.firehose.config.GrpcSinkConfig; +import com.gotocompany.firehose.evaluator.DefaultGrpcPayloadEvaluator; +import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; +import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.metrics.FirehoseInstrumentation; import com.gotocompany.firehose.sink.grpc.client.GrpcClient; import com.gotocompany.depot.metrics.StatsDReporter; @@ -10,6 +14,7 @@ import io.grpc.ManagedChannelBuilder; import com.gotocompany.stencil.client.StencilClient; import org.aeonbits.owner.ConfigFactory; +import org.apache.commons.lang3.StringUtils; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -37,8 +42,17 @@ public static AbstractSink create(Map configuration, StatsDRepor GrpcClient grpcClient = new GrpcClient(new FirehoseInstrumentation(statsDReporter, GrpcClient.class), grpcConfig, managedChannel, stencilClient); firehoseInstrumentation.logInfo("GRPC connection established"); + PayloadEvaluator grpcResponseRetryEvaluator = instantiateRetryEvaluator(grpcConfig, stencilClient); + return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcConfig, grpcResponseRetryEvaluator); + } - return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcConfig); + private static PayloadEvaluator instantiateRetryEvaluator(GrpcSinkConfig grpcSinkConfig, StencilClient stencilClient) { + if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { + return new GrpcResponseCelPayloadEvaluator( + stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), + grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); + } + return new DefaultGrpcPayloadEvaluator(); } } diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index f9fcc2d07..d5279e039 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -4,6 +4,9 @@ import com.gotocompany.firehose.config.GrpcSinkConfig; import com.gotocompany.firehose.consumer.GenericError; import com.gotocompany.firehose.consumer.GenericResponse; +import com.gotocompany.firehose.evaluator.DefaultGrpcPayloadEvaluator; +import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; +import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.exception.DeserializerException; import com.gotocompany.firehose.message.Message; import com.gotocompany.firehose.metrics.FirehoseInstrumentation; @@ -48,10 +51,15 @@ public class GrpcSinkTest { @Mock private GrpcSinkConfig grpcSinkConfig; + private PayloadEvaluator grpcResponsePayloadEvaluator; @Before public void setUp() { initMocks(this); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); + this.grpcResponsePayloadEvaluator = new GrpcResponseCelPayloadEvaluator( + GenericResponse.getDescriptor(), + "GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"4000\")" + ); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, new DefaultGrpcPayloadEvaluator()); } @Test @@ -93,7 +101,7 @@ public void shouldReturnBackListOfFailedMessages() throws IOException, Deseriali @Test public void shouldCloseStencilClient() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); sink.close(); verify(stencilClient, times(1)).close(); @@ -101,7 +109,7 @@ public void shouldCloseStencilClient() throws IOException { @Test public void shouldLogWhenClosingConnection() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); sink.close(); verify(firehoseInstrumentation, times(1)).logInfo("GRPC connection closing"); @@ -109,6 +117,7 @@ public void shouldLogWhenClosingConnection() throws IOException { @Test public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatches() throws InvalidProtocolBufferException { + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); GenericResponse response = GenericResponse.newBuilder() .setSuccess(false) @@ -123,13 +132,9 @@ public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatche response.getDescriptorForType(), response.toByteArray() ); - when(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()) - .thenReturn("GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"4000\")"); when(grpcClient.execute(any(), any())) .thenReturn(dynamicMessage); - when(stencilClient.get(any())) - .thenReturn(GenericResponse.getDescriptor()); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, grpcResponsePayloadEvaluator); List result = sink.pushMessage(Collections.singletonList(payload)); @@ -139,12 +144,13 @@ public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatche @Test public void shouldReturnFailedMessagesWithNonRetryableErrorsWhenCELExpressionDoesntMatch() throws InvalidProtocolBufferException { + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); GenericResponse response = GenericResponse.newBuilder() .setSuccess(false) .setDetail("detail") .addErrors(GenericError.newBuilder() - .setCode("4000") + .setCode("not-exist-code") .setCause("cause") .setEntity("gtf") .build()) @@ -153,13 +159,9 @@ public void shouldReturnFailedMessagesWithNonRetryableErrorsWhenCELExpressionDoe response.getDescriptorForType(), response.toByteArray() ); - when(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()) - .thenReturn("GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"5000\")"); when(grpcClient.execute(any(), any())) .thenReturn(dynamicMessage); - when(stencilClient.get(any())) - .thenReturn(GenericResponse.getDescriptor()); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, grpcResponsePayloadEvaluator); List result = sink.pushMessage(Collections.singletonList(payload)); From 188d258a142e64e79948f16dabba473f5b737ab7 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Tue, 9 Jul 2024 09:03:48 +0700 Subject: [PATCH 20/38] Remove unused sink config --- .../gotocompany/firehose/sink/grpc/GrpcSink.java | 3 --- .../firehose/sink/grpc/GrpcSinkFactory.java | 2 +- .../firehose/sink/grpc/GrpcSinkTest.java | 14 +++++++------- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index c8063f4aa..bf166e24b 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -26,19 +26,16 @@ public class GrpcSink extends AbstractSink { private final GrpcClient grpcClient; private final StencilClient stencilClient; - private final GrpcSinkConfig grpcSinkConfig; private List messages; private PayloadEvaluator retryEvaluator; public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, GrpcClient grpcClient, StencilClient stencilClient, - GrpcSinkConfig grpcSinkConfig, PayloadEvaluator retryEvaluator) { super(firehoseInstrumentation, "grpc"); this.grpcClient = grpcClient; this.stencilClient = stencilClient; - this.grpcSinkConfig = grpcSinkConfig; this.retryEvaluator = retryEvaluator; } diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java index 304fe9f5e..9e6bfdf52 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java @@ -43,7 +43,7 @@ public static AbstractSink create(Map configuration, StatsDRepor GrpcClient grpcClient = new GrpcClient(new FirehoseInstrumentation(statsDReporter, GrpcClient.class), grpcConfig, managedChannel, stencilClient); firehoseInstrumentation.logInfo("GRPC connection established"); PayloadEvaluator grpcResponseRetryEvaluator = instantiateRetryEvaluator(grpcConfig, stencilClient); - return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcConfig, grpcResponseRetryEvaluator); + return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcResponseRetryEvaluator); } private static PayloadEvaluator instantiateRetryEvaluator(GrpcSinkConfig grpcSinkConfig, StencilClient stencilClient) { diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index d5279e039..79af1fced 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -59,7 +59,7 @@ public void setUp() { GenericResponse.getDescriptor(), "GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"4000\")" ); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, new DefaultGrpcPayloadEvaluator()); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, new DefaultGrpcPayloadEvaluator()); } @Test @@ -101,7 +101,7 @@ public void shouldReturnBackListOfFailedMessages() throws IOException, Deseriali @Test public void shouldCloseStencilClient() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); sink.close(); verify(stencilClient, times(1)).close(); @@ -109,7 +109,7 @@ public void shouldCloseStencilClient() throws IOException { @Test public void shouldLogWhenClosingConnection() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); sink.close(); verify(firehoseInstrumentation, times(1)).logInfo("GRPC connection closing"); @@ -117,7 +117,7 @@ public void shouldLogWhenClosingConnection() throws IOException { @Test public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatches() throws InvalidProtocolBufferException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); GenericResponse response = GenericResponse.newBuilder() .setSuccess(false) @@ -134,7 +134,7 @@ public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatche ); when(grpcClient.execute(any(), any())) .thenReturn(dynamicMessage); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcResponsePayloadEvaluator); List result = sink.pushMessage(Collections.singletonList(payload)); @@ -144,7 +144,7 @@ public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatche @Test public void shouldReturnFailedMessagesWithNonRetryableErrorsWhenCELExpressionDoesntMatch() throws InvalidProtocolBufferException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); GenericResponse response = GenericResponse.newBuilder() .setSuccess(false) @@ -161,7 +161,7 @@ public void shouldReturnFailedMessagesWithNonRetryableErrorsWhenCELExpressionDoe ); when(grpcClient.execute(any(), any())) .thenReturn(dynamicMessage); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcResponsePayloadEvaluator); List result = sink.pushMessage(Collections.singletonList(payload)); From 3c33f10782060a0a1a7bffc88a7f6159b295fe3c Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Tue, 9 Jul 2024 09:39:15 +0700 Subject: [PATCH 21/38] Add more testcases --- .../GrpcResponseCelPayloadEvaluator.java | 16 ++++++++++------ .../firehose/sink/grpc/GrpcSink.java | 1 - .../GrpcResponseCelPayloadEvaluatorTest.java | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java index 576f6328f..68034ee13 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java @@ -23,6 +23,7 @@ @Slf4j public class GrpcResponseCelPayloadEvaluator implements PayloadEvaluator { + private final Descriptors.Descriptor descriptor; private CelRuntime.Program celProgram; /** @@ -32,7 +33,8 @@ public class GrpcResponseCelPayloadEvaluator implements PayloadEvaluator args = new HashMap<>(); args.put(payload.getDescriptorForType().getFullName(), payload); @@ -56,16 +61,15 @@ public boolean evaluate(Message payload) { /** * Builds the CEL environment required to evaluate the CEL expression. * - * @param descriptor the descriptor of the gRPC message * @param celExpression the CEL expression to evaluate against the message * @throws IllegalArgumentException if the CEL expression is invalid or if the evaluator cannot be constructed */ - private void buildCelEnvironment(Descriptors.Descriptor descriptor, String celExpression) { + private void buildCelEnvironment(String celExpression) { try { CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() - .setStandardMacros(CelStandardMacro.EXISTS) - .addVar(descriptor.getFullName(), StructTypeReference.create(descriptor.getFullName())) - .addMessageTypes(descriptor) + .setStandardMacros(CelStandardMacro.EXISTS, CelStandardMacro.EXISTS_ONE, CelStandardMacro.HAS) + .addVar(this.descriptor.getFullName(), StructTypeReference.create(this.descriptor.getFullName())) + .addMessageTypes(this.descriptor) .build(); CelRuntime celRuntime = CelRuntimeFactory.standardCelRuntimeBuilder() .build(); diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index bf166e24b..002365d83 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -3,7 +3,6 @@ import com.gotocompany.depot.error.ErrorInfo; import com.gotocompany.depot.error.ErrorType; -import com.gotocompany.firehose.config.GrpcSinkConfig; import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.exception.DefaultException; import com.gotocompany.firehose.exception.DeserializerException; diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java index c8a59be96..f141b90e3 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java @@ -3,6 +3,7 @@ import com.google.protobuf.Message; import com.gotocompany.firehose.consumer.GenericError; import com.gotocompany.firehose.consumer.GenericResponse; +import com.gotocompany.firehose.consumer.TestMessage; import org.junit.Before; import org.junit.Test; import org.junit.jupiter.api.Assertions; @@ -56,4 +57,22 @@ public void shouldThrowIllegalArgumentExceptionWhenCelValidationFailed() { Assertions.assertThrows(IllegalArgumentException.class, () -> new GrpcResponseCelPayloadEvaluator( GenericResponse.getDescriptor(), "GenericResponse.nonExistField == true")); } + + @Test + public void shouldThrowIllegalArgumentExceptionWhenPayloadIsNotRecognizedByDescriptor() { + TestMessage unregisteredPayload = TestMessage.newBuilder() + .setOrderUrl("url") + .build(); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> grpcPayloadEvaluator.evaluate(unregisteredPayload)); + } + + @Test + public void shouldThrowIllegalArgumentExceptionWhenCelExpressionContainsUnregisteredMacro() { + String expressionWithUnregisteredMacro = "GenericResponse.errors.filter(e, e.code == \"400\")"; + + Assertions.assertThrows(IllegalArgumentException.class, + () -> new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), expressionWithUnregisteredMacro)); + } } From 9f994e7ecf3438aa869330ce535c77f9a085ba36 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Tue, 9 Jul 2024 10:32:14 +0700 Subject: [PATCH 22/38] revert protoc version --- build.gradle | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build.gradle b/build.gradle index 56f04593c..3e0c09ff2 100644 --- a/build.gradle +++ b/build.gradle @@ -118,11 +118,11 @@ dependencies { protobuf { generatedFilesBaseDir = "$projectDir/src/generated" protoc { - artifact = "com.google.protobuf:protoc:3.17.3" + artifact = "com.google.protobuf:protoc:3.1.0" } plugins { grpc { - artifact = "io.grpc:protoc-gen-grpc-java:1.60.1" + artifact = "io.grpc:protoc-gen-grpc-java:1.0.3" } } generateProtoTasks { From 61bc69398d4fed07e7d485284901c848913f70a8 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Tue, 9 Jul 2024 14:25:30 +0700 Subject: [PATCH 23/38] Add more test cases --- .../DefaultGrpcPayloadEvaluator.java | 2 +- .../GrpcResponseCelPayloadEvaluator.java | 2 +- .../firehose/sink/grpc/GrpcSink.java | 1 + .../GrpcResponseCelPayloadEvaluatorTest.java | 45 ++++++++++++++++++- .../firehose/sink/grpc/GrpcSinkTest.java | 3 +- 5 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java index bc2ac7ad8..f436dde35 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java @@ -5,6 +5,6 @@ public class DefaultGrpcPayloadEvaluator implements PayloadEvaluator { @Override public boolean evaluate(Message payload) { - return false; + return true; } } diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java index 68034ee13..7291f4871 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java @@ -67,7 +67,7 @@ public boolean evaluate(Message payload) { private void buildCelEnvironment(String celExpression) { try { CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() - .setStandardMacros(CelStandardMacro.EXISTS, CelStandardMacro.EXISTS_ONE, CelStandardMacro.HAS) + .setStandardMacros(CelStandardMacro.values()) .addVar(this.descriptor.getFullName(), StructTypeReference.create(this.descriptor.getFullName())) .addMessageTypes(this.descriptor) .build(); diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index 002365d83..aa88e57a3 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -73,6 +73,7 @@ public void close() throws IOException { private void setRetryableErrorInfo(Message message, DynamicMessage dynamicMessage) { boolean eligibleToRetry = retryEvaluator.evaluate(dynamicMessage); if (eligibleToRetry) { + getFirehoseInstrumentation().logDebug("Retrying grpc service"); message.setErrorInfo(new ErrorInfo(new DefaultException("Retryable gRPC Error"), ErrorType.SINK_RETRYABLE_ERROR)); return; } diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java index f141b90e3..2a1ab67ae 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java @@ -52,6 +52,48 @@ public void shouldEvaluateResponseToFalseWhenCELExpressionDoesntMatchPayload() { Assertions.assertFalse(result); } + @Test + public void shouldEvaluateResponseToTrueWhenCelExpressionMatchesRange() { + PayloadEvaluator evaluator = new GrpcResponseCelPayloadEvaluator( + GenericResponse.getDescriptor(), + "GenericResponse.errors.exists(e, int(e.code) >= 1000 && int(e.code) <= 2000)" + ); + GenericResponse genericResponse = GenericResponse.newBuilder() + .setSuccess(false) + .setDetail("Detail Message") + .addErrors(GenericError.newBuilder() + .setCode("1500") + .setEntity("GoFin") + .setCause("Unknown") + .build()) + .build(); + + boolean result = evaluator.evaluate(genericResponse); + + Assertions.assertTrue(result); + } + + @Test + public void shouldEvaluateResponseToFalseWhenCelExpressionMatchesRangeAndNotInSet() { + PayloadEvaluator evaluator = new GrpcResponseCelPayloadEvaluator( + GenericResponse.getDescriptor(), + "GenericResponse.errors.exists(e, int(e.code) >= 1000 && int(e.code) <= 2000 && !(int(e.code) in [1500, 1600]))" + ); + GenericResponse genericResponse = GenericResponse.newBuilder() + .setSuccess(false) + .setDetail("Detail Message") + .addErrors(GenericError.newBuilder() + .setCode("1500") + .setEntity("GoFin") + .setCause("Unknown") + .build()) + .build(); + + boolean result = evaluator.evaluate(genericResponse); + + Assertions.assertFalse(result); + } + @Test public void shouldThrowIllegalArgumentExceptionWhenCelValidationFailed() { Assertions.assertThrows(IllegalArgumentException.class, () -> new GrpcResponseCelPayloadEvaluator( @@ -70,9 +112,10 @@ public void shouldThrowIllegalArgumentExceptionWhenPayloadIsNotRecognizedByDescr @Test public void shouldThrowIllegalArgumentExceptionWhenCelExpressionContainsUnregisteredMacro() { - String expressionWithUnregisteredMacro = "GenericResponse.errors.filter(e, e.code == \"400\")"; + String expressionWithUnregisteredMacro = "GenericResponse.errors.nonStandardMacro(e, e.code == \"400\")"; Assertions.assertThrows(IllegalArgumentException.class, () -> new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), expressionWithUnregisteredMacro)); } + } diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index 79af1fced..fd0fa7a3b 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -88,15 +88,16 @@ public void shouldReturnBackListOfFailedMessages() throws IOException, Deseriali TestGrpcResponse build = TestGrpcResponse.newBuilder().setSuccess(false).build(); DynamicMessage response = DynamicMessage.parseFrom(build.getDescriptorForType(), build.toByteArray()); when(grpcClient.execute(any(), any(RecordHeaders.class))).thenReturn(response); + List failedMessages = sink.pushMessage(Collections.singletonList(message)); assertFalse(failedMessages.isEmpty()); assertEquals(1, failedMessages.size()); - verify(firehoseInstrumentation, times(1)).logInfo("Preparing {} messages", 1); verify(firehoseInstrumentation, times(1)).logDebug("Response: {}", response); verify(firehoseInstrumentation, times(1)).logWarn("Grpc Service returned error"); verify(firehoseInstrumentation, times(1)).logDebug("Failed messages count: {}", 1); + verify(firehoseInstrumentation, times(1)).logDebug("Retrying grpc service"); } @Test From cea966e3cb6c42d84b6f9dc7a967d150c0508f4a Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Tue, 9 Jul 2024 14:42:04 +0700 Subject: [PATCH 24/38] Add more comprehensive documentation --- docs/docs/sinks/grpc-sink.md | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/docs/docs/sinks/grpc-sink.md b/docs/docs/sinks/grpc-sink.md index a4b5f5584..5a98d2043 100644 --- a/docs/docs/sinks/grpc-sink.md +++ b/docs/docs/sinks/grpc-sink.md @@ -68,8 +68,31 @@ Defines the amount of time (in milliseconds) gRPC clients are willing to wait fo ### `SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION` -Defines the CEL expression used to evaluate whether gRPC sink call should be retried or not based on the gRPC response +Defines the CEL(Common Expression Language) expression used to evaluate whether gRPC sink call should be retried or not based on the gRPC response. +Currently, we support all standard CEL macro including: has, all, exists, exists_one, map, map_filter, filter +For more information about CEL please refer to this documentation : https://github.com/google/cel-spec/blob/master/doc/langdef.md - Example value: `com.goto.entity.GrpcResponse.success == false && com.goto.entity.GrpcResponse.errors.exists(e, e.code == "4000")` - Type: `optional` -- Default value: `` \ No newline at end of file +- Default value: `` +- Use cases : + Example response proto : + ``` + syntax = "proto3"; + package com.gotocompany.generic; + + GenericResponse { + bool success = 1; + repeated Error errors = 2; + } + + Error { + string code = 1; + string reason = 2; + } + ``` + + Example retry rule : + - Retry on specific error code : `com.gotocompany.generic.GenericResponse.errors.exists(e, e.code == "400")` + - Retry on specific error code range : `com.gotocompany.generic.GenericResponse.errors.exists(e, int(e.code) >= 400 && int(e.code) <= 500)` + - Retry on error codes outside from specific error codes : `com.gotocompany.generic.GenericResponse.errors.exists(e, !(int(e.code) in [400, 500, 600]))` \ No newline at end of file From 08adb941b4c843a726e2f226fd5b61d8d17c4bc1 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Tue, 9 Jul 2024 16:53:05 +0700 Subject: [PATCH 25/38] Rename default class and update docs --- docs/docs/sinks/grpc-sink.md | 2 +- ...valuator.java => DefaultGrpcResponsePayloadEvaluator.java} | 2 +- .../com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java | 4 ++-- .../java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) rename src/main/java/com/gotocompany/firehose/evaluator/{DefaultGrpcPayloadEvaluator.java => DefaultGrpcResponsePayloadEvaluator.java} (66%) diff --git a/docs/docs/sinks/grpc-sink.md b/docs/docs/sinks/grpc-sink.md index 5a98d2043..c0fcae995 100644 --- a/docs/docs/sinks/grpc-sink.md +++ b/docs/docs/sinks/grpc-sink.md @@ -72,7 +72,7 @@ Defines the CEL(Common Expression Language) expression used to evaluate whether Currently, we support all standard CEL macro including: has, all, exists, exists_one, map, map_filter, filter For more information about CEL please refer to this documentation : https://github.com/google/cel-spec/blob/master/doc/langdef.md -- Example value: `com.goto.entity.GrpcResponse.success == false && com.goto.entity.GrpcResponse.errors.exists(e, e.code == "4000")` +- Example value: `com.gotocompany.generic.GrpcResponse.success == false && com.gotocompany.generic.GenericResponse.errors.exists(e, int(e.code) >= 400 && int(e.code) <= 500)` - Type: `optional` - Default value: `` - Use cases : diff --git a/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcResponsePayloadEvaluator.java similarity index 66% rename from src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java rename to src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcResponsePayloadEvaluator.java index f436dde35..673419f98 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcResponsePayloadEvaluator.java @@ -2,7 +2,7 @@ import com.google.protobuf.Message; -public class DefaultGrpcPayloadEvaluator implements PayloadEvaluator { +public class DefaultGrpcResponsePayloadEvaluator implements PayloadEvaluator { @Override public boolean evaluate(Message payload) { return true; diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java index 9e6bfdf52..2a87a6537 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java @@ -3,7 +3,7 @@ import com.google.protobuf.Message; import com.gotocompany.firehose.config.GrpcSinkConfig; -import com.gotocompany.firehose.evaluator.DefaultGrpcPayloadEvaluator; +import com.gotocompany.firehose.evaluator.DefaultGrpcResponsePayloadEvaluator; import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.metrics.FirehoseInstrumentation; @@ -52,7 +52,7 @@ private static PayloadEvaluator instantiateRetryEvaluator(GrpcSinkConfi stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); } - return new DefaultGrpcPayloadEvaluator(); + return new DefaultGrpcResponsePayloadEvaluator(); } } diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index fd0fa7a3b..048a1e44e 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -4,7 +4,7 @@ import com.gotocompany.firehose.config.GrpcSinkConfig; import com.gotocompany.firehose.consumer.GenericError; import com.gotocompany.firehose.consumer.GenericResponse; -import com.gotocompany.firehose.evaluator.DefaultGrpcPayloadEvaluator; +import com.gotocompany.firehose.evaluator.DefaultGrpcResponsePayloadEvaluator; import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.exception.DeserializerException; @@ -59,7 +59,7 @@ public void setUp() { GenericResponse.getDescriptor(), "GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"4000\")" ); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, new DefaultGrpcPayloadEvaluator()); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, new DefaultGrpcResponsePayloadEvaluator()); } @Test From a22d7abcee396b016e0e90d5dd6e4b3e2fdad02a Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 12 Jul 2024 11:29:53 +0700 Subject: [PATCH 26/38] Refactor typical cel functionality to util class --- .../GrpcResponseCelPayloadEvaluator.java | 42 ++---------- .../gotocompany/firehose/utils/CelUtils.java | 66 +++++++++++++++++++ 2 files changed, 73 insertions(+), 35 deletions(-) create mode 100644 src/main/java/com/gotocompany/firehose/utils/CelUtils.java diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java index 7291f4871..5196efc67 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java @@ -2,21 +2,12 @@ import com.google.protobuf.Descriptors; import com.google.protobuf.Message; -import com.gotocompany.firehose.exception.DeserializerException; -import dev.cel.common.CelAbstractSyntaxTree; -import dev.cel.common.CelValidationException; -import dev.cel.common.types.StructTypeReference; +import com.gotocompany.firehose.utils.CelUtils; import dev.cel.compiler.CelCompiler; -import dev.cel.compiler.CelCompilerFactory; -import dev.cel.parser.CelStandardMacro; -import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.CelRuntime; import dev.cel.runtime.CelRuntimeFactory; import lombok.extern.slf4j.Slf4j; -import java.util.HashMap; -import java.util.Map; - /** * Implementation of PayloadEvaluator that evaluates gRPC responses using CEL (Common Expression Language). */ @@ -42,20 +33,13 @@ public GrpcResponseCelPayloadEvaluator(Descriptors.Descriptor descriptor, String * * @param payload the gRPC message to be evaluated * @return true if the payload passes the evaluation, false otherwise - * @throws DeserializerException if the evaluation fails */ @Override public boolean evaluate(Message payload) { if (!descriptor.getFullName().equals(payload.getDescriptorForType().getFullName())) { throw new IllegalArgumentException("Payload does not match descriptor"); } - try { - Map args = new HashMap<>(); - args.put(payload.getDescriptorForType().getFullName(), payload); - return (boolean) celProgram.eval(args); - } catch (CelEvaluationException e) { - throw new DeserializerException("Failed to evaluate payload", e); - } + return (boolean) CelUtils.evaluate(this.celProgram, payload); } /** @@ -64,23 +48,11 @@ public boolean evaluate(Message payload) { * @param celExpression the CEL expression to evaluate against the message * @throws IllegalArgumentException if the CEL expression is invalid or if the evaluator cannot be constructed */ - private void buildCelEnvironment(String celExpression) { - try { - CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() - .setStandardMacros(CelStandardMacro.values()) - .addVar(this.descriptor.getFullName(), StructTypeReference.create(this.descriptor.getFullName())) - .addMessageTypes(this.descriptor) - .build(); - CelRuntime celRuntime = CelRuntimeFactory.standardCelRuntimeBuilder() - .build(); - CelAbstractSyntaxTree celAbstractSyntaxTree = celCompiler.compile(celExpression) - .getAst(); - this.celProgram = celRuntime.createProgram(celAbstractSyntaxTree); - } catch (CelValidationException e) { - throw new IllegalArgumentException("Invalid CEL expression: " + celExpression, e); - } catch (CelEvaluationException e) { - throw new IllegalArgumentException("Failed to construct CEL evaluator: " + e.getMessage(), e); - } + private void buildCelEnvironment(String celExpression) { + CelCompiler celCompiler = CelUtils.initializeCelCompiler(this.descriptor); + CelRuntime celRuntime = CelRuntimeFactory.standardCelRuntimeBuilder() + .build(); + this.celProgram = CelUtils.initializeCelProgram(celExpression, celRuntime, celCompiler); } } diff --git a/src/main/java/com/gotocompany/firehose/utils/CelUtils.java b/src/main/java/com/gotocompany/firehose/utils/CelUtils.java new file mode 100644 index 000000000..56975fc8b --- /dev/null +++ b/src/main/java/com/gotocompany/firehose/utils/CelUtils.java @@ -0,0 +1,66 @@ +package com.gotocompany.firehose.utils; + +import com.google.protobuf.Descriptors; +import com.google.protobuf.Message; +import dev.cel.common.CelAbstractSyntaxTree; +import dev.cel.common.CelValidationException; +import dev.cel.common.types.StructTypeReference; +import dev.cel.compiler.CelCompiler; +import dev.cel.compiler.CelCompilerFactory; +import dev.cel.parser.CelStandardMacro; +import dev.cel.runtime.CelEvaluationException; +import dev.cel.runtime.CelRuntime; +import org.aeonbits.owner.util.Collections; + +/** + * Utility class to instantiate CEL(Common Expression Language) related functionality. + * Official Documentation of CEL + */ +public class CelUtils { + + /** + * @param program the program to execute + * @param payload the proto payload to evaluated by program + * @return the dynamic value based on program execution of the payload + */ + public static Object evaluate(CelRuntime.Program program, Message payload) { + try { + return program.eval(Collections.map(payload.getDescriptorForType().getFullName(), payload)); + } catch (CelEvaluationException e) { + throw new IllegalArgumentException("Could not evaluate Cel expression", e); + } + } + + /** + * Initializes the CEL compiler with standard macros and message types. + * + * @return the initialized CEL compiler + */ + public static CelCompiler initializeCelCompiler(Descriptors.Descriptor descriptor) { + return CelCompilerFactory.standardCelCompilerBuilder() + .setStandardMacros(CelStandardMacro.values()) + .addVar(descriptor.getFullName(), StructTypeReference.create(descriptor.getFullName())) + .addMessageTypes(descriptor) + .build(); + } + + /** + * Initializes a CEL program for a given expression. + * + * @param celExpression the CEL expression to compile + * @param celRuntime the CEL runtime environment + * @param celCompiler the CEL compiler + * @return the compiled CEL program + * @throws IllegalArgumentException if the CEL program cannot be created + */ + public static CelRuntime.Program initializeCelProgram(String celExpression, CelRuntime celRuntime, CelCompiler celCompiler) { + try { + CelAbstractSyntaxTree celAbstractSyntaxTree = celCompiler.compile(celExpression) + .getAst(); + return celRuntime.createProgram(celAbstractSyntaxTree); + } catch (CelValidationException | CelEvaluationException e) { + throw new IllegalArgumentException("Failed to create CEL program with expression : " + celExpression, e); + } + } + +} From e963eba3033925dd65dcdc23e3a06018c8cf65f2 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Mon, 15 Jul 2024 09:59:23 +0700 Subject: [PATCH 27/38] Add checking for expression result --- .../GrpcResponseCelPayloadEvaluator.java | 6 ++++-- .../OperationNotSupportedException.java | 7 +++++++ .../gotocompany/firehose/utils/CelUtils.java | 20 +++++++++++++++---- .../GrpcResponseCelPayloadEvaluatorTest.java | 7 +++++++ 4 files changed, 34 insertions(+), 6 deletions(-) create mode 100644 src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java index 5196efc67..b9e78fdad 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java @@ -3,6 +3,7 @@ import com.google.protobuf.Descriptors; import com.google.protobuf.Message; import com.gotocompany.firehose.utils.CelUtils; +import dev.cel.common.types.CelKind; import dev.cel.compiler.CelCompiler; import dev.cel.runtime.CelRuntime; import dev.cel.runtime.CelRuntimeFactory; @@ -20,7 +21,7 @@ public class GrpcResponseCelPayloadEvaluator implements PayloadEvaluator celType.kind().equals(CelKind.BOOL)); } } diff --git a/src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java b/src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java new file mode 100644 index 000000000..ff961d975 --- /dev/null +++ b/src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java @@ -0,0 +1,7 @@ +package com.gotocompany.firehose.exception; + +public class OperationNotSupportedException extends RuntimeException { + public OperationNotSupportedException(String message) { + super(message); + } +} diff --git a/src/main/java/com/gotocompany/firehose/utils/CelUtils.java b/src/main/java/com/gotocompany/firehose/utils/CelUtils.java index 56975fc8b..3d9ed61dd 100644 --- a/src/main/java/com/gotocompany/firehose/utils/CelUtils.java +++ b/src/main/java/com/gotocompany/firehose/utils/CelUtils.java @@ -2,8 +2,10 @@ import com.google.protobuf.Descriptors; import com.google.protobuf.Message; +import com.gotocompany.firehose.exception.OperationNotSupportedException; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelValidationException; +import dev.cel.common.types.CelType; import dev.cel.common.types.StructTypeReference; import dev.cel.compiler.CelCompiler; import dev.cel.compiler.CelCompilerFactory; @@ -12,6 +14,8 @@ import dev.cel.runtime.CelRuntime; import org.aeonbits.owner.util.Collections; +import java.util.function.Predicate; + /** * Utility class to instantiate CEL(Common Expression Language) related functionality. * Official Documentation of CEL @@ -47,16 +51,24 @@ public static CelCompiler initializeCelCompiler(Descriptors.Descriptor descripto /** * Initializes a CEL program for a given expression. * - * @param celExpression the CEL expression to compile - * @param celRuntime the CEL runtime environment - * @param celCompiler the CEL compiler + * @param celExpression the CEL expression to compile + * @param celRuntime the CEL runtime environment + * @param celCompiler the CEL compiler + * @param resultTypeChecker the predicate to evaluate whether return type is supported or not * @return the compiled CEL program * @throws IllegalArgumentException if the CEL program cannot be created + * @throws OperationNotSupportedException if the return type is not supported */ - public static CelRuntime.Program initializeCelProgram(String celExpression, CelRuntime celRuntime, CelCompiler celCompiler) { + public static CelRuntime.Program initializeCelProgram(String celExpression, + CelRuntime celRuntime, + CelCompiler celCompiler, + Predicate resultTypeChecker) { try { CelAbstractSyntaxTree celAbstractSyntaxTree = celCompiler.compile(celExpression) .getAst(); + if (!resultTypeChecker.test(celAbstractSyntaxTree.getResultType())) { + throw new OperationNotSupportedException("Return type not supported for " + celExpression); + } return celRuntime.createProgram(celAbstractSyntaxTree); } catch (CelValidationException | CelEvaluationException e) { throw new IllegalArgumentException("Failed to create CEL program with expression : " + celExpression, e); diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java index 2a1ab67ae..b2f9eb13c 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java @@ -4,6 +4,7 @@ import com.gotocompany.firehose.consumer.GenericError; import com.gotocompany.firehose.consumer.GenericResponse; import com.gotocompany.firehose.consumer.TestMessage; +import com.gotocompany.firehose.exception.OperationNotSupportedException; import org.junit.Before; import org.junit.Test; import org.junit.jupiter.api.Assertions; @@ -118,4 +119,10 @@ public void shouldThrowIllegalArgumentExceptionWhenCelExpressionContainsUnregist () -> new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), expressionWithUnregisteredMacro)); } + @Test + public void shouldThrowOperationNotSupportedExceptionWhenCelExpressionResultIsNotBoolean() { + Assertions.assertThrows(OperationNotSupportedException.class, + () -> new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), "GenericResponse.errors")); + } + } From 93590c8283f1809bc209daedf6f4c384e8aef35b Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 18 Jul 2024 10:03:45 +0700 Subject: [PATCH 28/38] Use built in UnsupportedOperationException --- src/main/java/com/gotocompany/firehose/utils/CelUtils.java | 5 ++--- .../evaluator/GrpcResponseCelPayloadEvaluatorTest.java | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/utils/CelUtils.java b/src/main/java/com/gotocompany/firehose/utils/CelUtils.java index 3d9ed61dd..8d676edb6 100644 --- a/src/main/java/com/gotocompany/firehose/utils/CelUtils.java +++ b/src/main/java/com/gotocompany/firehose/utils/CelUtils.java @@ -2,7 +2,6 @@ import com.google.protobuf.Descriptors; import com.google.protobuf.Message; -import com.gotocompany.firehose.exception.OperationNotSupportedException; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelValidationException; import dev.cel.common.types.CelType; @@ -57,7 +56,7 @@ public static CelCompiler initializeCelCompiler(Descriptors.Descriptor descripto * @param resultTypeChecker the predicate to evaluate whether return type is supported or not * @return the compiled CEL program * @throws IllegalArgumentException if the CEL program cannot be created - * @throws OperationNotSupportedException if the return type is not supported + * @throws UnsupportedOperationException if the return type is not supported */ public static CelRuntime.Program initializeCelProgram(String celExpression, CelRuntime celRuntime, @@ -67,7 +66,7 @@ public static CelRuntime.Program initializeCelProgram(String celExpression, CelAbstractSyntaxTree celAbstractSyntaxTree = celCompiler.compile(celExpression) .getAst(); if (!resultTypeChecker.test(celAbstractSyntaxTree.getResultType())) { - throw new OperationNotSupportedException("Return type not supported for " + celExpression); + throw new UnsupportedOperationException("Return type not supported for " + celExpression); } return celRuntime.createProgram(celAbstractSyntaxTree); } catch (CelValidationException | CelEvaluationException e) { diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java index b2f9eb13c..c2d12cc55 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java @@ -4,7 +4,6 @@ import com.gotocompany.firehose.consumer.GenericError; import com.gotocompany.firehose.consumer.GenericResponse; import com.gotocompany.firehose.consumer.TestMessage; -import com.gotocompany.firehose.exception.OperationNotSupportedException; import org.junit.Before; import org.junit.Test; import org.junit.jupiter.api.Assertions; @@ -121,7 +120,7 @@ public void shouldThrowIllegalArgumentExceptionWhenCelExpressionContainsUnregist @Test public void shouldThrowOperationNotSupportedExceptionWhenCelExpressionResultIsNotBoolean() { - Assertions.assertThrows(OperationNotSupportedException.class, + Assertions.assertThrows(UnsupportedOperationException.class, () -> new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), "GenericResponse.errors")); } From b7f8ceccc80baf19c7b035bcfefe77c6068fda71 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 18 Jul 2024 10:05:36 +0700 Subject: [PATCH 29/38] Update build-info-extractor --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 3e0c09ff2..62b12263a 100644 --- a/build.gradle +++ b/build.gradle @@ -87,7 +87,7 @@ dependencies { } implementation 'io.confluent:monitoring-interceptors:3.0.0' implementation 'io.grpc:grpc-all:1.53.0' - implementation group: 'org.jfrog.buildinfo', name: 'build-info-extractor', version: '2.6.3' + implementation group: 'org.jfrog.buildinfo', name: 'build-info-extractor', version: '2.25.4' implementation group: 'com.google.gradle', name: 'osdetector-gradle-plugin', version: '1.2.1' implementation group: 'org.apache.ivy', name: 'ivy', version: '2.2.0' implementation group: 'org.mongodb', name: 'mongo-java-driver', version: '3.12.8' From 6283b9c71a339bac48fee5c8fe8323828bd2b739 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 18 Jul 2024 10:11:37 +0700 Subject: [PATCH 30/38] Update to classpath("org.jfrog.buildinfo:build-info-extractor-gradle:4.33.1") --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 62b12263a..b3ee9e7df 100644 --- a/build.gradle +++ b/build.gradle @@ -4,7 +4,7 @@ buildscript { } dependencies { classpath 'com.google.protobuf:protobuf-gradle-plugin:0.8.17' - classpath "org.jfrog.buildinfo:build-info-extractor-gradle:4.4.7" + classpath "org.jfrog.buildinfo:build-info-extractor-gradle:4.33.1" classpath "org.ajoberstar:gradle-git:1.6.0" } } From 3caa4c07b8dedae89cad3065eb915e512bdecaf5 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 18 Jul 2024 10:18:13 +0700 Subject: [PATCH 31/38] Remove OperationNotSupportedException.java --- .../firehose/exception/OperationNotSupportedException.java | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java diff --git a/src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java b/src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java deleted file mode 100644 index ff961d975..000000000 --- a/src/main/java/com/gotocompany/firehose/exception/OperationNotSupportedException.java +++ /dev/null @@ -1,7 +0,0 @@ -package com.gotocompany.firehose.exception; - -public class OperationNotSupportedException extends RuntimeException { - public OperationNotSupportedException(String message) { - super(message); - } -} From 7927c58d5e4d8eb1c8c9b1d2cbab72c44418d29c Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Thu, 18 Jul 2024 13:38:50 +0700 Subject: [PATCH 32/38] Remove jfrog build info on dependencies --- build.gradle | 1 - 1 file changed, 1 deletion(-) diff --git a/build.gradle b/build.gradle index b3ee9e7df..60aa52efe 100644 --- a/build.gradle +++ b/build.gradle @@ -87,7 +87,6 @@ dependencies { } implementation 'io.confluent:monitoring-interceptors:3.0.0' implementation 'io.grpc:grpc-all:1.53.0' - implementation group: 'org.jfrog.buildinfo', name: 'build-info-extractor', version: '2.25.4' implementation group: 'com.google.gradle', name: 'osdetector-gradle-plugin', version: '1.2.1' implementation group: 'org.apache.ivy', name: 'ivy', version: '2.2.0' implementation group: 'org.mongodb', name: 'mongo-java-driver', version: '3.12.8' From 02d39bed1f0a64af0144e69f73654853a4fe9768 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 19 Jul 2024 13:40:33 +0700 Subject: [PATCH 33/38] - Tidy up tests - Make exception message more verbose --- .../GrpcResponseCelPayloadEvaluator.java | 3 +- .../firehose/sink/grpc/GrpcSinkFactory.java | 4 +-- .../GrpcResponseCelPayloadEvaluatorTest.java | 34 ++++++++----------- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java index b9e78fdad..b234455a1 100644 --- a/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java +++ b/src/main/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluator.java @@ -38,7 +38,8 @@ public GrpcResponseCelPayloadEvaluator(Descriptors.Descriptor descriptor, String @Override public boolean evaluate(Message payload) { if (!descriptor.getFullName().equals(payload.getDescriptorForType().getFullName())) { - throw new IllegalArgumentException("Payload does not match descriptor"); + throw new IllegalArgumentException(String.format("Payload %s does not match descriptor %s", + payload.getDescriptorForType().getFullName(), descriptor.getFullName())); } return (boolean) CelUtils.evaluate(this.celProgram, payload); } diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java index 2a87a6537..10c10eabf 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java @@ -42,11 +42,11 @@ public static AbstractSink create(Map configuration, StatsDRepor GrpcClient grpcClient = new GrpcClient(new FirehoseInstrumentation(statsDReporter, GrpcClient.class), grpcConfig, managedChannel, stencilClient); firehoseInstrumentation.logInfo("GRPC connection established"); - PayloadEvaluator grpcResponseRetryEvaluator = instantiateRetryEvaluator(grpcConfig, stencilClient); + PayloadEvaluator grpcResponseRetryEvaluator = instantiatePayloadEvaluator(grpcConfig, stencilClient); return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcResponseRetryEvaluator); } - private static PayloadEvaluator instantiateRetryEvaluator(GrpcSinkConfig grpcSinkConfig, StencilClient stencilClient) { + private static PayloadEvaluator instantiatePayloadEvaluator(GrpcSinkConfig grpcSinkConfig, StencilClient stencilClient) { if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { return new GrpcResponseCelPayloadEvaluator( stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), diff --git a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java index c2d12cc55..6d99cba10 100644 --- a/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java +++ b/src/test/java/com/gotocompany/firehose/evaluator/GrpcResponseCelPayloadEvaluatorTest.java @@ -54,10 +54,7 @@ public void shouldEvaluateResponseToFalseWhenCELExpressionDoesntMatchPayload() { @Test public void shouldEvaluateResponseToTrueWhenCelExpressionMatchesRange() { - PayloadEvaluator evaluator = new GrpcResponseCelPayloadEvaluator( - GenericResponse.getDescriptor(), - "GenericResponse.errors.exists(e, int(e.code) >= 1000 && int(e.code) <= 2000)" - ); + PayloadEvaluator evaluator = forCELExpression("GenericResponse.errors.exists(e, int(e.code) >= 1000 && int(e.code) <= 2000)"); GenericResponse genericResponse = GenericResponse.newBuilder() .setSuccess(false) .setDetail("Detail Message") @@ -75,10 +72,7 @@ public void shouldEvaluateResponseToTrueWhenCelExpressionMatchesRange() { @Test public void shouldEvaluateResponseToFalseWhenCelExpressionMatchesRangeAndNotInSet() { - PayloadEvaluator evaluator = new GrpcResponseCelPayloadEvaluator( - GenericResponse.getDescriptor(), - "GenericResponse.errors.exists(e, int(e.code) >= 1000 && int(e.code) <= 2000 && !(int(e.code) in [1500, 1600]))" - ); + PayloadEvaluator evaluator = forCELExpression("GenericResponse.errors.exists(e, int(e.code) >= 1000 && int(e.code) <= 2000 && !(int(e.code) in [1500, 1600]))"); GenericResponse genericResponse = GenericResponse.newBuilder() .setSuccess(false) .setDetail("Detail Message") @@ -94,34 +88,34 @@ public void shouldEvaluateResponseToFalseWhenCelExpressionMatchesRangeAndNotInSe Assertions.assertFalse(result); } - @Test + @Test(expected = IllegalArgumentException.class) public void shouldThrowIllegalArgumentExceptionWhenCelValidationFailed() { - Assertions.assertThrows(IllegalArgumentException.class, () -> new GrpcResponseCelPayloadEvaluator( - GenericResponse.getDescriptor(), "GenericResponse.nonExistField == true")); + forCELExpression("GenericResponse.nonExistField == true"); } - @Test + @Test(expected = IllegalArgumentException.class) public void shouldThrowIllegalArgumentExceptionWhenPayloadIsNotRecognizedByDescriptor() { TestMessage unregisteredPayload = TestMessage.newBuilder() .setOrderUrl("url") .build(); - Assertions.assertThrows(IllegalArgumentException.class, - () -> grpcPayloadEvaluator.evaluate(unregisteredPayload)); + grpcPayloadEvaluator.evaluate(unregisteredPayload); } - @Test + @Test(expected = IllegalArgumentException.class) public void shouldThrowIllegalArgumentExceptionWhenCelExpressionContainsUnregisteredMacro() { String expressionWithUnregisteredMacro = "GenericResponse.errors.nonStandardMacro(e, e.code == \"400\")"; - Assertions.assertThrows(IllegalArgumentException.class, - () -> new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), expressionWithUnregisteredMacro)); + forCELExpression(expressionWithUnregisteredMacro); } - @Test + @Test(expected = UnsupportedOperationException.class) public void shouldThrowOperationNotSupportedExceptionWhenCelExpressionResultIsNotBoolean() { - Assertions.assertThrows(UnsupportedOperationException.class, - () -> new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), "GenericResponse.errors")); + forCELExpression("GenericResponse.errors"); + } + + private static PayloadEvaluator forCELExpression(String celExpression) { + return new GrpcResponseCelPayloadEvaluator(GenericResponse.getDescriptor(), celExpression); } } From 312cc133c9b79ce5537ab4f33cda7e5911d32414 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 19 Jul 2024 13:42:06 +0700 Subject: [PATCH 34/38] Bump version --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 60aa52efe..cc79f20e2 100644 --- a/build.gradle +++ b/build.gradle @@ -33,7 +33,7 @@ lombok { } group 'com.gotocompany' -version '0.10.3' +version '0.10.4' def projName = "firehose" From 6a706cb53c45ddebef9db0ed9bdcccae77277901 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 19 Jul 2024 16:33:14 +0700 Subject: [PATCH 35/38] Makes error type for retryable error configurable through env --- .../firehose/config/GrpcSinkConfig.java | 7 +++++ .../GrpcSinkRetryErrorTypeConverter.java | 14 ++++++++++ .../firehose/sink/grpc/GrpcSink.java | 6 +++- .../firehose/sink/grpc/GrpcSinkFactory.java | 2 +- .../GrpcSinkRetryErrorTypeConverterTest.java | 28 +++++++++++++++++++ .../firehose/sink/grpc/GrpcSinkTest.java | 15 +++++----- 6 files changed, 63 insertions(+), 9 deletions(-) create mode 100644 src/main/java/com/gotocompany/firehose/config/converter/GrpcSinkRetryErrorTypeConverter.java create mode 100644 src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java diff --git a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java index bda2bc286..3a9373ca1 100644 --- a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java +++ b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java @@ -1,6 +1,8 @@ package com.gotocompany.firehose.config; +import com.gotocompany.depot.error.ErrorType; import com.gotocompany.firehose.config.converter.GrpcMetadataConverter; +import com.gotocompany.firehose.config.converter.GrpcSinkRetryErrorTypeConverter; import io.grpc.Metadata; import org.aeonbits.owner.Config; @@ -34,6 +36,11 @@ public interface GrpcSinkConfig extends AppConfig { @DefaultValue("") String getSinkGrpcResponseRetryCELExpression(); + @Config.Key("SINK_GRPC_RESPONSE_RETRY_ERROR_TYPE") + @DefaultValue("ErrorType.DEFAULT_ERROR") + @ConverterClass(GrpcSinkRetryErrorTypeConverter.class) + ErrorType getSinkGrpcRetryErrorType(); + @Key("SINK_GRPC_METADATA") @DefaultValue("") @ConverterClass(GrpcMetadataConverter.class) diff --git a/src/main/java/com/gotocompany/firehose/config/converter/GrpcSinkRetryErrorTypeConverter.java b/src/main/java/com/gotocompany/firehose/config/converter/GrpcSinkRetryErrorTypeConverter.java new file mode 100644 index 000000000..25da0603a --- /dev/null +++ b/src/main/java/com/gotocompany/firehose/config/converter/GrpcSinkRetryErrorTypeConverter.java @@ -0,0 +1,14 @@ +package com.gotocompany.firehose.config.converter; + +import com.gotocompany.depot.error.ErrorType; +import org.aeonbits.owner.Converter; + +import java.lang.reflect.Method; +import java.util.Locale; + +public class GrpcSinkRetryErrorTypeConverter implements Converter { + @Override + public ErrorType convert(Method method, String s) { + return ErrorType.valueOf(s.trim().toUpperCase(Locale.ROOT)); + } +} diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java index aa88e57a3..048c057ef 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSink.java @@ -3,6 +3,7 @@ import com.gotocompany.depot.error.ErrorInfo; import com.gotocompany.depot.error.ErrorType; +import com.gotocompany.firehose.config.GrpcSinkConfig; import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.exception.DefaultException; import com.gotocompany.firehose.exception.DeserializerException; @@ -25,16 +26,19 @@ public class GrpcSink extends AbstractSink { private final GrpcClient grpcClient; private final StencilClient stencilClient; + private final GrpcSinkConfig grpcSinkConfig; private List messages; private PayloadEvaluator retryEvaluator; public GrpcSink(FirehoseInstrumentation firehoseInstrumentation, GrpcClient grpcClient, StencilClient stencilClient, + GrpcSinkConfig grpcSinkConfig, PayloadEvaluator retryEvaluator) { super(firehoseInstrumentation, "grpc"); this.grpcClient = grpcClient; this.stencilClient = stencilClient; + this.grpcSinkConfig = grpcSinkConfig; this.retryEvaluator = retryEvaluator; } @@ -74,7 +78,7 @@ private void setRetryableErrorInfo(Message message, DynamicMessage dynamicMessag boolean eligibleToRetry = retryEvaluator.evaluate(dynamicMessage); if (eligibleToRetry) { getFirehoseInstrumentation().logDebug("Retrying grpc service"); - message.setErrorInfo(new ErrorInfo(new DefaultException("Retryable gRPC Error"), ErrorType.SINK_RETRYABLE_ERROR)); + message.setErrorInfo(new ErrorInfo(new DefaultException("Retryable gRPC Error"), grpcSinkConfig.getSinkGrpcRetryErrorType())); return; } message.setErrorInfo(new ErrorInfo(new DefaultException("Non Retryable gRPC Error"), ErrorType.SINK_NON_RETRYABLE_ERROR)); diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java index 10c10eabf..5d6085b6e 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java @@ -43,7 +43,7 @@ public static AbstractSink create(Map configuration, StatsDRepor GrpcClient grpcClient = new GrpcClient(new FirehoseInstrumentation(statsDReporter, GrpcClient.class), grpcConfig, managedChannel, stencilClient); firehoseInstrumentation.logInfo("GRPC connection established"); PayloadEvaluator grpcResponseRetryEvaluator = instantiatePayloadEvaluator(grpcConfig, stencilClient); - return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcResponseRetryEvaluator); + return new GrpcSink(new FirehoseInstrumentation(statsDReporter, GrpcSink.class), grpcClient, stencilClient, grpcConfig, grpcResponseRetryEvaluator); } private static PayloadEvaluator instantiatePayloadEvaluator(GrpcSinkConfig grpcSinkConfig, StencilClient stencilClient) { diff --git a/src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java b/src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java new file mode 100644 index 000000000..c1bbbb0f8 --- /dev/null +++ b/src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java @@ -0,0 +1,28 @@ +package com.gotocompany.firehose.converter; + +import com.gotocompany.depot.error.ErrorType; +import com.gotocompany.firehose.config.converter.GrpcSinkRetryErrorTypeConverter; +import org.junit.Test; +import org.junit.jupiter.api.Assertions; + +import java.util.Arrays; +import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; + + +public class GrpcSinkRetryErrorTypeConverterTest { + @Test + public void shouldConvertToAppropriateEnumType() { + Map stringToExpectedValue = Arrays.stream(ErrorType.values()) + .collect(Collectors.toMap(ErrorType::toString, Function.identity())); + GrpcSinkRetryErrorTypeConverter grpcSinkRetryErrorTypeConverter = new GrpcSinkRetryErrorTypeConverter(); + + stringToExpectedValue.keySet().stream() + .forEach(key -> { + ErrorType expectedValue = stringToExpectedValue.get(key); + ErrorType actualValue = grpcSinkRetryErrorTypeConverter.convert(null, key); + Assertions.assertEquals(expectedValue, actualValue); + }); + } +} diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index 048a1e44e..3307efd8d 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -55,11 +55,12 @@ public class GrpcSinkTest { @Before public void setUp() { initMocks(this); + when(grpcSinkConfig.getSinkGrpcRetryErrorType()).thenReturn(ErrorType.SINK_RETRYABLE_ERROR); this.grpcResponsePayloadEvaluator = new GrpcResponseCelPayloadEvaluator( GenericResponse.getDescriptor(), "GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"4000\")" ); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, new DefaultGrpcResponsePayloadEvaluator()); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, new DefaultGrpcResponsePayloadEvaluator()); } @Test @@ -102,7 +103,7 @@ public void shouldReturnBackListOfFailedMessages() throws IOException, Deseriali @Test public void shouldCloseStencilClient() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); sink.close(); verify(stencilClient, times(1)).close(); @@ -110,7 +111,7 @@ public void shouldCloseStencilClient() throws IOException { @Test public void shouldLogWhenClosingConnection() throws IOException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); sink.close(); verify(firehoseInstrumentation, times(1)).logInfo("GRPC connection closing"); @@ -118,7 +119,7 @@ public void shouldLogWhenClosingConnection() throws IOException { @Test public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatches() throws InvalidProtocolBufferException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); GenericResponse response = GenericResponse.newBuilder() .setSuccess(false) @@ -135,7 +136,7 @@ public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatche ); when(grpcClient.execute(any(), any())) .thenReturn(dynamicMessage); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, grpcResponsePayloadEvaluator); List result = sink.pushMessage(Collections.singletonList(payload)); @@ -145,7 +146,7 @@ public void shouldReturnFailedMessagesWithRetryableErrorsWhenCELExpressionMatche @Test public void shouldReturnFailedMessagesWithNonRetryableErrorsWhenCELExpressionDoesntMatch() throws InvalidProtocolBufferException { - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, this.grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, this.grpcResponsePayloadEvaluator); Message payload = new Message(new byte[]{}, new byte[]{}, "topic", 0, 1); GenericResponse response = GenericResponse.newBuilder() .setSuccess(false) @@ -162,7 +163,7 @@ public void shouldReturnFailedMessagesWithNonRetryableErrorsWhenCELExpressionDoe ); when(grpcClient.execute(any(), any())) .thenReturn(dynamicMessage); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcResponsePayloadEvaluator); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, grpcResponsePayloadEvaluator); List result = sink.pushMessage(Collections.singletonList(payload)); From e3a4c3f8458d72a928e61b92cfc0b4dcff42e6b9 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 19 Jul 2024 16:34:54 +0700 Subject: [PATCH 36/38] Add 1 more test case --- .../converter/GrpcSinkRetryErrorTypeConverterTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java b/src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java index c1bbbb0f8..26e04d3e1 100644 --- a/src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java +++ b/src/test/java/com/gotocompany/firehose/converter/GrpcSinkRetryErrorTypeConverterTest.java @@ -25,4 +25,10 @@ public void shouldConvertToAppropriateEnumType() { Assertions.assertEquals(expectedValue, actualValue); }); } + + @Test(expected = IllegalArgumentException.class) + public void shouldThrowExceptionForInvalidValue() { + GrpcSinkRetryErrorTypeConverter grpcSinkRetryErrorTypeConverter = new GrpcSinkRetryErrorTypeConverter(); + grpcSinkRetryErrorTypeConverter.convert(null, "ErrorType.UNREGISTERED"); + } } From 2faa366df351ee6480635650ef128bb8d42cdb84 Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Fri, 19 Jul 2024 16:49:23 +0700 Subject: [PATCH 37/38] Update default value --- docs/docs/sinks/grpc-sink.md | 11 ++++++++++- .../gotocompany/firehose/config/GrpcSinkConfig.java | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/docs/sinks/grpc-sink.md b/docs/docs/sinks/grpc-sink.md index c0fcae995..69aca4a15 100644 --- a/docs/docs/sinks/grpc-sink.md +++ b/docs/docs/sinks/grpc-sink.md @@ -95,4 +95,13 @@ For more information about CEL please refer to this documentation : https://gith Example retry rule : - Retry on specific error code : `com.gotocompany.generic.GenericResponse.errors.exists(e, e.code == "400")` - Retry on specific error code range : `com.gotocompany.generic.GenericResponse.errors.exists(e, int(e.code) >= 400 && int(e.code) <= 500)` - - Retry on error codes outside from specific error codes : `com.gotocompany.generic.GenericResponse.errors.exists(e, !(int(e.code) in [400, 500, 600]))` \ No newline at end of file + - Retry on error codes outside from specific error codes : `com.gotocompany.generic.GenericResponse.errors.exists(e, !(int(e.code) in [400, 500, 600]))` + +### `SINK_GRPC_RESPONSE_RETRY_ERROR_TYPE` + +Defines the ErrorType to assign for a retryable error. This is used in conjunction with `SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION` and `ERROR_TYPES_FOR_RETRY`. +Value must be defined in com.gotocompany.depot.error.ErrorType + +- Example value: `SINK_RETRYABLE_ERROR` +- Type: `optional` +- Default Value: `DEFAULT_ERROR` diff --git a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java index 3a9373ca1..6f972f8f0 100644 --- a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java +++ b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java @@ -37,7 +37,7 @@ public interface GrpcSinkConfig extends AppConfig { String getSinkGrpcResponseRetryCELExpression(); @Config.Key("SINK_GRPC_RESPONSE_RETRY_ERROR_TYPE") - @DefaultValue("ErrorType.DEFAULT_ERROR") + @DefaultValue("DEFAULT_ERROR") @ConverterClass(GrpcSinkRetryErrorTypeConverter.class) ErrorType getSinkGrpcRetryErrorType(); From 8f2d33d16004d7f3bb39194ffe21ef522504aafe Mon Sep 17 00:00:00 2001 From: Eka Winata Date: Tue, 23 Jul 2024 21:56:58 +0700 Subject: [PATCH 38/38] Use default value of true on CEL Expression config to retry on default case. Remove implementation of DefaultGrpcResponsePayloadEvaluator.java --- docs/docs/sinks/grpc-sink.md | 3 +++ .../firehose/config/GrpcSinkConfig.java | 2 +- .../DefaultGrpcResponsePayloadEvaluator.java | 10 ---------- .../firehose/sink/grpc/GrpcSinkFactory.java | 17 ++++++----------- .../firehose/sink/grpc/GrpcSinkTest.java | 7 +++++-- 5 files changed, 15 insertions(+), 24 deletions(-) delete mode 100644 src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcResponsePayloadEvaluator.java diff --git a/docs/docs/sinks/grpc-sink.md b/docs/docs/sinks/grpc-sink.md index 2d3fa6b15..384018731 100644 --- a/docs/docs/sinks/grpc-sink.md +++ b/docs/docs/sinks/grpc-sink.md @@ -86,6 +86,7 @@ Defines the amount of time (in milliseconds) gRPC clients are willing to wait fo ### `SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION` Defines the CEL(Common Expression Language) expression used to evaluate whether gRPC sink call should be retried or not based on the gRPC response. +The given CEL expression should evaluate to a boolean value. If the expression evaluates to true, the unsuccessful gRPC sink call will be retried, otherwise it won't. Currently, we support all standard CEL macro including: has, all, exists, exists_one, map, map_filter, filter For more information about CEL please refer to this documentation : https://github.com/google/cel-spec/blob/master/doc/langdef.md @@ -113,6 +114,8 @@ For more information about CEL please refer to this documentation : https://gith - Retry on specific error code : `com.gotocompany.generic.GenericResponse.errors.exists(e, e.code == "400")` - Retry on specific error code range : `com.gotocompany.generic.GenericResponse.errors.exists(e, int(e.code) >= 400 && int(e.code) <= 500)` - Retry on error codes outside from specific error codes : `com.gotocompany.generic.GenericResponse.errors.exists(e, !(int(e.code) in [400, 500, 600]))` + - Disable retry on all cases : `false` + - Retry on all error codes : `true` ### `SINK_GRPC_RESPONSE_RETRY_ERROR_TYPE` diff --git a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java index 7f55ec842..03aaad343 100644 --- a/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java +++ b/src/main/java/com/gotocompany/firehose/config/GrpcSinkConfig.java @@ -34,7 +34,7 @@ public interface GrpcSinkConfig extends AppConfig { Long getSinkGrpcArgDeadlineMS(); @Config.Key("SINK_GRPC_RESPONSE_RETRY_CEL_EXPRESSION") - @DefaultValue("") + @DefaultValue("true") String getSinkGrpcResponseRetryCELExpression(); @Config.Key("SINK_GRPC_RESPONSE_RETRY_ERROR_TYPE") diff --git a/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcResponsePayloadEvaluator.java b/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcResponsePayloadEvaluator.java deleted file mode 100644 index 673419f98..000000000 --- a/src/main/java/com/gotocompany/firehose/evaluator/DefaultGrpcResponsePayloadEvaluator.java +++ /dev/null @@ -1,10 +0,0 @@ -package com.gotocompany.firehose.evaluator; - -import com.google.protobuf.Message; - -public class DefaultGrpcResponsePayloadEvaluator implements PayloadEvaluator { - @Override - public boolean evaluate(Message payload) { - return true; - } -} diff --git a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java index 6deb759d4..c8b315b73 100644 --- a/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java +++ b/src/main/java/com/gotocompany/firehose/sink/grpc/GrpcSinkFactory.java @@ -4,7 +4,6 @@ import com.google.protobuf.Message; import com.gotocompany.firehose.config.AppConfig; import com.gotocompany.firehose.config.GrpcSinkConfig; -import com.gotocompany.firehose.evaluator.DefaultGrpcResponsePayloadEvaluator; import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.metrics.FirehoseInstrumentation; @@ -16,7 +15,6 @@ import io.grpc.ManagedChannelBuilder; import com.gotocompany.stencil.client.StencilClient; import org.aeonbits.owner.ConfigFactory; -import org.apache.commons.lang3.StringUtils; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -37,9 +35,9 @@ public static AbstractSink create(Map configuration, StatsDRepor grpcConfig.getSinkGrpcServiceHost(), grpcConfig.getSinkGrpcServicePort(), grpcConfig.getSinkGrpcMethodUrl(), grpcConfig.getSinkGrpcResponseSchemaProtoClass()); firehoseInstrumentation.logDebug(grpcSinkConfig); ManagedChannel managedChannel = ManagedChannelBuilder.forAddress(grpcConfig.getSinkGrpcServiceHost(), grpcConfig.getSinkGrpcServicePort()) - .keepAliveTime(grpcConfig.getSinkGrpcArgKeepaliveTimeMS(), TimeUnit.MILLISECONDS) - .keepAliveTimeout(grpcConfig.getSinkGrpcArgKeepaliveTimeoutMS(), TimeUnit.MILLISECONDS) - .usePlaintext().build(); + .keepAliveTime(grpcConfig.getSinkGrpcArgKeepaliveTimeMS(), TimeUnit.MILLISECONDS) + .keepAliveTimeout(grpcConfig.getSinkGrpcArgKeepaliveTimeoutMS(), TimeUnit.MILLISECONDS) + .usePlaintext().build(); AppConfig appConfig = ConfigFactory.create(AppConfig.class, configuration); ProtoToMetadataMapper protoToMetadataMapper = new ProtoToMetadataMapper(stencilClient.get(appConfig.getInputSchemaProtoClass()), grpcConfig.getSinkGrpcMetadata()); GrpcClient grpcClient = new GrpcClient(new FirehoseInstrumentation(statsDReporter, GrpcClient.class), grpcConfig, managedChannel, stencilClient, protoToMetadataMapper); @@ -49,12 +47,9 @@ public static AbstractSink create(Map configuration, StatsDRepor } private static PayloadEvaluator instantiatePayloadEvaluator(GrpcSinkConfig grpcSinkConfig, StencilClient stencilClient) { - if (StringUtils.isNotBlank(grpcSinkConfig.getSinkGrpcResponseRetryCELExpression())) { - return new GrpcResponseCelPayloadEvaluator( - stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), - grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); - } - return new DefaultGrpcResponsePayloadEvaluator(); + return new GrpcResponseCelPayloadEvaluator( + stencilClient.get(grpcSinkConfig.getSinkGrpcResponseSchemaProtoClass()), + grpcSinkConfig.getSinkGrpcResponseRetryCELExpression()); } } diff --git a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java index 3307efd8d..9a9a1b464 100644 --- a/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java +++ b/src/test/java/com/gotocompany/firehose/sink/grpc/GrpcSinkTest.java @@ -4,7 +4,6 @@ import com.gotocompany.firehose.config.GrpcSinkConfig; import com.gotocompany.firehose.consumer.GenericError; import com.gotocompany.firehose.consumer.GenericResponse; -import com.gotocompany.firehose.evaluator.DefaultGrpcResponsePayloadEvaluator; import com.gotocompany.firehose.evaluator.GrpcResponseCelPayloadEvaluator; import com.gotocompany.firehose.evaluator.PayloadEvaluator; import com.gotocompany.firehose.exception.DeserializerException; @@ -51,16 +50,20 @@ public class GrpcSinkTest { @Mock private GrpcSinkConfig grpcSinkConfig; + @Mock + private PayloadEvaluator defaultGrpcResponsePayloadEvaluator; + private PayloadEvaluator grpcResponsePayloadEvaluator; @Before public void setUp() { initMocks(this); when(grpcSinkConfig.getSinkGrpcRetryErrorType()).thenReturn(ErrorType.SINK_RETRYABLE_ERROR); + when(defaultGrpcResponsePayloadEvaluator.evaluate(any())).thenReturn(true); this.grpcResponsePayloadEvaluator = new GrpcResponseCelPayloadEvaluator( GenericResponse.getDescriptor(), "GenericResponse.success == false && GenericResponse.errors.exists(e, e.code == \"4000\")" ); - sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, new DefaultGrpcResponsePayloadEvaluator()); + sink = new GrpcSink(firehoseInstrumentation, grpcClient, stencilClient, grpcSinkConfig, defaultGrpcResponsePayloadEvaluator); } @Test