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

spanner: add oc tests for session pool metrics. #2416

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

hengfengli
Copy link
Contributor

The tests verify the correctness of outputting session-pool metrics.

@hengfengli hengfengli added the api: spanner Issues related to the Spanner API. label Jun 10, 2020
@hengfengli hengfengli self-assigned this Jun 10, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 10, 2020
@hengfengli
Copy link
Contributor Author

@olavloite Thanks for reviewing this CL on Gerrit. I have moved it here and will fix all your comments.

@hengfengli
Copy link
Contributor Author

@olavloite

Patch Set #4, Line 36: t.Skip("TestExporter is not thread safe")

Not really related to this change, but I noticed it now: This problem was as far as I can remember fixed in https://code-review.googlesource.com/c/gocloud/+/54970, so I guess we can also enable this test as well.

Done.

Patch Set #4, Line 71: for _, test := range []struct {
Could this test be parallel?

No, I don't think so because it needs to register views a global view.

Patch Set #4, Line 214: server.TestSpanner.PutExecutionTime(stestutil.MethodGetSession,

As far as I can tell, GetSession will not be called by the code that follows in this test case. The client.Single().ReadRow(..) statement will create a session, but it will not call GetSession.

The get_session_timeouts statistic relates to the number of times the sessionPool.getReadSession(..) method will time out, but that method will never call the server method GetSession. Instead it will either wait for a session to come available when another process returns a session, or it will call (Batch)CreateSession(s) to create a new session. So in this case I would think that it would make more sense to set the execution time on one of those methods.

I may copy this from an existing test somewhere. I have deleted it.

row := stat.Rows[0]
m := getTagMap(row.Tags)
checkCommonTags(t, m)
if got, want := fmt.Sprintf("%v", row.Data), "&{1}"; got != want {
Copy link
Contributor

Choose a reason for hiding this comment

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

While it is highly likely that this is true, I'm a little bit afraid that this check might turn out to be flaky. The test uses a context with a timeout of 1ms, which means that it is highly likely to timeout, but there is no real guarantee that it will happen while waiting for a session. I think that adding a simulated execution time of 2ms for the BatchCreateSessions on the mock server would guarantee it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Done.

@@ -3,7 +3,7 @@ module cloud.google.com/go/spanner
go 1.11

require (
cloud.google.com/go v0.57.0
cloud.google.com/go v0.58.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I made some changes in https://github.com/googleapis/google-cloud-go/blob/master/internal/testutil/trace.go, which are included in v.0.58.0.

Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

LGTM once the merge conflicts are solved.

The tests verify the correctness of outputting session-pool metrics.

Change-Id: I97aba7a9457078f0fdf5e8a542fe6095451dab95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants