-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: send attempt and timestamp in headers #935
Conversation
...d-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracer.java
Outdated
Show resolved
Hide resolved
@@ -152,4 +156,8 @@ public void batchRequestSent(long elementCount, long requestSize) { | |||
child.batchRequestSent(elementCount, requestSize); | |||
} | |||
} | |||
|
|||
public int getAttempt() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc please...without context this is very confusing :)
@Override | ||
public ApiFuture futureCall(RequestT request, ApiCallContext apiCallContext) { | ||
int attemptCount = -1; | ||
if (apiCallContext.getTracer() instanceof CompositeTracer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would this be false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be false, just a sanity check, and so that the EnhancedBigtableStubTest#testCreateReadRowsRawCallable
test won't fail
import java.util.List; | ||
import java.util.Map; | ||
|
||
/** A callable that injects client timestamp and attempt count to request headers. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please clarify the attempt numbering scheme...is the first attempt 0 or 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and what does -1 imply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this part to stub/metrics/Util
Added comments to multiple places, attempt starts from 0.
ResponseObserver<ResponseT> responseObserver, | ||
ApiCallContext apiCallContext) { | ||
int attemptCount = -1; | ||
if (apiCallContext.getTracer() instanceof CompositeTracer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when is this false?
Arrays.asList(String.valueOf(attemptCount)), | ||
EnhancedBigtableStub.TIMESTAMP_HEADER_KEY.name(), | ||
Arrays.asList(String.valueOf(System.currentTimeMillis()))); | ||
ApiCallContext newCallContext = apiCallContext.withExtraHeaders(headers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont love the name client-timing. How would you feel about attempt-timestamp
? or attempt-epoch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be prefixerd with x-bigtable-
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to attempt-epoch. Will update the cl too
attemptCount = ((BigtableTracer) apiCallContext.getTracer()).getAttempt(); | ||
} | ||
ImmutableMap.Builder headers = ImmutableMap.<String, List<String>>builder(); | ||
if (attemptCount != -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.. I don't know why I wrote it that way 😓
callContext | ||
.getTracer() | ||
.attemptStarted(externalFuture.getAttemptSettings().getOverallAttemptCount()); | ||
callContext.getTracer().attemptStarted(externalFuture.getAttemptSettings().getAttemptCount()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I reverted the change
@@ -81,6 +88,7 @@ public void attemptStarted(int attemptNumber) { | |||
for (ApiTracer child : children) { | |||
child.attemptStarted(attemptNumber); | |||
} | |||
this.attempt = attemptNumber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably happen first
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can drop client-
, I dont think there is ambiguity who set this header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if its better, but have you considered sending the timestamp as binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* number starts from 0. | ||
*/ | ||
@InternalApi("For internal use only") | ||
public final class ExtraHeadersSeverStreamingCallable<RequestT, ResponseT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: missing r in server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename it to StatsHeader....
* number starts from 0. | ||
*/ | ||
static Map<String, List<String>> createExtraHeaders(ApiCallContext apiCallContext) { | ||
ImmutableMap.Builder headers = ImmutableMap.<String, List<String>>builder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please out type params on the variable type
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/Util.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm after comments
🤖 I have created a release \*beep\* \*boop\* --- ## [2.3.0](https://www.github.com/googleapis/java-bigtable/compare/v2.2.0...v2.3.0) (2021-11-16) ### Features * configure branch 2.2.x as a release branch ([#1044](https://www.github.com/googleapis/java-bigtable/issues/1044)) ([68e8790](https://www.github.com/googleapis/java-bigtable/commit/68e8790f61b90ce2b5e7479b3d23e2f964199d3e)) * send attempt and timestamp in headers ([#935](https://www.github.com/googleapis/java-bigtable/issues/935)) ([de3b476](https://www.github.com/googleapis/java-bigtable/commit/de3b476d4acd644d1e5bc782dc697ce5b145992e)) ### Bug Fixes * **java:** java 17 dependency arguments ([#1046](https://www.github.com/googleapis/java-bigtable/issues/1046)) ([422efa0](https://www.github.com/googleapis/java-bigtable/commit/422efa0289b232118b446224c5e084fe3bc19491)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v2.5.0 ([#1064](https://www.github.com/googleapis/java-bigtable/issues/1064)) ([5b72aa9](https://www.github.com/googleapis/java-bigtable/commit/5b72aa96bab018f4b5b1b565a6487dbb45ccd323)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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 thisattemptNumber
so we can access it fromApiCallContext
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:
Fixes #<issue_number_goes_here> ☕️