Conversation

olavloite

This PR adds an automatically generated identifier to all database clients that are created by the client library. This avoids collisions of the same metrics being registered multiple times, and makes it possible to distinguish different clients from each other in the monitoring.

This PR does not allow the user to specify the id, but this could be added in a future change. That would need an API change by adding an overload to the method getDatabaseClient(DatabaseId) with an additional clientId parameter.

This should also fix the build error in GoogleCloudPlatform/java-docs-samples#2323.

Fixes #106

@googlebotgooglebot added the cla: yesThis human has signed the Contributor License Agreement.label Mar 19, 2020
@olavloiteolavloite requested a review from skuruppu March 19, 2020 14:52
mayurkale22
mayurkale22 reviewed Mar 19, 2020
View reviewed changes

Choose a reason for hiding this comment

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

I have verified these changes locally and seems to be working fine.

If possible, please include unit test to validate the logic.

  1. Multiple database clients for the same database w/ different SessionPoolOptions
  2. Multiple database clients for different databases.

@olavloite

I have verified these changes locally and seems to be working fine.

If possible, please include unit test to validate the logic.

  1. Multiple database clients for the same database w/ different SessionPoolOptions
  2. Multiple database clients for different databases.

Thanks for verifying it locally 👍 . I've added a test case for the generated client id.

mayurkale22
mayurkale22 approved these changes Mar 19, 2020
View reviewed changes

Choose a reason for hiding this comment

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

lgtm

@skuruppuskuruppu added kokoro:force-runAdd this label to force Kokoro to re-run the tests.automergeMerge the pull request once unit tests and other checks pass.labels Mar 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@skuruppuskuruppu added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@skuruppu

I don't understand why cla/google is getting stuck. This has happened each time I've run the presubmits. It's run fine on other PRs.

@skuruppuskuruppu added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@skuruppuskuruppu added kokoro:force-runAdd this label to force Kokoro to re-run the tests.cla: noThis human has *not* signed the Contributor License Agreement.and removed cla: yesThis human has signed the Contributor License Agreement.labels Mar 20, 2020
@googlebotgooglebot added cla: yesThis human has signed the Contributor License Agreement.and removed cla: noThis human has *not* signed the Contributor License Agreement.labels Mar 20, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@skuruppuskuruppu added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@yoshi-kokoroyoshi-kokoro removed theAdd this label to force Kokoro to re-run the tests.label Mar 20, 2020
@gcf-merge-on-greengcf-merge-on-green bot merged commit 338e136 into master Mar 20, 2020
@gcf-merge-on-greengcf-merge-on-green bot deleted the add-client-id-to-metrics branch March 20, 2020 03:32
gcf-merge-on-green bot pushed a commit that referenced this pull request Mar 20, 2020
@release-please
chore: release 1.52.0 (#110)
1d3c4ac
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.52.0](https://www..com/googleapis/java-spanner/compare/v1.51.0...v1.52.0) (2020-03-20)


### Features

* add backup support ([#100](https://www..com/googleapis/java-spanner/issues/100)) ([ed3874a](https://www..com/googleapis/java-spanner/commit/ed3874afcf55fe7381354e03dab3a3b97d7eb520))
* add Backups protos and APIs ([#97](https://www..com/googleapis/java-spanner/issues/97)) ([5643c22](https://www..com/googleapis/java-spanner/commit/5643c22a4531dac75b9fac5b128eb714a27920a0))


### Bug Fixes

* add client id to metrics to avoid collisions ([#117](https://www..com/googleapis/java-spanner/issues/117)) ([338e136](https://www..com/googleapis/java-spanner/commit/338e136508edc6745f9371e8a5d66638021bc8d7)), closes [#106](https://www..com/googleapis/java-spanner/issues/106)
* ignore added interface methods for generated code ([#101](https://www..com/googleapis/java-spanner/issues/101)) ([402cfa1](https://www..com/googleapis/java-spanner/commit/402cfa1e1e2994f7bb1b783cf823021b54fb175e)), closes [#99](https://www..com/googleapis/java-spanner/issues/99)
* use grpc 1.27.2 to prevent version conflicts ([#105](https://www..com/googleapis/java-spanner/issues/105)) ([37b7c88](https://www..com/googleapis/java-spanner/commit/37b7c8859e5f35d85bd14ef72662614fd185c020))


### Dependencies

* update core dependencies ([#94](https://www..com/googleapis/java-spanner/issues/94)) ([f3ca4c9](https://www..com/googleapis/java-spanner/commit/f3ca4c99c3d54f64c5eda11e4a4c076140fdbc6a))
* update opencensus.version to v0.26.0 ([#116](https://www..com/googleapis/java-spanner/issues/116)) ([1b8db0b](https://www..com/googleapis/java-spanner/commit/1b8db0b407429e02bb1e4c9af839afeed21dac5d))
---


This PR was generated with [Release Please](https://.com/googleapis/release-please).
yoshi-automation added a commit that referenced this pull request Mar 25, 2020
@yoshi-automation
fix: add client id to metrics to avoid collisions (#117)
774402c
338e136
commit 338e136
Author: Knut Olav Løite <[email protected]>
Date:   Fri Mar 20 04:32:04 2020 +0100

    fix: add client id to metrics to avoid collisions (#117)

    This PR adds an automatically generated identifier to all database clients that are created by the client library. This avoids collisions of the same metrics being registered multiple times, and makes it possible to distinguish different clients from each other in the monitoring.

    This PR does not allow the user to specify the id, but this could be added in a future change. That would need an API change by adding an overload to the method `getDatabaseClient(DatabaseId)` with an additional `clientId` parameter.

    This should also fix the build error in GoogleCloudPlatform/java-docs-samples#2323.

    Fixes #106

google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestWithClosedSessionsEnv.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java
yoshi-automation added a commit that referenced this pull request Mar 25, 2020
@yoshi-automation
chore: release 1.52.0 (#110)
b4459d4
1d3c4ac
commit 1d3c4ac
Author: release-please[bot] <55107282+release-please[bot]@users.noreply..com>
Date:   Fri Mar 20 04:10:05 2020 +0000

    chore: release 1.52.0 (#110)

    🤖 I have created a release \*beep\* \*boop\*
    ---
    ## [1.52.0](https://www..com/googleapis/java-spanner/compare/v1.51.0...v1.52.0) (2020-03-20)

    ### Features

    * add backup support ([#100](https://www..com/googleapis/java-spanner/issues/100)) ([ed3874a](https://www..com/googleapis/java-spanner/commit/ed3874afcf55fe7381354e03dab3a3b97d7eb520))
    * add Backups protos and APIs ([#97](https://www..com/googleapis/java-spanner/issues/97)) ([5643c22](https://www..com/googleapis/java-spanner/commit/5643c22a4531dac75b9fac5b128eb714a27920a0))

    ### Bug Fixes

    * add client id to metrics to avoid collisions ([#117](https://www..com/googleapis/java-spanner/issues/117)) ([338e136](https://www..com/googleapis/java-spanner/commit/338e136508edc6745f9371e8a5d66638021bc8d7)), closes [#106](https://www..com/googleapis/java-spanner/issues/106)
    * ignore added interface methods for generated code ([#101](https://www..com/googleapis/java-spanner/issues/101)) ([402cfa1](https://www..com/googleapis/java-spanner/commit/402cfa1e1e2994f7bb1b783cf823021b54fb175e)), closes [#99](https://www..com/googleapis/java-spanner/issues/99)
    * use grpc 1.27.2 to prevent version conflicts ([#105](https://www..com/googleapis/java-spanner/issues/105)) ([37b7c88](https://www..com/googleapis/java-spanner/commit/37b7c8859e5f35d85bd14ef72662614fd185c020))

    ### Dependencies

    * update core dependencies ([#94](https://www..com/googleapis/java-spanner/issues/94)) ([f3ca4c9](https://www..com/googleapis/java-spanner/commit/f3ca4c99c3d54f64c5eda11e4a4c076140fdbc6a))
    * update opencensus.version to v0.26.0 ([#116](https://www..com/googleapis/java-spanner/issues/116)) ([1b8db0b](https://www..com/googleapis/java-spanner/commit/1b8db0b407429e02bb1e4c9af839afeed21dac5d))
    ---

    This PR was generated with [Release Please](https://.com/googleapis/release-please).

CHANGELOG.md
README.md
google-cloud-spanner-bom/pom.xml
google-cloud-spanner/pom.xml
grpc-google-cloud-spanner-admin-database-v1/pom.xml
grpc-google-cloud-spanner-admin-instance-v1/pom.xml
grpc-google-cloud-spanner-v1/pom.xml
pom.xml
proto-google-cloud-spanner-admin-database-v1/pom.xml
proto-google-cloud-spanner-admin-instance-v1/pom.xml
proto-google-cloud-spanner-v1/pom.xml
versions.txt
Sign up for free to join this conversation on . Already have an account? Sign in to comment

@skuruppu skuruppu skuruppu approved these changes

@mayurkale22 mayurkale22 mayurkale22 approved these changes

No one assigned
automergeMerge the pull request once unit tests and other checks pass.cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.

Session pool metrics registered multiple times