chore: misc non-functional cleanup (#9750)

Author: igorbernstein2

bigtable/bigtable-proxy/src/main/java/com/google/cloud/bigtable/examples/proxy/channelpool/ChannelPool.java

@@ -47,7 +47,7 @@
  * A {@link ManagedChannel} that will send requests round-robin via a set of channels.
  *
  * <p>In addition to spreading requests over a set of child connections, the pool will also actively
- * manage the lifecycle of the channels. Currently lifecycle management is limited to pre-emptively
+ * manage the lifecycle of the channels. Currently, lifecycle management is limited to pre-emptively
  * replacing channels every hour. In the future it will dynamically size the pool based on number of
  * outstanding requests.
  *

bigtable/bigtable-proxy/src/main/java/com/google/cloud/bigtable/examples/proxy/channelpool/DataChannel.java

@@ -49,6 +49,10 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/**
+ * Decorator for a Bigtable data plane connection to add channel warming via PingAndWarm. Channel
+ * warming will happen on creation and then every 3 minutes.
+ */
 public class DataChannel extends ManagedChannel {
   private static final Logger LOGGER = LoggerFactory.getLogger(DataChannel.class);
 
@@ -111,6 +115,8 @@ private void warm() {
       return;
     }
 
+    LOGGER.debug("Warming channel {} with: {}", inner, primingKeys);
+
     List<ListenableFuture<PingAndWarmResponse>> futures =
         primingKeys.stream().map(this::sendPingAndWarm).collect(Collectors.toList());
 

bigtable/bigtable-proxy/src/main/java/com/google/cloud/bigtable/examples/proxy/channelpool/ResourceCollector.java

@@ -37,7 +37,7 @@ public void collect(CallLabels labels) {
     try {
       PrimingKey.from(labels).ifPresent(k -> primingKeys.put(k, true));
     } catch (ParsingException e) {
-      LOG.atWarn().log("Failed to collect priming request for {}", labels, e);
+      LOG.warn("Failed to collect priming request for {}", labels, e);
     }
   }
 

bigtable/bigtable-proxy/src/main/java/com/google/cloud/bigtable/examples/proxy/commands/Verify.java

@@ -33,7 +33,6 @@
 import com.google.cloud.opentelemetry.metric.GoogleCloudMetricExporter;
 import com.google.cloud.opentelemetry.metric.MetricConfiguration;
 import com.google.common.collect.ImmutableList;
-import com.google.common.net.PercentEscaper;
 import com.google.protobuf.ByteString;
 import io.grpc.CallCredentials;
 import io.grpc.CallOptions;
@@ -49,18 +48,13 @@
 import io.grpc.MethodDescriptor;
 import io.grpc.StatusRuntimeException;
 import io.grpc.auth.MoreCallCredentials;
-import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.contrib.gcp.resource.GCPResourceProvider;
 import io.opentelemetry.sdk.common.CompletableResultCode;
 import io.opentelemetry.sdk.metrics.data.MetricData;
 import io.opentelemetry.sdk.metrics.export.MetricExporter;
-import io.opentelemetry.sdk.metrics.internal.data.ImmutableGaugeData;
-import io.opentelemetry.sdk.metrics.internal.data.ImmutableLongPointData;
-import io.opentelemetry.sdk.metrics.internal.data.ImmutableMetricData;
 import io.opentelemetry.sdk.resources.Resource;
-import java.time.Duration;
-import java.time.Instant;
-import java.time.temporal.ChronoUnit;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
 import java.util.Iterator;
 import java.util.concurrent.Callable;
 import java.util.concurrent.TimeUnit;
@@ -123,10 +117,12 @@ private void checkBigtable(CallCredentials callCredentials, String tableName) {
 
     try {
       Metadata md = new Metadata();
-      PercentEscaper escaper = new PercentEscaper("", true);
+
       md.put(
           Key.of("x-goog-request-params", Metadata.ASCII_STRING_MARSHALLER),
-          String.format("table_name=%s&app_profile_id=%s", escaper.escape(tableName), ""));
+          String.format(
+              "table_name=%s&app_profile_id=%s",
+              URLEncoder.encode(tableName, StandardCharsets.UTF_8), ""));
 
       BigtableBlockingStub stub =
           BigtableGrpc.newBlockingStub(channel)
@@ -186,43 +182,28 @@ private void checkBigtable(CallCredentials callCredentials, String tableName) {
   }
 
   void checkMetrics(Credentials creds) {
-    Instant end = Instant.now().truncatedTo(ChronoUnit.MINUTES);
-    Instant start = end.minus(Duration.ofMinutes(1));
+    MetricConfiguration config =
+        MetricConfiguration.builder()
+            .setCredentials(creds)
+            .setProjectId(metricsProjectId)
+            .setInstrumentationLibraryLabelsEnabled(false)
+            .build();
 
     GCPResourceProvider resourceProvider = new GCPResourceProvider();
     Resource resource = Resource.create(resourceProvider.getAttributes());
+    ImmutableList<MetricData> metricData =
+        ImmutableList.of(MetricsImpl.generateTestPresenceMeasurement(resource));
 
-    MetricExporter exporter =
-        GoogleCloudMetricExporter.createWithConfiguration(
-            MetricConfiguration.builder()
-                .setCredentials(creds)
-                .setProjectId(metricsProjectId)
-                .setInstrumentationLibraryLabelsEnabled(false)
-                .build());
+    try (MetricExporter exporter = GoogleCloudMetricExporter.createWithConfiguration(config)) {
+      CompletableResultCode result = exporter.export(metricData);
+      result.join(1, TimeUnit.MINUTES);
 
-    ImmutableList<MetricData> metricData =
-        ImmutableList.of(
-            ImmutableMetricData.createLongGauge(
-                resource,
-                MetricsImpl.INSTRUMENTATION_SCOPE_INFO,
-                MetricsImpl.METRIC_PRESENCE_NAME,
-                MetricsImpl.METRIC_PRESENCE_DESC,
-                MetricsImpl.METRIC_PRESENCE_UNIT,
-                ImmutableGaugeData.create(
-                    ImmutableList.of(
-                        ImmutableLongPointData.create(
-                            TimeUnit.MILLISECONDS.toNanos(start.toEpochMilli()),
-                            TimeUnit.MILLISECONDS.toNanos(end.toEpochMilli()),
-                            Attributes.empty(),
-                            1L)))));
-    CompletableResultCode result = exporter.export(metricData);
-    result.join(1, TimeUnit.MINUTES);
-
-    System.out.println("Metrics resource: " + resource);
-    if (result.isSuccess()) {
-      System.out.println("Metrics write: OK");
-    } else {
-      System.out.println("Metrics write: FAILED: " + result.getFailureThrowable().getMessage());
+      System.out.println("Metrics resource: " + resource);
+      if (result.isSuccess()) {
+        System.out.println("Metrics write: OK");
+      } else {
+        System.out.println("Metrics write: FAILED: " + result.getFailureThrowable().getMessage());
+      }
     }
   }
 

bigtable/bigtable-proxy/src/main/java/com/google/cloud/bigtable/examples/proxy/core/CallLabels.java

@@ -41,6 +41,10 @@
  *
  * <ul>
  *   <li>{@code x-goog-request-params} - contains the resource and app profile id
+ *   <li>{@code google-cloud-resource-prefix} - the previous version of {@code
+ *       x-goog-request-params}, used as a fallback
+ *   <li>{@code x-goog-cbt-cookie-routing} - an opaque blob used to routing RPCs on the serverside
+ *   <li>{@code bigtable-features} - the client's available features
  *   <li>{@code x-goog-api-client} - contains the client info of the downstream client
  * </ul>
  */
@@ -111,6 +115,7 @@ public static CallLabels create(MethodDescriptor<?, ?> method, Metadata headers)
         method, requestParams, legacyResourcePrefix, routingCookie, encodedFeatures, apiClient);
   }
 
+  @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
   @VisibleForTesting
   public static CallLabels create(
       MethodDescriptor<?, ?> method,
@@ -129,6 +134,12 @@ public static CallLabels create(
         apiClient);
   }
 
+  /**
+   * Extracts the resource name, will use {@link #getRequestParams()} if present, otherwise falls
+   * back on {@link #getLegacyResourcePrefix()}. If neither is present, {@link Optional#empty()} is
+   * returned. If there was an issue extracting, a {@link ParsingException} is thrown. In the
+   * primary case, the value will be url decoded.
+   */
   public Optional<String> extractResourceName() throws ParsingException {
     if (getRequestParams().isEmpty()) {
       return getLegacyResourcePrefix();
@@ -167,17 +178,19 @@ public Optional<String> extractResourceName() throws ParsingException {
     return resourceName.map(ResourceName::getValue);
   }
 
-  private static Optional<ResourceNameType> findType(String encodedKey) throws ParsingException {
-    String decodedKey = percentDecode(encodedKey);
-
+  private static Optional<ResourceNameType> findType(String key) {
     for (ResourceNameType type : ResourceNameType.values()) {
-      if (type.name.equals(decodedKey)) {
+      if (type.name.equals(key)) {
         return Optional.of(type);
       }
     }
     return Optional.empty();
   }
 
+  /**
+   * Extracts the app profile id from {@link #getRequestParams()}. Returns {@link Optional#empty()}
+   * if the key is missing. The value will be url decoded.
+   */
   public Optional<String> extractAppProfileId() throws ParsingException {
     String requestParams = getRequestParams().orElse("");
 
@@ -200,13 +213,17 @@ private static String percentDecode(String s) throws ParsingException {
     }
   }
 
+  /**
+   * Can be derived from {@link CallLabels} to create a priming request to keep the channel active
+   * for future RPCs.
+   */
   @AutoValue
   public abstract static class PrimingKey {
-    abstract Map<String, String> getMetadata();
+    protected abstract Map<String, String> getMetadata();
 
-    abstract String getName();
+    protected abstract String getName();
 
-    abstract Optional<String> getAppProfileId();
+    protected abstract Optional<String> getAppProfileId();
 
     public static Optional<PrimingKey> from(CallLabels labels) throws ParsingException {
       final ImmutableMap.Builder<String, String> md = ImmutableMap.builder();

bigtable/bigtable-proxy/src/main/java/com/google/cloud/bigtable/examples/proxy/metrics/InstrumentedCallCredentials.java

@@ -16,18 +16,40 @@
 
 package com.google.cloud.bigtable.examples.proxy.metrics;
 
+import com.google.cloud.bigtable.examples.proxy.channelpool.DataChannel;
+import com.google.cloud.bigtable.examples.proxy.core.CallLabels.PrimingKey;
+import com.google.cloud.bigtable.examples.proxy.core.ProxyHandler;
 import com.google.common.base.Stopwatch;
 import io.grpc.CallCredentials;
+import io.grpc.CallOptions;
 import io.grpc.InternalMayRequireSpecificExecutor;
 import io.grpc.Metadata;
+import io.grpc.ServerCall;
 import io.grpc.Status;
 import java.time.Duration;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+/**
+ * {@link CallCredentials} decorator that tracks latency for fetching credentials.
+ *
+ * <p>This expects that all RPCs that use these credentials embed a {@link Tracer} in the {@link
+ * io.grpc.CallOptions} using {@link Tracer#injectIntoCallOptions(CallOptions)}.
+ *
+ * <p>Known callers:
+ *
+ * <ul>
+ *   <li>{@link DataChannel#sendPingAndWarm(PrimingKey)}
+ *   <li>{@link ProxyHandler#startCall(ServerCall, Metadata)}
+ * </ul>
+ */
 public class InstrumentedCallCredentials extends CallCredentials
     implements InternalMayRequireSpecificExecutor {
+  private static final Logger LOG = LoggerFactory.getLogger(InstrumentedCallCredentials.class);
+
   private final CallCredentials inner;
   private final boolean specificExecutorRequired;
 
@@ -56,15 +78,21 @@ public void applyRequestMetadata(
         new MetadataApplier() {
           @Override
           public void apply(Metadata headers) {
-            tracer.onCredentialsFetch(
-                Status.OK, Duration.ofMillis(stopwatch.elapsed(TimeUnit.MILLISECONDS)));
+            Duration latency = Duration.ofMillis(stopwatch.elapsed(TimeUnit.MILLISECONDS));
+            // Most credentials fetches should very fast because they are cached
+            if (latency.compareTo(Duration.ofMillis(1)) >= 1) {
+              LOG.debug("Fetching Credentials took {}", latency);
+            }
+            tracer.onCredentialsFetch(Status.OK, latency);
             applier.apply(headers);
           }
 
           @Override
           public void fail(Status status) {
-            tracer.onCredentialsFetch(
-                status, Duration.ofMillis(stopwatch.elapsed(TimeUnit.MILLISECONDS)));
+            Duration latency = Duration.ofMillis(stopwatch.elapsed(TimeUnit.MILLISECONDS));
+
+            LOG.warn("Failed to fetch Credentials after {}: {}", latency, status);
+            tracer.onCredentialsFetch(status, latency);
             applier.fail(status);
           }
         });

bigtable/bigtable-proxy/src/main/java/com/google/cloud/bigtable/examples/proxy/metrics/Metrics.java

@@ -21,6 +21,7 @@
 import io.grpc.Status;
 import java.time.Duration;
 
+/** Interface for tracking measurements across the application. */
 public interface Metrics {
   MetricsAttributes createAttributes(CallLabels callLabels);