gcp-observability: updated config to public preview config #9622

DNVindhya · 2022-10-13T14:52:51Z

This PR implements config changes defined for public preview.
With new config schema, logging filters are matched on text-order basis compared to precedence basis and takes separate filters for client and server RPC events.

b/245415575

CC @sanjaypujare @ejona86

ejona86 · 2022-10-13T18:00:41Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java

+    /* Set of fullMethodNames. */
+    public final Set<String> methods;
+
+    /** Number of bytes of header to log. */
    public final Integer headerBytes;


I'd expect these to be unboxed, since they have clear defaults defined. Things like if (logFilter.excludePattern) { are just asking for a NPE right now.

Is matchAll in the same situation?

I think boxed types were used, because of the return types from JsonUtil. But I see that using unboxed types makes sense here. I will update.

ejona86 · 2022-10-13T18:03:41Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

+        new ImmutableSet.Builder<>();
+    ImmutableSet.Builder<String> methodsSetBuilder =
+        new ImmutableSet.Builder<>();
+    List<?> methodsList = JsonUtil.getList(logFilterMap, "methods");


getListOfStrings()

ejona86 · 2022-10-13T18:05:04Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

+    if (methodsList != null) {
+      for (Object pattern : methodsList) {
+        String methodOrServicePattern = String.valueOf(pattern);
+        if (methodOrServicePattern != null) {


This is always true. And will still always be true if swapped to getListOfStrings() above.

ejona86 · 2022-10-13T18:10:51Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java

-    /** Pattern indicating which service/method to log. */
-    public final String pattern;
+    /** Pattern for service/method filter. */
+    public final List<String> pattern;


What is this used for?

This is just used to store patterns as we see in config. Since we already parse it, I will remove.

ejona86 · 2022-10-13T18:13:04Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

+  private void parseMonitoringObject(Map<String, ?> config, String jsonObjectName) {
+    Map<String, ?> rawCloudMonitoring = JsonUtil.getObject(config, jsonObjectName);
+    if (rawCloudMonitoring != null) {
+      if (rawCloudMonitoring.isEmpty()) {


What is with this check?

Added this check for validation, if there are any nested objects and fields we might not want to enable monitoring. But as per new design, having an empty object enabled monitoring. I will remove this check.

ejona86 · 2022-10-13T18:14:13Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

+    if (rawCloudTracing != null) {
+      enableCloudTracing = true;
+      this.sampler = Samplers.probabilitySampler(0.0);
+      if (!rawCloudTracing.isEmpty()) {


What is with this check?

Added this check the intention of adding non-default sampler when specified. Now that you pointed, I see it is redundant. Will remove this check.

ejona86 · 2022-10-13T18:20:22Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

+
+  private void parseLoggingObject(Map<String, ?> config,  String jsonObjectName) {
+    Map<String, ?> rawCloudLogging = JsonUtil.getObject(config, jsonObjectName);
+    if (rawCloudLogging != null) {


nit: These methods would have been better served by if (rawCloudLogging == null) return to avoid the indentation.

Makes sense. I will update.

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java

sanjaypujare · 2022-10-14T05:24:25Z

...are matched on iterative basis...

Might be better to say it uses "text order" to match since "iterative" could be confusing

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java

gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java

gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java

sanjaypujare

Finished this round of review. Will re-review once comments addressed

DNVindhya · 2022-10-17T20:34:41Z

Addressed comments. PTAL

stanley-cheung · 2022-10-17T21:16:22Z

Thanks for the fix. The integration tests all passed.

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java

sanjaypujare

pls resolve conflicts
a few comments left and it will be good to resolve them

…ents(1)

sanjaypujare · 2022-10-18T20:21:34Z

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java

-    /** Number of bytes of each header to log. */
-    public final Integer headerBytes;
+    /* Set of fullMethodNames. */
+    public final Set<String> methods;


I might have alluded to this somewhere else: e.g. when user provides ["service1/method2", "*"] it will be good to sort it to ["*", "service1/method2"] since everything will match "*" and there is no need to do redundant comparison to "service1/method2" in the unsorted one. So it would have been ideal for this to be a List and sorted as per above. But we can make this optimization later.

logRpcMethod in ConfigFilterHelper tries to achieve this by comparing in the order:

check for * filter present in the list of methods

check for all methods under service filter i.e service/* instances

Check against exact method i.e service/method

sanjaypujare

LGTM

ejona86 reviewed Oct 13, 2022

View reviewed changes