Visitar URL original
feat: update tracers to use built in metrics by mutianf · Pull Request #1244 · googleapis/java-bigtable · GitHub
Skip to content
This repository was archived by the owner on Aug 30, 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

@mutianf mutianf 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 team May 10, 2022 18:48
@product-auto-label product-auto-label Bot added size: xl Pull request size is extra large. api: bigtable Issues related to the googleapis/java-bigtable API. labels May 10, 2022
@mutianf
mutianf force-pushed the update_tracer branch 2 times, most recently from 25d61e5 to ca28be6 Compare May 11, 2022 18:42
@mutianf
mutianf force-pushed the update_tracer branch 2 times, most recently from db124f2 to 8a2aa0e Compare May 24, 2022 03:29
@mutianf mutianf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2022
@yoshi-kokoro yoshi-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 Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Resolved
Comment thread ...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Resolved
Comment thread ...ain/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...rc/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java Resolved
Comment thread google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/Util.java Resolved
Comment thread pom.xml Resolved
@mutianf
mutianf force-pushed the update_tracer branch 4 times, most recently from 38b2804 to 353aeb7 Compare June 8, 2022 17:24
igorbernstein2
igorbernstein2 suggested changes Jun 8, 2022
View reviewed changes
Comment thread google-cloud-bigtable-bom/pom.xml Resolved
Comment thread google-cloud-bigtable-stats/pom.xml Resolved
Comment thread ...ud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracer.java Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
@mutianf
mutianf force-pushed the update_tracer branch 4 times, most recently from a198247 to 82e63f0 Compare June 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 Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...rc/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java Resolved
Comment thread .../main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerStreamingCallable.java Resolved
Comment thread ...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java Resolved
Comment thread ...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracer.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
@product-auto-label product-auto-label Bot removed the size: xl Pull request size is extra large. label Jun 21, 2022
@product-auto-label product-auto-label Bot 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 Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
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);

@igorbernstein2 igorbernstein2 Jun 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?

@mutianf mutianf Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

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 Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
Comment thread ...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java Resolved
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");
}

@igorbernstein2 igorbernstein2 Jun 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?

@mutianf mutianf Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

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 Resolved
igorbernstein2
igorbernstein2 approved these changes Jun 28, 2022
View reviewed changes

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

@mutianf mutianf added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 28, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 28, 2022
@mutianf
mutianf merged commit df77560 into googleapis:main Jun 29, 2022
@mutianf
mutianf deleted the update_tracer branch June 29, 2022 14:15
@release-please release-please Bot mentioned this pull request Jun 29, 2022
Merged
gcf-merge-on-green Bot 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 free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

Assignees

No one assigned

Labels

api: bigtable Issues related to the googleapis/java-bigtable API. size: l Pull 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