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

Session pool metrics registered multiple times #106

Closed
olavloite opened this issue Mar 15, 2020 · 5 comments · Fixed by #117
Closed

Session pool metrics registered multiple times #106

olavloite opened this issue Mar 15, 2020 · 5 comments · Fixed by #117
Assignees
Labels
api: spanner priority: p1 🚨 type: bug

Comments

@olavloite
Copy link
Contributor

@olavloite olavloite commented Mar 15, 2020

The new metrics that have been added for the SessionPool are added multiple times if multiple different session pools are created by a Spanner instance. This could happen if the user creates database clients for different databases, or if multiple database clients using different SessionPoolOptions are requested. This again will lead to an InvalidArgumentException being thrown. See for example https://source.cloud.google.com/results/invocations/a8dc0694-336f-483b-bb8f-2ee502506a2b/targets/cloud-devrel%2Fjava%2Fjava-docs-samples%2Fjava8%2Fpresubmit/log

@olavloite olavloite added type: bug priority: p1 api: spanner labels Mar 15, 2020
@olavloite olavloite self-assigned this Mar 15, 2020
olavloite added a commit that referenced this issue Mar 15, 2020
The default MetricsRegistry used by OpenCensus is a singleton that does
not allow the same metrics to be registered multiple times. The session
pool metrics gathering did not take this into account, and when multiple
session pools were created within the same class loader, it would throw
an InvalidArgumentException for the second session pool.

Fixes #106.
@mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Mar 16, 2020

Copying discussion from OpenCensus PR census-instrumentation/opencensus-java#2017 (comment):

The only concern that I still have is what it looks like in the dashboards when there are multiple different instances of a SessionPool with the same label values. I understand that it won't cause any errors in the client library, but how is it presented to the user? This situation would occur when an application is opening two connections to the same database with different SessionPoolOptions. This would also mean different values for constant metrics like MaxSessions.

@mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Mar 16, 2020

(Thinking out loud) This is a great point. Yes, you're correct, it won't cause any errors in the client library, but as we are having same label values for multiple database clients for the same database (w/ different SessionPoolOptions), the collected metrics would mix with each other and would look messy. Also, it would be challenging/impossible for the users to make any sense of it. I am not sure how to handle this scenario, please let me know if you have any suggestion.

+@rghetia

@rghetia
Copy link

@rghetia rghetia commented Mar 16, 2020

One option is to create two more labels.

  1. SessionPoolID - UUID for each sessionPool.
  2. SessionPoolOptionHash - Hash value computed from all session options.

/1 allows to keep all stats separate.
/2 allows to group stats on the backend (stackdriver) with same options (if hash has no collision).

Users could customize their views to include /1 and/or /2 as necessary.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Mar 17, 2020

One option is to create two more labels.

  1. SessionPoolID - UUID for each sessionPool.
  2. SessionPoolOptionHash - Hash value computed from all session options.

/1 allows to keep all stats separate.
/2 allows to group stats on the backend (stackdriver) with same options (if hash has no collision).

Users could customize their views to include /1 and/or /2 as necessary.

That sounds reasonable to me. The only thing that concerns me a little bit is that a user will have no idea which SessionPoolID and which SessionPoolOptionHash values belong to which client, so if there's a problem, it will still be very difficult to figure out which client is causing the problem. Or is there some way that a user can set something like an ApplicationID or ClientID in OpenCensus, which indicates 'these metrics are all coming from this application/client'?

@rghetia
Copy link

@rghetia rghetia commented Mar 18, 2020

That sounds reasonable to me. The only thing that concerns me a little bit is that a user will have no idea which SessionPoolID and which SessionPoolOptionHash values belong to which client, so if there's a problem, it will still be very difficult to figure out which client is causing the problem. Or is there some way that a user can set something like an ApplicationID or ClientID in OpenCensus, which indicates 'these metrics are all coming from this application/client'?

Ideally, it would be nice if user provides such metadata as part of creating client-connection/session-pool which can then be used as regular label just like SessionPoolID (above). Not sure if such metadata exists.
In the absence of such metadata, application-owners could use constant labels. Not ideal as it requires one more thing to do for the app-owners.

gcf-merge-on-green bot pushed a commit that referenced this issue Mar 20, 2020
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
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 20, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.52.0](https://www.github.com/googleapis/java-spanner/compare/v1.51.0...v1.52.0) (2020-03-20)


### Features

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


### Bug Fixes

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


### Dependencies

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


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
yoshi-automation added a commit that referenced this issue Mar 25, 2020
338e136
commit 338e136
Author: Knut Olav Løite <koloite@gmail.com>
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 issue Mar 25, 2020
1d3c4ac
commit 1d3c4ac
Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.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.github.com/googleapis/java-spanner/compare/v1.51.0...v1.52.0) (2020-03-20)

    ### Features

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

    ### Bug Fixes

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

    ### Dependencies

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

    This PR was generated with [Release Please](https://github.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
@yoshi-automation yoshi-automation added the 🚨 label Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner priority: p1 🚨 type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants