Revamp Filters, Support for HTTP Filters, AccessLog Filters #71

ajaypj · 2024-07-19T12:34:50Z

No description provided.

kingster · 2024-07-20T16:10:08Z

core/src/main/java/com/flipkart/gjex/core/filter/GrpcAccessLogFilter.java

+        .append(serverRequestParams.getMethodName()).append(" ")
+        .append(size).append(" ")
+        .append(System.currentTimeMillis()-startTime);
+    error(sb.toString());


why error? here

added access-log separately and made it info

kingster · 2024-07-20T16:11:30Z

core/src/main/java/com/flipkart/gjex/core/web/filter/HttpAccessLogFilter.java

+  public void init(FilterConfig filterConfig) throws ServletException {}
+
+  @Override
+  public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {


We should change the signature to match how the grpc filter has

public void doProcessRequest(R request)

and

public void doProcessResponse(S response)

kingster · 2024-07-20T17:19:01Z

examples/build.gradle

@@ -88,7 +89,7 @@ task runHelloWorldServer(type: JavaExec) {
    classpath = sourceSets.main.runtimeClasspath
    mainClass = 'com.flipkart.gjex.examples.helloworld.HelloWorldApplication'
    args "server", "./src/main/resources/hello_world_config.yml"
-    jvmArgs '--add-opens=java.base/java.lang=ALL-UNNAMED'
+//     jvmArgs '--add-opens=java.base/java.lang=ALL-UNNAMED'


uncheck this line.

kingster · 2024-07-20T17:21:00Z

guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java

+                    if (enableAccessLogs){
+                        filtersMap.get(methodSignature).add(new GrpcAccessLogFilter());
+                    }


why not do this before, and get it added in filtersForMethod as required

kingster · 2024-07-20T17:21:35Z

guice/src/main/java/com/flipkart/gjex/grpc/service/ApiServer.java

+		if (enableAccessLogs) {
+			context.addFilter(new FilterHolder(accessLogFilter), "/*" ,
+					EnumSet.of(DispatcherType.REQUEST));
+		}


we should make extract this out to a private separate function

kingster · 2024-07-20T17:22:33Z

guice/src/main/java/com/flipkart/gjex/guice/GuiceBundle.java

@@ -150,7 +150,7 @@ public void run(T configuration, U configMap, Environment environment) {

 		// Add all Grpc Filters to the Grpc Server
 		filters = getInstances(baseInjector, Filter.class);
-		grpcServer.registerFilters(filters, bindableServices);
+		grpcServer.registerFilters(filters, bindableServices, configuration.getGrpc().isEnableAccessLogs());


lets not pass boolean in arguments. Arguments should be proper class or objects.

kingster · 2024-07-23T11:51:52Z

core/src/main/java/com/flipkart/gjex/core/filter/GjexFilter.java

+ *
+ * @author ajay.jalgaonkar
+ */
+public abstract class GjexFilter<Req, Res, M> implements Logging {


Remove the prefix Gjex. Classes have aboslute names, so we don't need to prefix Gjex since the class path itself has gjex

kingster · 2024-07-23T11:52:14Z

core/src/main/java/com/flipkart/gjex/core/filter/grpc/GjexGrpcFilter.java

+ * @param <Req> Proto V3 message
+ * @param <Res> Proto V3 message
+ */
+public abstract class GjexGrpcFilter<Req extends GeneratedMessageV3, Res extends GeneratedMessageV3>


Remove the prefix Gjex. Classes have aboslute names, so we don't need to prefix Gjex since the class path itself has gjex

kingster · 2024-07-23T11:52:51Z

core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcAccessLogGjexGrpcFilter.java

+ * @author ajay.jalgaonkar
+ *
+ */
+public class GrpcAccessLogGjexGrpcFilter<R extends GeneratedMessageV3,


Too much-repeated names?GrpcAccessLogFilter should have been enough.

kingster · 2024-07-23T11:53:26Z

core/src/main/java/com/flipkart/gjex/core/filter/http/GjexHttpFilter.java

+ * @author ajay.jalgaonkar
+ */
+@Data
+public abstract class GjexHttpFilter extends GjexFilter<ServletRequest, ServletResponse,


please rename to HttpFilter

kingster · 2024-07-23T11:54:02Z

core/src/main/java/com/flipkart/gjex/core/filter/http/GjexHttpFilter.java

+  public void doProcessRequest(RequestParams<ServletRequest, Set<String>> requestParams) {
+    super.doProcessRequest(requestParams);
+  }
+
+  @Override
+  public void doProcessResponseHeaders(Set<String> responseHeaders) {
+    super.doProcessResponseHeaders(responseHeaders);
+  }
+
+  @Override
+  public void doProcessResponse(ResponseParams<ServletResponse> responseParams) {
+    super.doProcessResponse(responseParams);
+  }


why are you defining this methods? These needs to be implemented by the implementors of this abstract class.

kingster · 2024-07-23T11:54:57Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/AuthGjexGrpcFilter.java

 * information contained in the Request headers
 *
 * @author regu.b
 *
 */
 @Named("AuthFilter")
-public class AuthFilter implements Filter<HelloRequest, HelloReply>, Logging {
+public class AuthGjexGrpcFilter extends GjexGrpcFilter<HelloRequest, HelloReply> implements Logging {


Name correction please.

kingster · 2024-07-23T11:55:04Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/LoggingGjexGrpcFilter.java

 * @author regu.b
 */
 @Named("LoggingFilter")
-public class LoggingFilter<Req extends GeneratedMessageV3, Res extends GeneratedMessageV3> implements Filter<Req, Res>, Logging {
+public class LoggingGjexGrpcFilter<Req extends GeneratedMessageV3,


Name correction please.

kingster · 2024-07-23T11:55:54Z

examples/src/main/resources/hello_world_config.yml

@@ -1,6 +1,8 @@
 Grpc:
    server.port: 50051
    server.executorThreads : 4
+    grpcFilterConfig:


Access log can be a top level config.

If there are other config params other than accesslog, then this filterconfig can be used.
did not want to keep accesslog related config separate

kingster · 2024-07-23T11:56:01Z

examples/src/main/resources/hello_world_config.yml

@@ -14,6 +16,8 @@ Api:
    service.selectors: 10
    service.workers: 30
    scheduledexecutor.threadpool.size: 1
+    httpFilterConfig:


Access log can be a top level config.

kingster · 2024-07-23T11:58:33Z

grpc-jexpress-template/src/main/java/com/flipkart/grpc/jexpress/SampleApplication.java

-
-import java.util.Map;
-
-public class SampleApplication extends Application<SampleConfiguration, Map> {


Why have we deleted these files?

kingster · 2024-07-23T11:59:14Z

guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java

                Context previous = attachContext(contextWithHeaders);   // attaching headers to gRPC context
                try {
-                    filters.forEach(filter -> filter.doProcessResponse((GeneratedMessageV3) response));
+                    grpcFilters.forEach(filter -> filter.doProcessResponse(ResponseParams.builder().response(response).build()));


why wrap? what is the usecase?

ResponseParams is a common class used for both http and grpc responses. so wrapped the grpc "response"

kingster · 2024-07-23T11:59:58Z

guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java

+                            RequestParams requestParams =
+                                RequestParams.builder()
+                                    .clientIp(call.getAttributes().get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR).toString())
+                                    .resourcePath(call.getMethodDescriptor().getFullMethodName().toLowerCase())
+                                    .metadata(headers)
+                                    .request(request)
+                                    .build();


Why create this in loop? Can it not be outside?

put it inside.

kingster · 2024-07-24T05:44:48Z

core/src/main/java/com/flipkart/gjex/core/config/ApiService.java

-    public void setScheduledExecutorThreadPoolSize(int scheduledExecutorThreadPoolSize) {
-        this.scheduledExecutorThreadPoolSize = scheduledExecutorThreadPoolSize;
-    }
+    @JsonProperty("httpFilterConfig")


should be named filterConfig since API Section itself is for HTTP

Also, given that it contains a single boolean, may be we flatten it, and make accessLog as top level attribute.

we should keep filterconfig as separate config for grpc and http. so it can't be top level.
in future other updates to filterconfig can come

kingster · 2024-07-24T05:45:06Z

core/src/main/java/com/flipkart/gjex/core/config/GrpcConfig.java

-        this.executorThreads = executorThreads;
-    }
+    @JsonProperty("grpcFilterConfig")
+    private GrpcFilterConfig grpcFilterConfig;


Same here, should be filterConfig simply

kingster · 2024-07-24T05:46:10Z

core/src/main/java/com/flipkart/gjex/core/filter/Filter.java

+ * A Filter interface for processing Request, Request-Headers, Response and Response-Headers 
+ * around gRPC and HTTP method invocation
 *
- * @param <Req> Proto V3 message
- * @param <Res> Proto V3 message
+ * @author ajay.jalgaonkar


Please dont change the original author information. Since there has been no new code here.

kingster · 2024-07-24T05:46:36Z

core/src/main/java/com/flipkart/gjex/core/filter/ResponseParams.java

+
+@Getter
+@Builder
+public class ResponseParams <S>{


Why is this required?

kingster · 2024-07-24T05:47:26Z

core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java

+  public long getStartTime() {
+    return startTime;
+  }
+
+  public void setStartTime(long startTime) {
+    this.startTime = startTime;
+  }
+
+  public RequestParams<R, Metadata> getRequestParams() {
+    return requestParams;
+  }
+
+  public void setRequestParams(RequestParams<R, Metadata> requestParams) {
+    this.requestParams = requestParams;
+  }
+
+  public StringBuilder getSb() {
+    return sb;
+  }
+
+  public void setSb(StringBuilder sb) {
+    this.sb = sb;
+  }


Why are these required, given that @Data has been already used?

kingster · 2024-07-24T05:51:05Z

core/src/main/java/com/flipkart/gjex/core/filter/http/AccessLogHttpFilter.java

+  public long getStartTime() {
+    return startTime;
+  }
+
+  public void setStartTime(long startTime) {
+    this.startTime = startTime;
+  }
+
+  public StringBuilder getSb() {
+    return sb;
+  }
+
+  public void setSb(StringBuilder sb) {
+    this.sb = sb;
+  }


Is this really required?

kingster · 2024-07-24T05:51:35Z

core/src/main/java/com/flipkart/gjex/core/filter/http/HttpFilter.java

+ * @author ajay.jalgaonkar
+ */
+
+@Data


Why does this need to have Data annotation? This is not a pojo class.

kingster · 2024-07-24T05:53:06Z

core/src/main/java/com/flipkart/gjex/core/filter/http/HttpFilterParams.java

+public class HttpFilterParams {
+  private final Filter filter;
+  private final String pathSpec;
+}


We already have RequestParams which has simlar, can we merge?

kingster · 2024-07-24T05:55:04Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/AuthGrpcFilter.java

-@Named("AuthFilter")
-public class AuthFilter implements Filter<HelloRequest, HelloReply>, Logging {
+@Named("AuthGrpcFilter")
+public class AuthGrpcFilter extends GrpcFilter<HelloRequest, HelloReply> {


This can remain AuthFilter

Lets not change the grpc filter names. Gjex is major for grpc and http is provided for migration period. In long term i don't see the need for having this explicit name in filter.s

kingster · 2024-07-24T05:55:20Z

examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/LoggingGrpcFilter.java

-@Named("LoggingFilter")
-public class LoggingFilter<Req extends GeneratedMessageV3, Res extends GeneratedMessageV3> implements Filter<Req, Res>, Logging {
+@Named("LoggingGrpcFilter")
+public class LoggingGrpcFilter<Req extends GeneratedMessageV3,


This can remain LoggingFilter

Lets not change the grpc filter names. Gjex is major for grpc and http is provided for migration period. In long term i don't see the need for having this explicit name in filter.s

This reverts commit 38a2b4a.

* http-filters: tests Update HttpFilterInterceptor.java FilterInteceptor

ajay-jalgaonkar added 2 commits July 19, 2024 18:03

added http access log

8952324

added enableAccessLogs flag

0f99aca

ajaypj changed the title ~~added http access log filter and support for custom http filters~~ added access log filter for grpc and http and custom http filter support Jul 19, 2024

ajay-jalgaonkar added 2 commits July 19, 2024 23:28

Added GrpcAccessLogFilter

60c54a2

added default true for access log filters

bf39538

ajaypj changed the title ~~added access log filter for grpc and http and custom http filter support~~ added access log filter for grpc and http, and custom http filter support Jul 19, 2024

ajay-jalgaonkar added 5 commits July 19, 2024 23:44

added default true for access log filters

df8105b

using arraylist instead of linkedlist

40d375e

author

68c472e

added example

30da9b9

added example

1f686fd

kingster reviewed Jul 20, 2024

View reviewed changes

ajay-jalgaonkar added 2 commits July 20, 2024 22:38

added access log separate for logging

845bb19

added access log separate for logging

e7e5fc7

kingster reviewed Jul 20, 2024

View reviewed changes

added access log separate for logging

0fb3927

kingster reviewed Jul 20, 2024

View reviewed changes

ajay-jalgaonkar added 3 commits July 23, 2024 15:56

added common filters interfaces for http and grpc

c3f72e0

added filter configs

09b8a45

added filter configs

a8f033e

kingster reviewed Jul 23, 2024

View reviewed changes

kingster requested changes Jul 23, 2024

View reviewed changes

ajay-jalgaonkar added 4 commits July 23, 2024 17:54

uncommented

f5303c8

template changes

91ff9ad

template changes

b293189

template changes

0072103

ajay-jalgaonkar added 2 commits July 24, 2024 10:32

added stringbuilder in AccessLogGrpcFilter

0372965

no change

f5a71dc

kingster requested changes Jul 24, 2024

View reviewed changes

ajay-jalgaonkar and others added 7 commits July 24, 2024 12:55

renaming

c6b69df

fucntion name updated

d181572

uncommented

b965c83

Split Request & Request Params

8801ca8

docs

a0f93cf

Update mvn template files

3ef92b3

Update hello_world_config.yml

44a11d2

kingster changed the title ~~added access log filter for grpc and http, and custom http filter support~~ Revamp Filters, Support for HTTP Filters, AccessLog Filters Jul 24, 2024

kingster and others added 16 commits July 24, 2024 19:43

docs

d12fe01

fixes

56fdef2

docs

cf26e64

adding MDC, updated logging pattern

38a2b4a

FilterInteceptor

87ed452

Update HttpFilterInterceptor.java

497c035

tests

07abf21

Revert "adding MDC, updated logging pattern"

ed035f0

This reverts commit 38a2b4a.

Merge branch 'http-filters' into http-accesslog

1f62b1f

* http-filters: tests Update HttpFilterInterceptor.java FilterInteceptor

updated pattern

1e5d26e

added healthcheck in dashboard

50d5f76

author

888fab5

author

1c9601c

author

04c6d6d

minor rename

8a91c93

minor fixes

c18d63b

kingster approved these changes Jul 26, 2024

View reviewed changes

kingster merged commit 404e415 into master Jul 26, 2024
3 checks passed

kingster deleted the http-accesslog branch July 26, 2024 09:42


		import java.util.Map;

		public class SampleApplication extends Application<SampleConfiguration, Map> {

Revamp Filters, Support for HTTP Filters, AccessLog Filters #71

Revamp Filters, Support for HTTP Filters, AccessLog Filters #71

Conversation

ajaypj commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment