Add new target attribute to attempts and operation attributes #2260

meeral-k · 2024-06-13T21:21:12Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)
Rollback plan is reviewed and LGTMed
All new data plane features have a completed end to end testing plan

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

…ptor pulls target from GRPC response header.

meeral-k · 2024-06-13T21:22:23Z

@igorbernstein2 PTAL!

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

igorbernstein2 · 2024-06-13T21:38:32Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java

 import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.internal.StringUtils;


please dont use internals of other apis
You probably want:
https://guava.dev/releases/20.0/api/docs/com/google/common/base/Strings.html#isNullOrEmpty-java.lang.String-

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java

igorbernstein2 · 2024-06-13T21:40:05Z

...d-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracer.java

@@ -17,6 +17,7 @@

 import com.google.api.gax.tracing.ApiTracer;
 import com.google.common.collect.ImmutableList;
+import io.opentelemetry.api.internal.StringUtils;


same as above

did you forget to push? its still present

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/Util.java

igorbernstein2 · 2024-06-13T21:41:04Z

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/Util.java

    // Record gfe metrics
    tracer.recordGfeMetadata(latency, throwable);
+    if (responseMetadata.getMetadata() != null) {
+      Metadata.Key<String> remoteAddressKey =


who sets this metadata?

...le/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTestUtils.java

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java

meeral-k · 2024-06-20T15:29:07Z

Addressed all comments, changed implementation to use interceptor with injected api tracer to add target directly.

igorbernstein2 · 2024-06-20T16:06:10Z

...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java

@@ -275,7 +276,9 @@ public static ClientContext createClientContext(EnhancedBigtableStubSettings set
            }

            managedChannelBuilder.intercept(errorCountPerConnectionMetricTracker.getInterceptor());
-
+            if(settings.getIsDirectpath()) {


why only directpath?

igorbernstein2 · 2024-06-20T16:09:04Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

@@ -777,6 +794,7 @@ private Builder() {

      featureFlags =
          FeatureFlags.newBuilder().setReverseScans(true).setLastScannedRowResponses(true);
+      isDirectpath = Boolean.parseBoolean(System.getenv(CBT_ENABLE_DIRECTPATH));


please dont access env vars from multiple places...there should be one place converts the env var into an instance variables in settings and then everything should reference the instance var/getter

igorbernstein2 · 2024-06-20T16:10:16Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java

@@ -175,6 +179,12 @@ public void attemptSucceeded() {
    recordAttemptCompletion(null);
  }

+  public void addTarget(String target) {


setAttemptTarget - there should only be a single target per attempt ... add implies there are multiple targets

igorbernstein2 · 2024-06-20T16:11:24Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java

@@ -85,6 +87,8 @@ class BuiltinMetricsTracer extends BigtableTracer {

  private Long serverLatencies = null;

+  private String target_endpoint = "unspecified";


does this need to be volatile? please check that the thread that sets the target is the same as the thread that reads it

igorbernstein2 · 2024-06-20T16:13:19Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java

    attemptLatenciesHistogram.record(
-        convertToMs(attemptTimer.elapsed(TimeUnit.NANOSECONDS)), attributes);
+        convertToMs(attemptTimer.elapsed(TimeUnit.NANOSECONDS)), attributes.toBuilder().put(TARGET_KEY,target_endpoint).build());


please reformat this line to make it easier to see that you are modifying the attribtues. Ideally split it up into 2 statements the first having a comment that attempt latency metrics differ from others to include the destination

igorbernstein2 · 2024-06-20T16:15:21Z

...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/TargetTracerInterceptor.java

+                    ((BuiltinMetricsTracer)bigtableTracer).addTarget(String.valueOf(remoteAddr));
+                  }
+                } catch (Throwable t) {
+                  LOG.log(


do you really want to log for every RPC?

igorbernstein2 · 2024-06-20T16:16:16Z

...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/TargetTracerInterceptor.java

+                  if(bigtableTracer instanceof BuiltinMetricsTracer) {
+                    ((BuiltinMetricsTracer)bigtableTracer).addTarget(String.valueOf(remoteAddr));
+                  }
+                } catch (Throwable t) {


java java interrupted exceptions require special handling

igorbernstein2 · 2024-06-20T16:17:19Z

...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/TargetTracerInterceptor.java

+                  }
+                } catch (Throwable t) {
+                  LOG.log(
+                      Level.WARNING, "Unexpected error while updating target label", t);


customers will be the ones to see this message and the will have no idea what it means. PLease udpate the message so someone else can understand what it means, what its implications are

igorbernstein2 · 2024-06-20T16:18:38Z

google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/FakeServiceBuilder.java

@@ -31,6 +31,7 @@ public class FakeServiceBuilder {
  private final List<BindableService> services = new ArrayList<>();
  private final List<ServerTransportFilter> transportFilters = new ArrayList<>();

+  private int serverPort = 0;


you can already get this from the returned server?

java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/functional/ReadRowsTest.java

Line 61 in 7247c32

BigtableDataSettings.newBuilderForEmulator(server.getPort())

igorbernstein2 · 2024-06-20T16:19:17Z

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java

@@ -129,6 +131,7 @@ public class BuiltinMetricsTracerTest {
  private static final long SERVER_LATENCY = 100;
  private static final long APPLICATION_LATENCY = 200;
  private static final long SLEEP_VARIABILITY = 15;
+  private static final String TARGET_ENDPOINT_VALUE_FORMAT = "localhost/127.0.0.1:%s";


This is pretty brittle and will fail on ipv6 machines that run tests

meeral-k added 15 commits May 5, 2024 16:43

trying out new interceptor

d1c29ed

testing adding new target label

5155fb2

remove tracker

48b7589

fix tracer

55f65ac

fix interceptor

3ff3223

fix interceptor

c1875fe

add null check

0a34918

testing tracer

6fa4b3f

try using grpc tracer

87e0a54

adding response header

f8a3abe

logging target

29f29a1

add method to composite tracer

85cd43b

Add target label to attributes, populated via new interceptor. Intece…

0f41fda

…ptor pulls target from GRPC response header.

mend

3693912

remove uneecessary logging

10d1958

Merge branch 'main' into b333386278

09f208e

igorbernstein2 requested changes Jun 13, 2024

View reviewed changes

product-auto-label bot added the api: bigtable Issues related to the googleapis/java-bigtable API. label Jun 14, 2024

Inject target into tracer from new target interceptor, update tests

b2861df

product-auto-label bot added the size: m Pull request size is medium. label Jun 20, 2024

meeral-k added 2 commits June 20, 2024 15:21

fix unit test

31c4d21

addressed code comments

ccf7785

igorbernstein2 requested changes Jun 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new target attribute to attempts and operation attributes #2260

Add new target attribute to attempts and operation attributes #2260

meeral-k commented Jun 13, 2024

meeral-k commented Jun 13, 2024

igorbernstein2 Jun 13, 2024

igorbernstein2 Jun 13, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 13, 2024

meeral-k commented Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

igorbernstein2 Jun 20, 2024

		import io.opentelemetry.api.common.Attributes;
		import io.opentelemetry.api.internal.StringUtils;

		@@ -85,6 +87,8 @@ class BuiltinMetricsTracer extends BigtableTracer {

		private Long serverLatencies = null;

		private String target_endpoint = "unspecified";

Add new target attribute to attempts and operation attributes #2260

Are you sure you want to change the base?

Add new target attribute to attempts and operation attributes #2260

Conversation

meeral-k commented Jun 13, 2024

meeral-k commented Jun 13, 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

meeral-k commented Jun 20, 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