-
Notifications
You must be signed in to change notification settings - Fork 86
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: Surface the server-timing metric #535
Conversation
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java
Outdated
Show resolved
Hide resolved
...-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/RpcViewConstants.java
Outdated
Show resolved
Hide resolved
...-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerTest.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #535 +/- ##
============================================
+ Coverage 81.30% 81.66% +0.35%
- Complexity 1128 1151 +23
============================================
Files 106 109 +3
Lines 7040 7192 +152
Branches 368 376 +8
============================================
+ Hits 5724 5873 +149
- Misses 1119 1120 +1
- Partials 197 199 +2
Continue to review full report at Codecov.
|
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java
Outdated
Show resolved
Hide resolved
...gtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableInterceptorProvider.java
Outdated
Show resolved
Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java
Outdated
Show resolved
Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java
Outdated
Show resolved
Hide resolved
...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/ClientHeaderInterceptor.java
Outdated
Show resolved
Hide resolved
...le-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/RpcViews.java
Show resolved
Hide resolved
...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java
Show resolved
Hide resolved
...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/ClientHeaderInterceptor.java
Outdated
Show resolved
Hide resolved
...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/ClientHeaderInterceptor.java
Outdated
Show resolved
Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracer.java
Outdated
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.
This looks great. I just had a few nit picks
.setStats(stats) | ||
.setTagger(tagger) | ||
.setStatsAttributes( | ||
ImmutableMap.<TagKey, TagValue>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.
Can you make this map instance shared with the attrs above?
@AutoValue | ||
public abstract class HeaderTracer { | ||
|
||
public static final Metadata.Key<String> SERVER_TIMING_HEADER_KEY = |
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.
do these have to be public?
|
||
public abstract Builder setStatsAttributes(@Nonnull Map<TagKey, TagValue> statsAttributes); | ||
|
||
public abstract Tagger getTagger(); |
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.
do we need the getters on the builder?
HeaderTracer headerTracer = autoBuild(); | ||
Preconditions.checkNotNull(headerTracer.getStats(), "StatsRecorder must be set"); | ||
Preconditions.checkNotNull(headerTracer.getTagger(), "Tagger must be set"); | ||
Preconditions.checkNotNull(headerTracer.getStatsAttributes(), "Stats attributes must be set"); |
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.
Unless I;m mistaken, this shouldnt be necessary, autovalue should do null checks for all properties that arent marked Nullable
|
||
public abstract Map<TagKey, TagValue> getStatsAttributes(); | ||
|
||
public void recordGfeMetrics(@Nonnull Metadata metadata, String spanName) { |
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 comment as to when spanName might be null. Its a bit surprising to me
...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerCallableTest.java
Show resolved
Hide resolved
@RunWith(JUnit4.class) | ||
public class HeaderTracerTest { | ||
|
||
private StatsComponent localStats = new StatsComponentImpl(); |
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.
final?
|
||
@RunWith(JUnit4.class) | ||
public class HeaderTracerCallableTest { | ||
@Rule public MockitoRule mockitoRule = MockitoJUnit.rule(); |
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.
final?
assertThat(readRowsMissingCount).isEqualTo(readRowsCalls); | ||
} | ||
|
||
private class ReadRowAnswer implements Answer { |
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 is a personal taste thing, but why go through mockito for this? Wouldnt it be simpler to just implement a fake BigtableImplBase?
private StatsComponent localStats = new StatsComponentImpl(); | ||
|
||
@Test | ||
public void testEmptyBuilder() { |
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.
nit. testDefaultBuilder()
Oh, one more thing: we should chat about DIrectPath interactions |
Also please update the readme |
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!
Thanks for putting this together!
🤖 I have created a release \*beep\* \*boop\* --- ## [1.20.0](https://www.github.com/googleapis/java-bigtable/compare/v1.19.2...v1.20.0) (2021-02-05) ### Features * Surface the server-timing metric ([#535](https://www.github.com/googleapis/java-bigtable/issues/535)) ([8240779](https://www.github.com/googleapis/java-bigtable/commit/8240779434a602dc8b2bf90dbe539c5d7470d850)) ### Bug Fixes * fix MetricTracerTest to rebase on head ([#581](https://www.github.com/googleapis/java-bigtable/issues/581)) ([23e97cb](https://www.github.com/googleapis/java-bigtable/commit/23e97cb308403b35fbe972b08048d0e59423e694)) * fix MutateRowsAttemptCallable to avoid NPE in MetricTracer ([#557](https://www.github.com/googleapis/java-bigtable/issues/557)) ([8d71020](https://www.github.com/googleapis/java-bigtable/commit/8d7102003b54757b64fd598290301d3b24fd9c29)) * Retry "received rst stream" ([#586](https://www.github.com/googleapis/java-bigtable/issues/586)) ([b09a21c](https://www.github.com/googleapis/java-bigtable/commit/b09a21c1dd1a05b68bfd3a0134089ba32dca1774)) * update repo name ([#615](https://www.github.com/googleapis/java-bigtable/issues/615)) ([bb3ed6d](https://www.github.com/googleapis/java-bigtable/commit/bb3ed6dcbadbd70dbd9c68152c8d93c4cefd4dcb)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v0.17.1 ([#590](https://www.github.com/googleapis/java-bigtable/issues/590)) ([5035ad0](https://www.github.com/googleapis/java-bigtable/commit/5035ad0db01a9247634137050698c30da29722a6)) * update dependency com.google.cloud:google-cloud-shared-dependencies to v0.18.0 ([#592](https://www.github.com/googleapis/java-bigtable/issues/592)) ([c58b73a](https://www.github.com/googleapis/java-bigtable/commit/c58b73a7d70c8da1581ac06d77b5e362648a0868)) * update dependency com.google.errorprone:error_prone_annotations to v2.5.0 ([#591](https://www.github.com/googleapis/java-bigtable/issues/591)) ([dfa4da7](https://www.github.com/googleapis/java-bigtable/commit/dfa4da75e5ac81cc941177462326f7c38f18bacd)) * update dependency com.google.errorprone:error_prone_annotations to v2.5.1 ([#594](https://www.github.com/googleapis/java-bigtable/issues/594)) ([ea599a1](https://www.github.com/googleapis/java-bigtable/commit/ea599a10e2e4fdbaf56c45b74fbb1ea5a708a7f2)) ### Documentation * Expand hello world snippet to show how to access specific cells ([#516](https://www.github.com/googleapis/java-bigtable/issues/516)) ([a9001a8](https://www.github.com/googleapis/java-bigtable/commit/a9001a88f338fc2acf6bc48927765f29819124ee)) * Update stackdriver examples for tracing and stats ([#613](https://www.github.com/googleapis/java-bigtable/issues/613)) ([3e8af74](https://www.github.com/googleapis/java-bigtable/commit/3e8af747b329f6656a410160e8da14fd8227c8fc)) * use autogenerated readme functionality and regenerate ([#568](https://www.github.com/googleapis/java-bigtable/issues/568)) ([844e5be](https://www.github.com/googleapis/java-bigtable/commit/844e5beb6230df6ca220935056aae7f6e73d2bc0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
* Extract server-timing trailer and create metrics for gfe latency * Add more tests and refactor * Refactor comments and imports * reformatting * Clean up comments * Refactor, use GrpcMetadataResponse to get the trailer * Fix based on the code review * clean up HeaderTracerResponseObserver * Add more tests for all the ops * Improve documents, changes for directPath and more tests * Small fixes in the doc * small clean up
🤖 I have created a release \*beep\* \*boop\* --- ## [1.20.0](https://www.github.com/googleapis/java-bigtable/compare/v1.19.2...v1.20.0) (2021-02-05) ### Features * Surface the server-timing metric ([googleapis#535](https://www.github.com/googleapis/java-bigtable/issues/535)) ([8240779](https://www.github.com/googleapis/java-bigtable/commit/8240779434a602dc8b2bf90dbe539c5d7470d850)) ### Bug Fixes * fix MetricTracerTest to rebase on head ([googleapis#581](https://www.github.com/googleapis/java-bigtable/issues/581)) ([23e97cb](https://www.github.com/googleapis/java-bigtable/commit/23e97cb308403b35fbe972b08048d0e59423e694)) * fix MutateRowsAttemptCallable to avoid NPE in MetricTracer ([googleapis#557](https://www.github.com/googleapis/java-bigtable/issues/557)) ([8d71020](https://www.github.com/googleapis/java-bigtable/commit/8d7102003b54757b64fd598290301d3b24fd9c29)) * Retry "received rst stream" ([googleapis#586](https://www.github.com/googleapis/java-bigtable/issues/586)) ([b09a21c](https://www.github.com/googleapis/java-bigtable/commit/b09a21c1dd1a05b68bfd3a0134089ba32dca1774)) * update repo name ([googleapis#615](https://www.github.com/googleapis/java-bigtable/issues/615)) ([bb3ed6d](https://www.github.com/googleapis/java-bigtable/commit/bb3ed6dcbadbd70dbd9c68152c8d93c4cefd4dcb)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v0.17.1 ([googleapis#590](https://www.github.com/googleapis/java-bigtable/issues/590)) ([5035ad0](https://www.github.com/googleapis/java-bigtable/commit/5035ad0db01a9247634137050698c30da29722a6)) * update dependency com.google.cloud:google-cloud-shared-dependencies to v0.18.0 ([googleapis#592](https://www.github.com/googleapis/java-bigtable/issues/592)) ([c58b73a](https://www.github.com/googleapis/java-bigtable/commit/c58b73a7d70c8da1581ac06d77b5e362648a0868)) * update dependency com.google.errorprone:error_prone_annotations to v2.5.0 ([googleapis#591](https://www.github.com/googleapis/java-bigtable/issues/591)) ([dfa4da7](https://www.github.com/googleapis/java-bigtable/commit/dfa4da75e5ac81cc941177462326f7c38f18bacd)) * update dependency com.google.errorprone:error_prone_annotations to v2.5.1 ([googleapis#594](https://www.github.com/googleapis/java-bigtable/issues/594)) ([ea599a1](https://www.github.com/googleapis/java-bigtable/commit/ea599a10e2e4fdbaf56c45b74fbb1ea5a708a7f2)) ### Documentation * Expand hello world snippet to show how to access specific cells ([googleapis#516](https://www.github.com/googleapis/java-bigtable/issues/516)) ([a9001a8](https://www.github.com/googleapis/java-bigtable/commit/a9001a88f338fc2acf6bc48927765f29819124ee)) * Update stackdriver examples for tracing and stats ([googleapis#613](https://www.github.com/googleapis/java-bigtable/issues/613)) ([3e8af74](https://www.github.com/googleapis/java-bigtable/commit/3e8af747b329f6656a410160e8da14fd8227c8fc)) * use autogenerated readme functionality and regenerate ([googleapis#568](https://www.github.com/googleapis/java-bigtable/issues/568)) ([844e5be](https://www.github.com/googleapis/java-bigtable/commit/844e5beb6230df6ca220935056aae7f6e73d2bc0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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> ☕️