Add CSM for batch write flow control #2685

kongweihan · 2025-10-06T16:18:19Z

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:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
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.

mutianf · 2025-10-06T19:59:25Z

...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java

      Instant now = Instant.now();

      if (now.isBefore(nextTime)) {
+        bigtableTracer.addBatchWriteFlowControlFactor(cappedFactor, status, false);


Do we need to check here that bigtableTracer is not null? I don't think you need status, it doesn't mean anything for this metric? Also the QPS is not updated, do we want to update the metric?

Hmm good point, I guess in case context.getTracer() instanceof BigtableTracer is false? How do we deal with it now? Looks like we don't log error?

Recall that status is to record to indicate whether the factor is from the server or due to errors. One theory for the original issue was that a bunch of errors results in min_factor and throttles the client. We'd want to confirm that theory and know which error it is in the future.

We want to report this factor distribution even if it's not applied to target QPS, thus the field "applied" in the metric.

We skip exporting the tracer if it's not an instance of BigtableTracer ( which should never happen). I think you can add a sanity check for not null.

...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java

mutianf · 2025-10-06T20:03:22Z

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

+   *
+   * @param targetQps The new target QPS for the client.
+   */
+  public void setBatchWriteFlowControlTargetQps(double targetQps) {


@InternalApi

Why this and not other methods in this class?

mutianf · 2025-10-06T20:03:30Z

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

+   * @param status  Status of the request.
+   * @param applied Whether the factor was actually applied.
+   */
+  public void addBatchWriteFlowControlFactor(double factor, @Nullable Throwable status, boolean applied) {


@InternalApi

mutianf · 2025-10-06T20:04:02Z

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

+        Aggregation.sum(),
+        InstrumentType.GAUGE,
+        "1",
+        ImmutableSet.<AttributeKey>builder().addAll(COMMON_ATTRIBUTES).add(STATUS_KEY).build());


I don't think this is what you defined on the server side? You shouldn't need the status_key

hmm very strange I didn't recall I added this... removed.

It's still here?

And it should have method_key

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

mutianf · 2025-10-08T13:22:14Z

...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java

      Instant now = Instant.now();

      if (now.isBefore(nextTime)) {
+        bigtableTracer.addBatchWriteFlowControlFactor(cappedFactor, status, false);


We skip exporting the tracer if it's not an instance of BigtableTracer ( which should never happen). I think you can add a sanity check for not null.

mutianf · 2025-10-08T13:25:35Z

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

+   *
+   * @param targetQps The new target QPS for the client.
+   */
+  public void setBatchWriteFlowControlTargetQps(double targetQps) {}


Please add @InternalApi annotation here and the other method.

mutianf · 2025-10-08T13:26:15Z

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

  static final AttributeKey<String> METHOD_KEY = AttributeKey.stringKey("method");
  static final AttributeKey<String> STATUS_KEY = AttributeKey.stringKey("status");
  static final AttributeKey<String> CLIENT_UID_KEY = AttributeKey.stringKey("client_uid");
+  static final AttributeKey<Boolean> APPLIED_KEY = AttributeKey.booleanKey("applied");


nit: maybe rename it to BATCH_WRITE_QPS_APPLIED_KEY

mutianf · 2025-10-08T13:28:00Z

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

+        Aggregation.sum(),
+        InstrumentType.GAUGE,
+        "1",
+        ImmutableSet.<AttributeKey>builder().addAll(COMMON_ATTRIBUTES).add(STATUS_KEY).build());


It's still here?

mutianf · 2025-10-08T13:28:36Z

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

+        "1",
+        ImmutableSet.<AttributeKey>builder()
+            .addAll(COMMON_ATTRIBUTES)
+            .add(STREAMING_KEY, STATUS_KEY)


I don't think this is right, this should be APPLIED_KEY, STATUS_KEY and METHOD_KEY

mutianf · 2025-10-08T13:28:46Z

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

+        Aggregation.sum(),
+        InstrumentType.GAUGE,
+        "1",
+        ImmutableSet.<AttributeKey>builder().addAll(COMMON_ATTRIBUTES).add(STATUS_KEY).build());


And it should have method_key

mutianf · 2025-10-08T13:29:46Z

...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java

    }

-    private void updateQps(double factor, Duration period) {
+    private void updateQps(double factor, Duration period, @Nullable Throwable t) {


Maybe instead of throwable pass in the status. So you don't need to do the null check, you can just have OK status if the update is from a response

But updateQps is used by both onResponseImpl and onErrorImpl

kongweihan requested review from a team as code owners October 6, 2025 16:18

product-auto-label bot added size: m Pull request size is medium. api: bigtable Issues related to the googleapis/java-bigtable API. labels Oct 6, 2025

kongweihan force-pushed the csm-batch-write-flow-control branch 2 times, most recently from 2ca884e to b4006a2 Compare October 6, 2025 16:54

mutianf reviewed Oct 6, 2025

View reviewed changes

kongweihan force-pushed the csm-batch-write-flow-control branch from b4006a2 to e8a246f Compare October 7, 2025 03:03

mutianf reviewed Oct 8, 2025

View reviewed changes

Add CSM for batch write flow control

ae0ed5a

kongweihan force-pushed the csm-batch-write-flow-control branch from e8a246f to ae0ed5a Compare October 8, 2025 14:27

Add CSM for batch write flow control #2685

Are you sure you want to change the base?

Add CSM for batch write flow control #2685

Uh oh!

Conversation

kongweihan commented Oct 6, 2025

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!

Uh oh!

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!

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