feat: Surface the server-timing metric by mutianf · Pull Request #535 · googleapis/java-bigtable

mutianf · 2020-11-12T23:49:51Z

Added a BIGTABLE_GFE_LATENCY_VIEW which will track the GFE t4t7 latency.
Added a BIGTABLE_GFE_HEADER_MISSING_COUNT_VIEW which will track the number of responses without the server-timing header.
Added new callables in the callable chains to inject a GrpcMetadataResponse to access the header and trailers. In the callback and response observer, retrieve the serve-timing header from GrpcMetadataResponse, and publish the metrics through HeaderTracer.
GFE related views can be enabled through a new method BigtableDataSettings.enableGfeOpenCensusStats(). The code will only check and publish the metrics when the GFE views are registered. This is because in the case where direct path is enabled, requests won't go through GFE/CFE and therefore won't have server-timing header.

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> ☕️

codecov · 2020-11-12T23:59:05Z

Codecov Report

Merging #535 (757d12b) into master (89c6936) will increase coverage by 0.35%.
The diff coverage is 95.29%.

@@             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

Impacted Files	Coverage Δ	Complexity Δ
...e/cloud/bigtable/data/v2/BigtableDataSettings.java	`69.44% <0.00%> (-1.99%)`	`8.00 <0.00> (ø)`
.../cloud/bigtable/data/v2/stub/metrics/RpcViews.java	`77.27% <84.61%> (+10.60%)`	`7.00 <4.00> (+4.00)`
...ata/v2/stub/metrics/HeaderTracerUnaryCallable.java	`89.47% <89.47%> (ø)`	`3.00 <3.00> (?)`
...ud/bigtable/data/v2/stub/metrics/HeaderTracer.java	`94.87% <94.87%> (ø)`	`9.00 <9.00> (?)`
...ud/bigtable/data/v2/stub/EnhancedBigtableStub.java	`96.91% <100.00%> (+0.85%)`	`26.00 <4.00> (+2.00)`
...ble/data/v2/stub/EnhancedBigtableStubSettings.java	`97.66% <100.00%> (+0.07%)`	`23.00 <1.00> (+1.00)`
...v2/stub/metrics/HeaderTracerStreamingCallable.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
...able/data/v2/stub/metrics/RpcMeasureConstants.java	`94.44% <100.00%> (+1.58%)`	`1.00 <0.00> (ø)`
...igtable/data/v2/stub/metrics/RpcViewConstants.java	`98.07% <100.00%> (+0.40%)`	`1.00 <0.00> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89c6936...757d12b. Read the comment docs.

igorbernstein2

This looks great. I just had a few nit picks

igorbernstein2 · 2020-12-02T15:18:31Z

+            .setStats(stats)
+            .setTagger(tagger)
+            .setStatsAttributes(
+                ImmutableMap.<TagKey, TagValue>builder()


Can you make this map instance shared with the attrs above?

igorbernstein2 · 2020-12-02T15:25:58Z

+@AutoValue
+public abstract class HeaderTracer {
+
+  public static final Metadata.Key<String> SERVER_TIMING_HEADER_KEY =


do these have to be public?

igorbernstein2 · 2020-12-02T15:26:55Z

+
+    public abstract Builder setStatsAttributes(@Nonnull Map<TagKey, TagValue> statsAttributes);
+
+    public abstract Tagger getTagger();


do we need the getters on the builder?

igorbernstein2 · 2020-12-02T15:28:15Z

+      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");


Unless I;m mistaken, this shouldnt be necessary, autovalue should do null checks for all properties that arent marked Nullable

igorbernstein2 · 2020-12-02T15:31:28Z

+
+  public abstract Map<TagKey, TagValue> getStatsAttributes();
+
+  public void recordGfeMetrics(@Nonnull Metadata metadata, String spanName) {


Please add a comment as to when spanName might be null. Its a bit surprising to me

igorbernstein2 · 2020-12-02T15:47:14Z

+@RunWith(JUnit4.class)
+public class HeaderTracerTest {
+
+  private StatsComponent localStats = new StatsComponentImpl();


igorbernstein2 · 2020-12-02T15:47:30Z

+
+@RunWith(JUnit4.class)
+public class HeaderTracerCallableTest {
+  @Rule public MockitoRule mockitoRule = MockitoJUnit.rule();


igorbernstein2 · 2020-12-02T15:49:46Z

+    assertThat(readRowsMissingCount).isEqualTo(readRowsCalls);
+  }
+
+  private class ReadRowAnswer implements Answer {


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?

igorbernstein2 · 2020-12-02T15:50:20Z

+  private StatsComponent localStats = new StatsComponentImpl();
+
+  @Test
+  public void testEmptyBuilder() {


nit. testDefaultBuilder()

igorbernstein2 · 2020-12-02T15:55:10Z

Oh, one more thing: we should chat about DIrectPath interactions

igorbernstein2 · 2020-12-02T16:10:43Z

Also please update the readme

igorbernstein2

lgtm!

Thanks for putting this together!

* 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

mutianf added 3 commits November 12, 2020 01:17

Extract server-timing trailer and create metrics for gfe latency

62a20df

Add more tests and refactor

335d1dc

Refactor comments and imports

5fac4bd

mutianf requested a review from a team November 12, 2020 23:49

product-auto-label Bot added the api: bigtable Issues related to the googleapis/java-bigtable API. label Nov 12, 2020

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 12, 2020

yoshi-code-bot reviewed Nov 12, 2020

View reviewed changes

mutianf added 2 commits November 13, 2020 00:14

reformatting

8ae57a7

Clean up comments

dff4d28

mutianf requested a review from igorbernstein2 November 13, 2020 00:49

kolea2 added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 13, 2020

Merge branch 'master' into gfe-metrics

6a7f526

igorbernstein2 reviewed Nov 25, 2020

View reviewed changes

mutianf added 5 commits November 30, 2020 21:43

Refactor, use GrpcMetadataResponse to get the trailer

c55d9d6

Fix based on the code review

ea9bb30

clean up HeaderTracerResponseObserver

d2e8f8c

Merge branch 'master' into gfe-metrics

eb78b63

Add more tests for all the ops

8ee3b64

igorbernstein2 approved these changes Dec 2, 2020

View reviewed changes

mutianf added 2 commits December 3, 2020 18:48

Improve documents, changes for directPath and more tests

797e9b2

Merge branch 'master' into gfe-metrics

125587f

mutianf requested a review from a team December 3, 2020 18:49

mutianf added 2 commits December 3, 2020 19:07

Small fixes in the doc

20ec8d6

small clean up

757d12b

igorbernstein2 approved these changes Dec 17, 2020

View reviewed changes

igorbernstein2 merged commit 8240779 into googleapis:master Dec 17, 2020

mutianf deleted the gfe-metrics branch December 17, 2020 18:27


		public abstract Builder setStatsAttributes(@Nonnull Map<TagKey, TagValue> statsAttributes);

		public abstract Tagger getTagger();


		public abstract Map<TagKey, TagValue> getStatsAttributes();

		public void recordGfeMetrics(@Nonnull Metadata metadata, String spanName) {

Conversation

mutianf commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

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!

igorbernstein2 commented Dec 2, 2020

Uh oh!

igorbernstein2 commented Dec 2, 2020

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mutianf commented Nov 12, 2020 •

edited

Loading

codecov Bot commented Nov 12, 2020 •

edited

Loading