gcp-observability: Populate global interceptors from observability #9309

DNVindhya · 2022-06-23T01:46:54Z

This PR populates Global Interceptors and Stream Tracers based on observability configuration.
Also, adds stackdriver exporter required for exporting metric and trace to Google cloud.

CC @sanjaypujare

sanjaypujare · 2022-06-24T18:08:29Z

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

+              observabilityConfig.getDestinationProjectId(),
+              globalLoggingTags.getLocationTags(),
+              globalLoggingTags.getCustomTags(),
+              observabilityConfig.getFlushMessageCount());
      LogHelper helper = new LogHelper(sink, TimeProvider.SYSTEM_TIME_PROVIDER);
      ConfigFilterHelper configFilterHelper = ConfigFilterHelper.factory(observabilityConfig);


Since we are passing observabilityConfig to the non-static grpcInit we can construct configFilterHelper in there? Unless I am missing something?

May be the same thing for many other objects being constructed here and passed to the non-static grpcInit such as sink helper etc?

The reason I am saying is because the non-static grpcInit is unit tested/unit-testable so the more code it has the better it is. But we need to look into it carefully

sanjaypujare · 2022-06-28T20:28:17Z

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


  /**
   * Initialize grpc-observability.
   *
   * @throws ProviderNotFoundException if no underlying channel/server provider is available.
   */
-  public static synchronized GcpObservability grpcInit() throws IOException {
+  public static synchronized GcpObservability grpcInit() throws Exception {


Which other exception type made this necessary from IOException to Exception? Can we just add the additional types (if practical) instead of moving up to Exception?

Stackdriver<Stats/Trace>Exporter throws IllegalStateException. So I will add IllegalStateException to the list of exceptions instead of generic Exception.

gcp-observability/build.gradle

sanjaypujare · 2022-06-30T16:23:48Z

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

+      try {
+        StackdriverStatsExporter.createAndRegister(statsConfigurationBuilder.build());
+      } catch (IOException e) {
+        throw new IOException("Failed to register Stackdriver stats exporter, " + e.getMessage());


This is unusual: catching an IOException and throwing another IOException with just the message modified. Wouldn't the original message be sufficient - so no need to do this? Same for line 181

Also I realized about the IllegalStateException: so we will get that for duplicate registration attempt. Ideally that should not happen because of the guard around grpcInit but a user could have registered the exporter on their own. I guess the only way to resolve that is to document that in the UG? Unless there is a use-case for the user to have a separate registration for their own purpose?

Removed catching and re-throwing exceptions.

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

sanjaypujare · 2022-06-30T18:43:17Z

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

-      Sink sink = new GcpLogSink(observabilityConfig.getDestinationProjectId(),
-          globalLoggingTags.getLocationTags(), globalLoggingTags.getCustomTags(),
-          observabilityConfig.getFlushMessageCount());
+      Sink sink =


This is formatting only change with no real code change - will be good to avoid it to minimize the amount of code showing up as different

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

sanjaypujare · 2022-06-30T19:03:15Z

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

+      tracerFactories.add(InternalCensusTracingAccessor.getServerStreamTracerFactory());
+    }
+
+    if (!clientInterceptors.isEmpty()


Based on the latest change in the other PR, you should remove this check in the if. Even if empty you should now set the interceptors or tracers in the GlobalInterceptors.

Hopefully you can unit test this scenario since it's important for correct functioning

Added unit test to verify empty lists are returned when grpcInit is called and none of the observability features are enabled.

sanjaypujare · 2022-06-30T19:11:13Z

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

+      RpcViews.registerAllGrpcViews();
+      StackdriverStatsConfiguration.Builder statsConfigurationBuilder =
+          StackdriverStatsConfiguration.builder();
+      if (!Strings.isNullOrEmpty(projectId)) {


I can understand null, do we want to include empty to have the same semantics? 2 Qs

will the ObservabilityConfig ever return empty string for the project ID?

if it does do we want special or different semantics for empty project ID?

ObservabilityConfig will not return empty, if the value does not exist in config it returns null instead.
Updated the check.

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

sanjaypujare · 2022-06-30T19:34:14Z

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

-            "'global_trace_sampling_rate' needs to be between 0.0 and 1.0");
-        this.sampler = new Sampler(samplingRate);
+            "'global_trace_sampling_rate' needs to be between [0.0, 1.0]");
+        if (samplingRate == 1.0) {


Comparison of double numbers is tricky and because of precision (and conversion from String to double) issues this equality check may not go through. Has this been unit tested? https://www.baeldung.com/java-comparing-doubles#comparing_in_plain_Java

Updated. PTAL

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

… exporters

sanjaypujare · 2022-07-13T07:05:21Z

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

      }
+      double epsilon = 1e-6;


Why not make this a class final constant with some comment (e.g. why 1e-6)?

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

sanjaypujare · 2022-07-13T07:21:54Z

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

-          "only one of 'global_trace_sampler' or 'global_trace_sampling_rate' can be specified");
-      if (sampler != null) {
-        this.sampler = new Sampler(SamplerType.valueOf(sampler.toUpperCase()));
+      if (enableCloudTracing && samplingRate == null) {


Would it be simpler/better if you just remove the check for enableCloudTracing and do it as follows (with epsilon defined as a class constant)?:

if (samplingRate == null) { this.sampler = Samplers.probabilitySampler(0.0); } else { checkArgument( samplingRate >= 0.0 && samplingRate <= 1.0, "'global_trace_sampling_rate' needs to be between [0.0, 1.0]"); // Using alwaysSample() instead of probabilitySampler() because according to // {@link io.opencensus.trace.samplers.ProbabilitySampler#shouldSample} // there is a (very) small chance of *not* sampling if probability = 1.00. if (Math.abs(1 - samplingRate) < EPSILON) { this.sampler = Samplers.alwaysSample(); } else { this.sampler = Samplers.probabilitySampler(samplingRate); } }

sanjaypujare · 2022-07-13T07:23:26Z

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

+import io.grpc.ClientCall;
+import io.grpc.ClientInterceptor;
+import io.grpc.InternalGlobalInterceptors;
+// import io.grpc.ManagedChannelProvider;


remove the commented line?

sanjaypujare · 2022-07-13T10:58:44Z

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

+        // Using alwaysSample() instead of probabilitySampler() because according to
+        // {@link io.opencensus.trace.samplers.ProbabilitySampler#shouldSample}
+        // there is a (very) small chance of *not* sampling if probability = 1.00.
+        if (Math.abs(1 - samplingRate) < epsilon) {


nit: Since samplingRate is always going to be <= 1.0 (because of the check on line 112) the Math.abs is strictly unnecessary. But I am okay to leave it there as a generalized double equality comparison logic

Removed Math.abs

sanjaypujare · 2022-07-13T10:59:43Z

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

+import io.grpc.ServerCall;
+import io.grpc.ServerCallHandler;
+import io.grpc.ServerInterceptor;
+// import io.grpc.ServerProvider;


another commented import line? Any reason to leave them here?

This was an oversight. Removed both.

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

sanjaypujare · 2022-07-13T11:05:08Z

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

+          mock(InternalLoggingServerInterceptor.Factory.class);
+      when(serverInterceptorFactory.create()).thenReturn(serverInterceptor);
+
+      try (GcpObservability observability =


observability is unused - I am surprised you don't get a compile or link warning. If that was the case you can rename this var to unused or use the SuppressWarning annotation.

sanjaypujare

Left a few comments and it will be good to get them addressed but otherwise looks good.

…rpc#9309) * Populate global interceptors from observability and added stackdriver exporters

sanjaypujare reviewed Jun 24, 2022

View reviewed changes

DNVindhya marked this pull request as ready for review June 27, 2022 21:27

sanjaypujare reviewed Jun 28, 2022

View reviewed changes

sanjaypujare reviewed Jun 30, 2022

View reviewed changes

gcp-observability/build.gradle Show resolved Hide resolved

sanjaypujare reviewed Jun 30, 2022

View reviewed changes

gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java Show resolved Hide resolved

sanjaypujare reviewed Jun 30, 2022

View reviewed changes

gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java Show resolved Hide resolved

sanjaypujare reviewed Jun 30, 2022

View reviewed changes

gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java Show resolved Hide resolved

sanjaypujare reviewed Jun 30, 2022

View reviewed changes

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java Show resolved Hide resolved

sanjaypujare reviewed Jun 30, 2022

View reviewed changes

gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java Outdated Show resolved Hide resolved

DNVindhya added 6 commits June 30, 2022 15:40

Populate global interceptors from observability and added stackdriver…

935a02f

… exporters

Using OpenCensus samplers; added unit test

d407fdd

added project_id for unit tests

4d685d3

refactor stackdriver exporter

528c9f5

throw IO Exception instead of generic Exception

47d8125

addressed comments

6e713a0

DNVindhya force-pushed the gcp-o11y-populate-o11y-interceptors branch from 88efda3 to 6e713a0 Compare July 1, 2022 23:39

sanjaypujare reviewed Jul 13, 2022

View reviewed changes

gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java Show resolved Hide resolved

sanjaypujare reviewed Jul 13, 2022

View reviewed changes

gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java Show resolved Hide resolved

sanjaypujare reviewed Jul 13, 2022

View reviewed changes

sanjaypujare approved these changes Jul 13, 2022

View reviewed changes

addressed comments (2)

fc45695

sanjaypujare merged commit ef89bd3 into grpc:master Jul 14, 2022

DNVindhya deleted the gcp-o11y-populate-o11y-interceptors branch July 14, 2022 17:37

larry-safran pushed a commit to larry-safran/grpc-java that referenced this pull request Jul 14, 2022

gcp-observability: Populate global interceptors from observability (g…

b30cb6a

…rpc#9309) * Populate global interceptors from observability and added stackdriver exporters

github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcp-observability: Populate global interceptors from observability #9309

gcp-observability: Populate global interceptors from observability #9309

DNVindhya commented Jun 23, 2022

sanjaypujare Jun 24, 2022 •

edited

sanjaypujare Jun 28, 2022

DNVindhya Jun 28, 2022

sanjaypujare Jun 30, 2022 •

edited

sanjaypujare Jun 30, 2022 •

edited

DNVindhya Jul 2, 2022

sanjaypujare Jun 30, 2022

DNVindhya Jul 2, 2022

sanjaypujare Jun 30, 2022 •

edited

DNVindhya Jul 2, 2022

sanjaypujare Jun 30, 2022

DNVindhya Jul 2, 2022

sanjaypujare Jun 30, 2022

DNVindhya Jul 2, 2022

sanjaypujare Jul 13, 2022

DNVindhya Jul 13, 2022

sanjaypujare Jul 13, 2022 •

edited

DNVindhya Jul 13, 2022

sanjaypujare Jul 13, 2022

sanjaypujare Jul 13, 2022

DNVindhya Jul 13, 2022

sanjaypujare Jul 13, 2022

DNVindhya Jul 13, 2022

sanjaypujare Jul 13, 2022

DNVindhya Jul 13, 2022

sanjaypujare left a comment

gcp-observability: Populate global interceptors from observability #9309

gcp-observability: Populate global interceptors from observability #9309

Conversation

DNVindhya commented Jun 23, 2022

sanjaypujare Jun 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjaypujare Jun 30, 2022 • edited

Choose a reason for hiding this comment

sanjaypujare Jun 30, 2022 • edited

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

sanjaypujare Jun 30, 2022 • edited

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

sanjaypujare Jul 13, 2022 • edited

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

sanjaypujare left a comment

Choose a reason for hiding this comment

sanjaypujare Jun 24, 2022 •

edited

sanjaypujare Jun 30, 2022 •

edited

sanjaypujare Jun 30, 2022 •

edited

sanjaypujare Jun 30, 2022 •

edited

sanjaypujare Jul 13, 2022 •

edited