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.

Sign up for GitHub

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

Jump to bottom

feat: update tracers to use built in metrics #1244

Merged

mutianf merged 15 commits into googleapis:main from mutianf:update_tracer

Jun 29, 2022

Contributor

mutianf commented May 10, 2022

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 requested review from a team as code owners

May 10, 2022 18:48

product-auto-label bot added size: xl api: bigtable labels

mutianf force-pushed the update_tracer branch 2 times, most recently from 25d61e5 to ca28be6 Compare

May 11, 2022 18:42

mutianf force-pushed the update_tracer branch 2 times, most recently from db124f2 to 8a2aa0e Compare

May 24, 2022 03:29

mutianf added the kokoro:force-run label

yoshi-kokoro removed the kokoro:force-run label

igorbernstein2 reviewed

View reviewed changes

...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Outdated Show resolved Hide resolved

...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Outdated Show resolved Hide resolved

...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...rc/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java Outdated Show resolved Hide resolved

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/Util.java Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

mutianf force-pushed the update_tracer branch 4 times, most recently from 38b2804 to 353aeb7 Compare

June 8, 2022 17:24

igorbernstein2 requested changes

View reviewed changes

google-cloud-bigtable-bom/pom.xml Outdated Show resolved Hide resolved

google-cloud-bigtable-stats/pom.xml Outdated Show resolved Hide resolved

...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracer.java Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

mutianf force-pushed the update_tracer branch 4 times, most recently from a198247 to 82e63f0 Compare

June 9, 2022 00:57

igorbernstein2 requested changes

View reviewed changes

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...rc/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java Outdated Show resolved Hide resolved

.../main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerStreamingCallable.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracer.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Show resolved Hide resolved

mutianf added 7 commits

June 21, 2022 14:55


          feat: add built in metrics measure and views

a1b64c7


          remove status from application latency

2f8a903


          Rename methods and add comments

20511cf


          update based on comments

1ef694f


          feat: update tracers to use built in metrics

61bdae1


          update on comments

b91b8e9


          make stopwatch thread safe

35bb7fa

mutianf force-pushed the update_tracer branch from c7290fa to 63d6dab Compare

June 21, 2022 19:19

product-auto-label bot removed the size: xl label

product-auto-label bot added the size: l label


          update comments

9c71da9

mutianf force-pushed the update_tracer branch from 63d6dab to 9c71da9 Compare

June 21, 2022 19:25

mutianf added 2 commits

June 24, 2022 12:31


          calculate application latency correctly

02c4389


          remove unused check

fab63cc

igorbernstein2 requested changes

View reviewed changes

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...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);

Contributor

igorbernstein2 Jun 28, 2022

why is this atomic?

Contributor Author

mutianf Jun 28, 2022

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?

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

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

Contributor

igorbernstein2 Jun 28, 2022

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

Contributor Author

mutianf Jun 28, 2022

It will be! In my next pr :)

...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Outdated Show resolved Hide resolved

mutianf added 4 commits

June 28, 2022 12:11


          clean up tests

fc1f215


          fix typo

bdada3f


          update test


          fix flaky test

ee75571

igorbernstein2 approved these changes

View reviewed changes

Contributor

igorbernstein2 left a comment

lgtm


          fix retry count

eceb78e

mutianf added the owlbot:run label

gcf-owl-bot bot removed the owlbot:run label

mutianf merged commit df77560 into googleapis:main

mutianf deleted the update_tracer branch

June 29, 2022 14:15

release-please bot mentioned this pull request

chore(main): release 2.9.0 #1275

Merged

gcf-merge-on-green bot pushed a commit that referenced this pull request


          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


          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


          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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable size: l