Add new target attribute to attempts and operation attributes by meeral-k · Pull Request #2260 · googleapis/java-bigtable

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!

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

 import com.google.common.math.IntMath;
+import io.grpc.CallOptions;
 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-

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


 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

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

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


who sets this metadata?

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


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


why only directpath?

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


      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

    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


  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

-
    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

+                    ((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

+                  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

+                  }
+                } 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

  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

  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

igorbernstein2 closed this Mar 31, 2025


		managedChannelBuilder.intercept(errorCountPerConnectionMetricTracker.getInterceptor());

		if(settings.getIsDirectpath()) {


		private Long serverLatencies = null;

		private String target_endpoint = "unspecified";

Conversation

meeral-k commented Jun 13, 2024

Uh oh!

meeral-k commented Jun 13, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

meeral-k commented Jun 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants