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

feat: add num_sessions_in_pool metric #128

Merged
merged 2 commits into from Jun 11, 2020

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Mar 25, 2020

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:
Screen Shot 2020-03-25 at 1 42 39 PM

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.

@googlebot googlebot added the cla: yes label Mar 25, 2020
@mayurkale22 mayurkale22 requested review from skuruppu, olavloite and rghetia and removed request for skuruppu Mar 25, 2020
@mayurkale22 mayurkale22 changed the title add num_sessions_in_pool metric feat: add num_sessions_in_pool metric Mar 25, 2020
Copy link
Contributor

@skuruppu skuruppu left a comment

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.

@@ -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";
Copy link
Contributor

@skuruppu skuruppu Mar 26, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@mayurkale22 mayurkale22 May 27, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@skuruppu skuruppu May 29, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@skuruppu skuruppu May 29, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@mayurkale22 mayurkale22 May 29, 2020

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@olavloite olavloite left a comment

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.

@mayurkale22 mayurkale22 force-pushed the sessions_in_pool_metric branch 2 times, most recently from 9df0fea to 48e8ec9 Compare Mar 26, 2020
@@ -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);
Copy link

@rghetia rghetia Mar 27, 2020

Choose a reason for hiding this comment

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

No test for new Gauges?

Copy link
Member Author

@mayurkale22 mayurkale22 May 27, 2020

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@hengfengli hengfengli commented May 26, 2020

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

@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented May 26, 2020

@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
Copy link
Member Author

@mayurkale22 mayurkale22 commented Jun 1, 2020

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

@olavloite olavloite added the kokoro:force-run label Jun 2, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jun 2, 2020
@olavloite
Copy link
Contributor

@olavloite olavloite commented Jun 2, 2020

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

@mayurkale22 mayurkale22 added the kokoro:force-run label Jun 2, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jun 2, 2020
@hengfengli hengfengli added the kokoro:force-run label Jun 3, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jun 3, 2020
@mayurkale22 mayurkale22 added the kokoro:force-run label Jun 4, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jun 4, 2020
@mayurkale22 mayurkale22 force-pushed the sessions_in_pool_metric branch from b5af757 to b5ac645 Compare Jun 4, 2020
@codecov
Copy link

@codecov codecov bot commented Jun 4, 2020

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
Copy link
Contributor

@skuruppu skuruppu commented Jun 4, 2020

@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
Copy link
Contributor

@olavloite olavloite commented Jun 4, 2020

@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
Copy link
Contributor

@skuruppu skuruppu commented Jun 4, 2020

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
Copy link
Contributor

@olavloite olavloite commented Jun 4, 2020

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.

@skuruppu skuruppu added the kokoro:force-run label Jun 11, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jun 11, 2020
@skuruppu skuruppu merged commit 3a7a8ad into googleapis:master Jun 11, 2020
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants