Skip to content
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

Merged
merged 15 commits into from Dec 17, 2020

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Nov 12, 2020

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

@mutianf mutianf requested a review from as a code owner Nov 12, 2020
@product-auto-label product-auto-label bot added the api: bigtable label Nov 12, 2020
@google-cla google-cla bot added the cla: yes label Nov 12, 2020
@codecov
Copy link

@codecov codecov bot commented Nov 12, 2020

Codecov Report

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

Impacted file tree graph

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

@mutianf mutianf requested a review from igorbernstein2 Nov 13, 2020
@kolea2 kolea2 added kokoro:force-run and removed kokoro:force-run labels Nov 13, 2020
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

This looks great. I just had a few nit picks

.setStats(stats)
.setTagger(tagger)
.setStatsAttributes(
ImmutableMap.<TagKey, TagValue>builder()
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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 =
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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();
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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");
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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) {
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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

@RunWith(JUnit4.class)
public class HeaderTracerTest {

private StatsComponent localStats = new StatsComponentImpl();
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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();
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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 {
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

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() {
Copy link
Contributor

@igorbernstein2 igorbernstein2 Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. testDefaultBuilder()

@igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Dec 2, 2020

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

@igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Dec 2, 2020

Also please update the readme

@mutianf mutianf requested a review from as a code owner Dec 3, 2020
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

lgtm!

Thanks for putting this together!

@igorbernstein2 igorbernstein2 merged commit 8240779 into googleapis:master Dec 17, 2020
19 checks passed
@mutianf mutianf deleted the gfe-metrics branch Dec 17, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Feb 8, 2021
🤖 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).
ad548 pushed a commit to ad548/java-bigtable that referenced this issue Mar 13, 2021
* 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
ad548 pushed a commit to ad548/java-bigtable that referenced this issue Mar 13, 2021
🤖 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).
BenWhitehead pushed a commit to BenWhitehead/java-bigtable that referenced this issue Sep 30, 2021
* ci(java): switch to GitHub Actions (googleapis#533)

* ci: switch to GitHub Actions

* update based on comment

Source-Author: Stephanie Wang <stephaniewang526@users.noreply.github.com>
Source-Date: Mon May 11 12:54:00 2020 -0400
Source-Repo: googleapis/synthtool
Source-Sha: 55cdc844877d97139f25004229842624a6a86a02
Source-Link: googleapis/synthtool@55cdc84

* chore: move ci.yaml to workflows dir (googleapis#535)

Source-Author: Stephanie Wang <stephaniewang526@users.noreply.github.com>
Source-Date: Mon May 11 16:24:04 2020 -0400
Source-Repo: googleapis/synthtool
Source-Sha: 98c50772ec23295c64cf0d2ddf199ea52961fd4c
Source-Link: googleapis/synthtool@98c5077
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants