feat: add num_sessions_in_pool metric #128

mayurkale22 · 2020-03-25T23:44:44Z

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)

Updates #53, this is based on the last discussion in the thread: #53 (comment)

This is how it looks in Stackdriver UI:

Breaking Note:
The cloud.google.com/java/spanner/in_use_sessions metric has been deprecated since v1.55.2, use cloud.google.com/java/spanner/num_sessions_in_pool instead.

skuruppu

LGTM

My one concern is about removing metrics. Since we haven't had any metrics before, we don't have a process for it right now but open to hearing suggestions on how people do this in other products.

skuruppu · 2020-03-26T03:39:48Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java

@@ -46,20 +55,20 @@
  // The Metric name and description
  static final String MAX_IN_USE_SESSIONS = "cloud.google.com/java/spanner/max_in_use_sessions";
  static final String MAX_ALLOWED_SESSIONS = "cloud.google.com/java/spanner/max_allowed_sessions";
-  static final String IN_USE_SESSIONS = "cloud.google.com/java/spanner/in_use_sessions";


I'm worried for anyone relying on this metric already in their monitoring? What is the process for deprecating metrics in OpenCensus? Is there some monitoring to figure out if someone is exporting it?

What is the process for deprecating metrics in OpenCensus?

In OC, (usually) we first mark the existing API as deprecated and keep it for 18 months before removing it. I think the feature is fairly new and looks safe to remove it with a note in the Changelog.

Is there some monitoring to figure out if someone is exporting it?

I think it is feasible to see the ingested metrics in stackdriver, but I haven't done it personally.

I'm worried for anyone relying on this metric already in their monitoring?

Another option is keep existing(IN_USE_SESSIONS) metric along with new metric. In this case, in_use_sessions data will be accounted/exported twice.

WDYT?

Ok, I think you're right that this is relatively new. We will mark this in the Changelog.

If you can include something about this in the PR description, that would be great. Then we can copy it in when doing the release.

olavloite

LGTM.
One thing we should keep in the back of our heads is that sum of these new metrics could vary a little bit, even in a situation where the actual number of sessions in the pool is steady. Each individual value is read independently of each other, and it is possible that a session is counted twice or not at all, for example when it is transferred from numSessionsBeingPrepared to numWritePreparedSessions. That could cause some confusion for users, especially when they have configured the session pool with MinSessions=MaxSessions. In that case you would expect the number of sessions in the pool to always be constant.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java

rghetia · 2020-03-27T00:02:32Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java

@@ -1638,7 +1637,6 @@ public Void call() {
        .containsEntry(MetricRegistryConstants.NUM_ACQUIRED_SESSIONS, 3L);
    assertThat(record.getMetrics())
        .containsEntry(MetricRegistryConstants.NUM_RELEASED_SESSIONS, 3L);
-    assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.IN_USE_SESSIONS, 0L);


No test for new Gauges?

Added test to validate newly added Metric. I had to modify MetricRegistryTestUtils to support and test multiple timeseries entries. PTAL when you get a chance.

hengfengli · 2020-05-26T01:26:31Z

@mayurkale22 I just want to check what is latest status of this PR? any progress?

mayurkale22 · 2020-05-26T17:59:48Z

@mayurkale22 I just want to check what is latest status of this PR? any progress?

Oops! This went off my radar due to other priorities. I'm going to rebase the PR and fix unresolved comments soon.

Thanks for bringing this to my attention.

mayurkale22 · 2020-06-01T21:31:49Z

@skuruppu @olavloite do you know why build is failing (Kokoro - Test: Integration), looks unrelated to my change.

olavloite · 2020-06-02T06:25:57Z

@mayurkale22 That is unfortunately an (infrastructure) problem that we are running into on a semi-regular basis. It has nothing to do with your change, and the only way to circumvent it at the moment is triggering a new build, for example by adding the kokoro:force-run label to the PR.

codecov · 2020-06-04T07:21:35Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1ec0f34). Click here to learn what that means.
The diff coverage is n/a.

skuruppu · 2020-06-04T09:49:47Z

@olavloite, the latest failure is interesting:

com.google.cloud.spanner.SpannerException: INVALID_ARGUMENT: com.google.api.gax.rpc.InvalidArgumentException: io.grpc.StatusRuntimeException: INVALID_ARGUMENT: Query optimizer version: 10000 is not supported

would you be able to take a quick look?

olavloite · 2020-06-04T09:57:01Z

@olavloite, the latest failure is interesting:

com.google.cloud.spanner.SpannerException: INVALID_ARGUMENT: com.google.api.gax.rpc.InvalidArgumentException: io.grpc.StatusRuntimeException: INVALID_ARGUMENT: Query optimizer version: 10000 is not supported

would you be able to take a quick look?

Hmmm... That's strange. There has been no changes to the test case lately, so my initial guess would be that the backend is now reacting differently. Up until now the backend accepted query options to be specified for DML statements, but seemed to ignore them. Now it seems that they are at least checked for a valid value.

skuruppu · 2020-06-04T09:59:45Z

That's interesting. I also wondered why this suddenly came up given that we haven't touched this code in a couple of months.

olavloite · 2020-06-04T10:14:24Z

That's interesting. I also wondered why this suddenly came up given that we haven't touched this code in a couple of months.

I think we should just remove this specific test case for now, and ask the backend team what the expected behavior of QueryOptions should be for DML statements.

…s to v1.18.0 (googleapis#128) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [com.google.api.grpc:proto-google-common-protos](https://togithub.com/googleapis/java-iam) | minor | `1.17.0` -> `1.18.0` | --- ### Renovate configuration :date: **Schedule**: At any time (no schedule defined). :vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied. :recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. :no_bell: **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/java-spanner-jdbc).

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2020

mayurkale22 requested review from skuruppu, olavloite and rghetia and removed request for skuruppu March 25, 2020 23:44

mayurkale22 changed the title ~~add num_sessions_in_pool metric~~ feat: add num_sessions_in_pool metric Mar 25, 2020

skuruppu approved these changes Mar 26, 2020

View reviewed changes

olavloite approved these changes Mar 26, 2020

View reviewed changes

rghetia reviewed Mar 27, 2020

View reviewed changes

mayurkale22 mentioned this pull request May 27, 2020

chore: document OpenCensus Metrics #244

Merged

rghetia approved these changes May 29, 2020

View reviewed changes

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2020

mayurkale22 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2020

hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 3, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 3, 2020

mayurkale22 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 4, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 4, 2020

mayurkale22 added 2 commits June 4, 2020 00:20

feat: add num_sessions_in_pool metric

cf3981b

add tests

b5ac645

skuruppu added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2020

skuruppu merged commit 3a7a8ad into googleapis:master Jun 11, 2020

mayurkale22 mentioned this pull request Aug 31, 2020

chore: fix OpenCensus metrics list #408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add num_sessions_in_pool metric #128

feat: add num_sessions_in_pool metric #128

mayurkale22 commented Mar 25, 2020 •

edited

skuruppu left a comment

skuruppu Mar 26, 2020

mayurkale22 May 27, 2020

skuruppu May 29, 2020

skuruppu May 29, 2020

mayurkale22 May 29, 2020

olavloite left a comment

rghetia Mar 27, 2020

mayurkale22 May 27, 2020

hengfengli commented May 26, 2020

mayurkale22 commented May 26, 2020

mayurkale22 commented Jun 1, 2020

olavloite commented Jun 2, 2020

codecov bot commented Jun 4, 2020

skuruppu commented Jun 4, 2020

olavloite commented Jun 4, 2020

skuruppu commented Jun 4, 2020

olavloite commented Jun 4, 2020

feat: add num_sessions_in_pool metric #128

feat: add num_sessions_in_pool metric #128

Conversation

mayurkale22 commented Mar 25, 2020 • edited

skuruppu left a comment

Choose a reason for hiding this comment

skuruppu Mar 26, 2020

Choose a reason for hiding this comment

mayurkale22 May 27, 2020

Choose a reason for hiding this comment

skuruppu May 29, 2020

Choose a reason for hiding this comment

skuruppu May 29, 2020

Choose a reason for hiding this comment

mayurkale22 May 29, 2020

Choose a reason for hiding this comment

olavloite left a comment

Choose a reason for hiding this comment

rghetia Mar 27, 2020

Choose a reason for hiding this comment

mayurkale22 May 27, 2020

Choose a reason for hiding this comment

hengfengli commented May 26, 2020

mayurkale22 commented May 26, 2020

mayurkale22 commented Jun 1, 2020

olavloite commented Jun 2, 2020

codecov bot commented Jun 4, 2020

Codecov Report

skuruppu commented Jun 4, 2020

olavloite commented Jun 4, 2020

skuruppu commented Jun 4, 2020

olavloite commented Jun 4, 2020

mayurkale22 commented Mar 25, 2020 •

edited