Skip to content
This repository was archived by the owner on May 8, 2026. It is now read-only.

feat: update tracers to use built in metrics - #1244

Merged
mutianf merged 15 commits into
googleapis:mainfrom
mutianf:update_tracer
Jun 29, 2022
Merged

feat: update tracers to use built in metrics#1244
mutianf merged 15 commits into
googleapis:mainfrom
mutianf:update_tracer

Conversation

@mutianf

@mutianfmutianf commented May 10, 2022

Copy link
Copy Markdown
Contributor

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

If you write sample code, please follow the samples format.

@mutianf
mutianf requested review from a teamMay 10, 2022 18:48
@product-auto-labelproduct-auto-labelBot added size: xl Pull request size is extra large. api: bigtable Issues related to the googleapis/java-bigtable API. labels May 10, 2022
@mutianf
mutianfforce-pushed the update_tracer branch 2 times, most recently from 25d61e5 to ca28be6CompareMay 11, 2022 18:42
@mutianf
mutianfforce-pushed the update_tracer branch 2 times, most recently from db124f2 to 8a2aa0eCompareMay 24, 2022 03:29
@mutianfmutianf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2022
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2022
igorbernstein2
igorbernstein2 reviewed Jun 7, 2022
View reviewed changes
Comment thread...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java Outdated
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Outdated
Comment thread...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Outdated
Comment thread...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Outdated
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...rc/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java Outdated
Comment threadgoogle-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/Util.java Outdated
Comment threadpom.xml Outdated
@mutianf
mutianfforce-pushed the update_tracer branch 4 times, most recently from 38b2804 to 353aeb7CompareJune 8, 2022 17:24
igorbernstein2
igorbernstein2 suggested changes Jun 8, 2022
View reviewed changes
Comment threadgoogle-cloud-bigtable-bom/pom.xml
Comment threadgoogle-cloud-bigtable-stats/pom.xml
Comment thread...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracer.java
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
@mutianf
mutianfforce-pushed the update_tracer branch 4 times, most recently from a198247 to 82e63f0CompareJune 9, 2022 00:57
igorbernstein2
igorbernstein2 suggested changes Jun 21, 2022
View reviewed changes
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...rc/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java Outdated
Comment thread.../main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerStreamingCallable.java Outdated
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracer.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java
@product-auto-labelproduct-auto-labelBot removed the size: xl Pull request size is extra large. label Jun 21, 2022
@product-auto-labelproduct-auto-labelBot added the size: l Pull request size is large. label Jun 21, 2022
igorbernstein2
igorbernstein2 suggested changes Jun 28, 2022
View reviewed changes
Comment thread...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
statsRecorderWrapper));

final long applicationLatency = 200;
final AtomicInteger counter = new AtomicInteger(0);

@igorbernstein2igorbernstein2Jun 28, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is this atomic?

@mutianfmutianfJun 28, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hmm, because it's used in response observer so it needs to be final, and I also need to update the value, so i need to AtomicInteger here?

Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java
Comment on lines +317 to +320
assertThat(zone.getAllValues()).containsExactly(UNDEFINED, UNDEFINED, UNDEFINED, UNDEFINED);
assertThat(cluster.getAllValues()).containsExactly(UNDEFINED, UNDEFINED, UNDEFINED, UNDEFINED);
assertThat(status.getAllValues()).containsExactly("UNAVAILABLE", "UNAVAILABLE", "OK", "OK");
}

@igorbernstein2igorbernstein2Jun 28, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should there be a test that checks what happens when a retry attempt changes cluster/zone?

@mutianfmutianfJun 28, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It will be! In my next pr :)

Comment thread...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated
igorbernstein2
igorbernstein2 approved these changes Jun 28, 2022
View reviewed changes

@igorbernstein2igorbernstein2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@mutianfmutianf added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 28, 2022
@gcf-owl-botgcf-owl-botBot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 28, 2022
@mutianf
mutianf merged commit df77560 into googleapis:mainJun 29, 2022
@mutianf
mutianf deleted the update_tracer branch June 29, 2022 14:15
@release-pleaserelease-pleaseBot mentioned this pull request Jun 29, 2022
Merged
gcf-merge-on-greenBot pushed a commit that referenced this pull request Jun 30, 2022
@release-please
chore(main): release 2.9.0 (#1275)
333c1f6
🤖 I have created a release *beep* *boop*
---
## [2.9.0](v2.8.0...v2.9.0) (2022-06-30)
### Features
* add built-in metric constants ([#1243](#1243)) ([b1fc00a](b1fc00a))
* **generated:** Add storage_utilization_gib_per_node to Autoscaling target ([a4c6621](a4c6621))
* **generated:** Cloud Bigtable Undelete Table service and message proto files ([a4c6621](a4c6621))
* update tracers to use built in metrics ([#1244](#1244)) ([df77560](df77560))
### Bug Fixes
* fix flaky test ([#1293](#1293)) ([146c824](146c824))
* remove the shaded profile from root pom ([#1274](#1274)) ([5eafcc3](5eafcc3))
* rename HeaderTracer callables to BigtableTracer callables ([#1276](#1276)) ([e0bd6c9](e0bd6c9))
* rename response observer and callbacks ([#1291](#1291)) ([2705a47](2705a47))
### Documentation
* **sample:** update README for native image sample ([#1281](#1281)) ([4a3a2c9](4a3a2c9))
### Dependencies
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.13.0 ([#1285](#1285)) ([1dc7787](1dc7787))
* update dependency org.graalvm.buildtools:junit-platform-native to v0.9.12 ([#1283](#1283)) ([ba7844a](ba7844a))
* update dependency org.graalvm.buildtools:native-maven-plugin to v0.9.12 ([#1284](#1284)) ([75d0066](75d0066))
---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
mpeddada1 pushed a commit that referenced this pull request Jul 18, 2022
@mutianf@mpeddada1
feat: update tracers to use built in metrics (#1244)
3d0da81
* feat: add built in metrics measure and views
* remove status from application latency
* Rename methods and add comments
* update based on comments
* feat: update tracers to use built in metrics
* update on comments
* make stopwatch thread safe
* update comments
* calculate application latency correctly
* remove unused check
* clean up tests
* fix typo
* update test
* fix flaky test
* fix retry count
mpeddada1 pushed a commit that referenced this pull request Jul 18, 2022
@release-please@mpeddada1
chore(main): release 2.9.0 (#1275)
5ec5467
🤖 I have created a release *beep* *boop*
---
## [2.9.0](v2.8.0...v2.9.0) (2022-06-30)
### Features
* add built-in metric constants ([#1243](#1243)) ([b1fc00a](b1fc00a))
* **generated:** Add storage_utilization_gib_per_node to Autoscaling target ([a4c6621](a4c6621))
* **generated:** Cloud Bigtable Undelete Table service and message proto files ([a4c6621](a4c6621))
* update tracers to use built in metrics ([#1244](#1244)) ([df77560](df77560))
### Bug Fixes
* fix flaky test ([#1293](#1293)) ([146c824](146c824))
* remove the shaded profile from root pom ([#1274](#1274)) ([5eafcc3](5eafcc3))
* rename HeaderTracer callables to BigtableTracer callables ([#1276](#1276)) ([e0bd6c9](e0bd6c9))
* rename response observer and callbacks ([#1291](#1291)) ([2705a47](2705a47))
### Documentation
* **sample:** update README for native image sample ([#1281](#1281)) ([4a3a2c9](4a3a2c9))
### Dependencies
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.13.0 ([#1285](#1285)) ([1dc7787](1dc7787))
* update dependency org.graalvm.buildtools:junit-platform-native to v0.9.12 ([#1283](#1283)) ([ba7844a](ba7844a))
* update dependency org.graalvm.buildtools:native-maven-plugin to v0.9.12 ([#1284](#1284)) ([75d0066](75d0066))
---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@igorbernstein2igorbernstein2igorbernstein2 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigtableIssues related to the googleapis/java-bigtable API.size: lPull request size is large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mutianf@igorbernstein2@yoshi-kokoro