feat: send attempt and timestamp in headers #935

mutianf · 2021-08-04T15:30:45Z

Add attempt number and client timestamp to request headers so on the server side we can collect these client metrics.
BigtableTracer will record attempt number when #attemptStart(int attemptNubmer) is called. Added a getter to get this attemptNumber so we can access it from ApiCallContext in the callable chain.

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)

Fixes #<issue_number_goes_here> ☕️

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

igorbernstein2 · 2021-08-09T18:35:22Z

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

@@ -152,4 +156,8 @@ public void batchRequestSent(long elementCount, long requestSize) {
      child.batchRequestSent(elementCount, requestSize);
    }
  }
+
+  public int getAttempt() {


doc please...without context this is very confusing :)

igorbernstein2 · 2021-08-09T18:37:26Z

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

+  @Override
+  public ApiFuture futureCall(RequestT request, ApiCallContext apiCallContext) {
+    int attemptCount = -1;
+    if (apiCallContext.getTracer() instanceof CompositeTracer) {


when would this be false?

It shouldn't be false, just a sanity check, and so that the EnhancedBigtableStubTest#testCreateReadRowsRawCallable test won't fail

igorbernstein2 · 2021-08-09T18:39:52Z

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

+import java.util.List;
+import java.util.Map;
+
+/** A callable that injects client timestamp and attempt count to request headers. */


Please add a bit about about the other pieces that it interacts with. ie It colborates with the Bigtable/CompositeTracer to identify the retry attempt

Also, please clarify the attempt numbering scheme...is the first attempt 0 or 1?

and what does -1 imply?

Moved this part to stub/metrics/Util

Added comments to multiple places, attempt starts from 0.

igorbernstein2 · 2021-08-09T18:40:58Z

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

+      ResponseObserver<ResponseT> responseObserver,
+      ApiCallContext apiCallContext) {
+    int attemptCount = -1;
+    if (apiCallContext.getTracer() instanceof CompositeTracer) {


when is this false?

igorbernstein2 · 2021-08-09T18:42:41Z

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

+            Arrays.asList(String.valueOf(attemptCount)),
+            EnhancedBigtableStub.TIMESTAMP_HEADER_KEY.name(),
+            Arrays.asList(String.valueOf(System.currentTimeMillis())));
+    ApiCallContext newCallContext = apiCallContext.withExtraHeaders(headers);


I would extract both the key constants and extraction of the api tracer and the creation of the map into a helper class. This way this feature lives in one place and the unary/streaming callables just use it

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

igorbernstein2 · 2021-09-09T14:37:47Z

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

+  static final Metadata.Key<String> ATTEMPT_HEADER_KEY =
+      Metadata.Key.of("attempt", Metadata.ASCII_STRING_MARSHALLER);
+  static final Metadata.Key<String> TIMESTAMP_HEADER_KEY =
+      Metadata.Key.of("client-timing", Metadata.ASCII_STRING_MARSHALLER);


I dont love the name client-timing. How would you feel about attempt-timestamp? or attempt-epoch?

Should these be prefixerd with x-bigtable-?

Updated to attempt-epoch. Will update the cl too

igorbernstein2 · 2021-09-09T14:47:00Z

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

+      attemptCount = ((BigtableTracer) apiCallContext.getTracer()).getAttempt();
+    }
+    ImmutableMap.Builder headers = ImmutableMap.<String, List<String>>builder();
+    if (attemptCount != -1) {


I dont think adding magic value here is useful. I think it would be cleaner to restructure as:

ImmutableMap.Builder headers = ImmutableMap.<String, List<String>>builder(); if (apiCallContext.getTracer() instanceof BigtableTracer) { BigtableTracer bigtableTracer = (BigtableTracer) apiCallContext.getTracer(); int attemptNumber = bigtableTracer.getAttempt(); headers.put(ATTEMPT_HEADER_KEY.name(), Arrays.asList(String.valueOf(attemptNumber))); }

updated.. I don't know why I wrote it that way 😓

igorbernstein2 · 2021-09-09T14:47:47Z

...c/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java

-      callContext
-          .getTracer()
-          .attemptStarted(externalFuture.getAttemptSettings().getOverallAttemptCount());
+      callContext.getTracer().attemptStarted(externalFuture.getAttemptSettings().getAttemptCount());


I think this might be a typo?

Yeah, I reverted the change

igorbernstein2 · 2021-09-13T19:01:50Z

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

@@ -81,6 +88,7 @@ public void attemptStarted(int attemptNumber) {
    for (ApiTracer child : children) {
      child.attemptStarted(attemptNumber);
    }
+    this.attempt = attemptNumber;


this should probably happen first

igorbernstein2 · 2021-09-13T19:02:58Z

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

+  static final Metadata.Key<String> ATTEMPT_HEADER_KEY =
+      Metadata.Key.of("bigtable-attempt", Metadata.ASCII_STRING_MARSHALLER);
+  static final Metadata.Key<String> ATTEMPT_EPOCH_KEY =
+      Metadata.Key.of("bigtable-client-attempt-epoch", Metadata.ASCII_STRING_MARSHALLER);


I think you can drop client-, I dont think there is ambiguity who set this header

Not sure if its better, but have you considered sending the timestamp as binary?

Will keep the naming and timestamp for since the change on the server side depends on this and is already released. We can update them later if we see any problems.

igorbernstein2 · 2021-10-27T15:49:45Z

.../java/com/google/cloud/bigtable/data/v2/stub/metrics/ExtraHeadersSeverStreamingCallable.java

+ * number starts from 0.
+ */
+@InternalApi("For internal use only")
+public final class ExtraHeadersSeverStreamingCallable<RequestT, ResponseT>


typo: missing r in server

Maybe rename it to StatsHeader....

igorbernstein2 · 2021-10-27T15:52:19Z

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

+   * number starts from 0.
+   */
+  static Map<String, List<String>> createExtraHeaders(ApiCallContext apiCallContext) {
+    ImmutableMap.Builder headers = ImmutableMap.<String, List<String>>builder();


please out type params on the variable type

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

igorbernstein2

lgtm after comments

product-auto-label bot added the api: bigtable Issues related to the googleapis/java-bigtable API. label Aug 4, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 4, 2021

igorbernstein2 reviewed Aug 9, 2021

View reviewed changes

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

igorbernstein2 reviewed Aug 9, 2021

View reviewed changes

mutianf force-pushed the headers1 branch from 7c0c253 to 87934b2 Compare August 10, 2021 19:10

mutianf marked this pull request as ready for review August 10, 2021 20:12

mutianf requested review from a team as code owners August 10, 2021 20:12

mutianf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 11, 2021

yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 11, 2021

mutianf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 11, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 11, 2021

igorbernstein2 requested changes Sep 9, 2021

View reviewed changes

mutianf added 5 commits September 9, 2021 18:56

feat: send attempt and timestamp in headers

2e0fe77

refactor

e6e53f8

update clirr files

49142dc

refactor

73cecdd

review updates

ec37f18

mutianf force-pushed the headers1 branch from adecc91 to ec37f18 Compare September 9, 2021 18:56

remove "x-"

857ebef

igorbernstein2 reviewed Oct 7, 2021

View reviewed changes

mutianf added 4 commits October 7, 2021 20:21

update

875840c

Add epoch precision to the header

3d94f6e

use microseconds

492cdab

update micro seconds calculation

22a0f62

fix formatting

98d6043

igorbernstein2 reviewed Oct 27, 2021

View reviewed changes

igorbernstein2 approved these changes Oct 27, 2021

View reviewed changes

mutianf added 3 commits October 27, 2021 12:59

Rename headers class

69bdbba

rename local variables

bee61ed

update comment

6ec8f12

mutianf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2021

mutianf merged commit de3b476 into googleapis:main Oct 29, 2021

mutianf deleted the headers1 branch October 29, 2021 13:41

igorbernstein2 mentioned this pull request Apr 12, 2022

feat: send retry attempt header to ease debugging googleapis/nodejs-bigtable#1068

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: send attempt and timestamp in headers #935

feat: send attempt and timestamp in headers #935

mutianf commented Aug 4, 2021 •

edited

igorbernstein2 Aug 9, 2021

igorbernstein2 Aug 9, 2021

mutianf Aug 10, 2021

igorbernstein2 Aug 9, 2021

igorbernstein2 Aug 9, 2021

igorbernstein2 Aug 9, 2021

mutianf Aug 10, 2021

igorbernstein2 Aug 9, 2021

igorbernstein2 Aug 9, 2021

igorbernstein2 Sep 9, 2021

igorbernstein2 Sep 9, 2021

mutianf Sep 9, 2021

igorbernstein2 Sep 9, 2021

mutianf Sep 9, 2021

igorbernstein2 Sep 9, 2021

mutianf Sep 9, 2021

igorbernstein2 Sep 13, 2021

igorbernstein2 Sep 13, 2021

igorbernstein2 Sep 13, 2021

mutianf Oct 7, 2021

igorbernstein2 Oct 27, 2021

igorbernstein2 Oct 27, 2021

igorbernstein2 Oct 27, 2021

igorbernstein2 left a comment

feat: send attempt and timestamp in headers #935

feat: send attempt and timestamp in headers #935

Conversation

mutianf commented Aug 4, 2021 • 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

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

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

igorbernstein2 left a comment

Choose a reason for hiding this comment

mutianf commented Aug 4, 2021 •

edited