From f6cda0407b054c127260efec7f435776d300948a Mon Sep 17 00:00:00 2001 From: gtr Date: Wed, 23 Aug 2023 16:22:12 -0400 Subject: [PATCH] sqlstats: unskip tests hitting combinedstmts and stmts endpoints Fixes: #109184. Previously, tests which hit the `combinedstmts` and `statements` endpoints were skipped under stress because they would occaisonally fail. This commit unskips these tests and additonally introduces a new function `GetStatusJSONProtoWithAdminAndTimeoutOption` which provides a parameter to add additional time to the http client's timeout. The default timeout is 10 seconds. 20 seconds is added for tests executed under stress, which should be enough time for these tests to pass. Release note: None --- pkg/server/application_api/sql_stats_test.go | 59 +++++++++++++------ pkg/server/srvtestutils/testutils.go | 13 ++++ pkg/testutils/serverutils/test_server_shim.go | 23 ++++++++ 3 files changed, 78 insertions(+), 17 deletions(-) diff --git a/pkg/server/application_api/sql_stats_test.go b/pkg/server/application_api/sql_stats_test.go index cbf3a895cd6e..1f930e9d4c70 100644 --- a/pkg/server/application_api/sql_stats_test.go +++ b/pkg/server/application_api/sql_stats_test.go @@ -13,6 +13,7 @@ package application_api_test import ( "context" gosql "database/sql" + "encoding/json" "fmt" "net/url" "reflect" @@ -48,8 +49,11 @@ func TestStatusAPICombinedTransactions(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // Skip under stress until we extend the timeout for the http client. - skip.UnderStressWithIssue(t, 109184) + // Increase the timeout for the http client if under stress. + additionalTimeout := 0 * time.Second + if skip.Stress() { + additionalTimeout = 20 * time.Second + } var params base.TestServerArgs params.Knobs.SpanConfig = &spanconfig.TestingKnobs{ManagerDisableJobCreation: true} // TODO(irfansharif): #74919. @@ -128,7 +132,7 @@ func TestStatusAPICombinedTransactions(t *testing.T) { // Hit query endpoint. var resp serverpb.StatementsResponse - if err := srvtestutils.GetStatusJSONProto(firstServerProto, "combinedstmts", &resp); err != nil { + if err := srvtestutils.GetStatusJSONProtoWithAdminAndTimeoutOption(firstServerProto, "combinedstmts", &resp, true, additionalTimeout); err != nil { t.Fatal(err) } @@ -190,6 +194,12 @@ func TestStatusAPITransactions(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + // Increase the timeout for the http client if under stress. + additionalTimeout := 0 * time.Second + if skip.Stress() { + additionalTimeout = 20 * time.Second + } + testCluster := serverutils.StartCluster(t, 3, base.TestClusterArgs{}) ctx := context.Background() defer testCluster.Stopper().Stop(ctx) @@ -263,7 +273,7 @@ func TestStatusAPITransactions(t *testing.T) { // Hit query endpoint. var resp serverpb.StatementsResponse - if err := srvtestutils.GetStatusJSONProto(firstServerProto, "statements", &resp); err != nil { + if err := srvtestutils.GetStatusJSONProtoWithAdminAndTimeoutOption(firstServerProto, "statements", &resp, true, additionalTimeout); err != nil { t.Fatal(err) } @@ -383,8 +393,11 @@ func TestStatusAPIStatements(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // Skip under stress until we extend the timeout for the http client. - skip.UnderStressWithIssue(t, 109184) + // Increase the timeout for the http client if under stress. + additionalTimeout := 0 * time.Second + if skip.Stress() { + additionalTimeout = 20 * time.Second + } // Aug 30 2021 19:50:00 GMT+0000 aggregatedTs := int64(1630353000) @@ -432,7 +445,7 @@ func TestStatusAPIStatements(t *testing.T) { testPath := func(path string, expectedStmts []string) { // Hit query endpoint. - if err := srvtestutils.GetStatusJSONProtoWithAdminOption(firstServerProto, path, &resp, false); err != nil { + if err := srvtestutils.GetStatusJSONProtoWithAdminAndTimeoutOption(firstServerProto, path, &resp, false, additionalTimeout); err != nil { t.Fatal(err) } @@ -497,8 +510,11 @@ func TestStatusAPICombinedStatementsWithFullScans(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // Skip under stress until we extend the timeout for the http client. - skip.UnderStressWithIssue(t, 109184) + // Increase the timeout for the http client if under stress. + additionalTimeout := 0 * time.Second + if skip.Stress() { + additionalTimeout = 20 * time.Second + } // Aug 30 2021 19:50:00 GMT+0000 aggregatedTs := int64(1630353000) @@ -591,7 +607,7 @@ func TestStatusAPICombinedStatementsWithFullScans(t *testing.T) { } verifyCombinedStmtStats := func() { - err := srvtestutils.GetStatusJSONProtoWithAdminOption(firstServerProto, endpoint, &resp, false) + err := srvtestutils.GetStatusJSONProtoWithAdminAndTimeoutOption(firstServerProto, endpoint, &resp, false, additionalTimeout) require.NoError(t, err) for _, respStatement := range resp.Statements { @@ -607,10 +623,16 @@ func TestStatusAPICombinedStatementsWithFullScans(t *testing.T) { continue } - require.Equal(t, expectedData.fullScan, actualFullScan) - require.Equal(t, expectedData.distSQL, actualDistSQL) - require.Equal(t, expectedData.failed, actualFailed) - require.Equal(t, expectedData.count, int(actualCount)) + bytes, err := json.Marshal(respStatement) + if err != nil { + t.Fatal(err) + } + respString := (string(bytes)) + + require.Equal(t, expectedData.fullScan, actualFullScan, "failed for respStatement: %v", respString) + require.Equal(t, expectedData.distSQL, actualDistSQL, "failed for respStatement: %v", respString) + require.Equal(t, expectedData.failed, actualFailed, "failed for respStatement: %v", respString) + require.Equal(t, expectedData.count, int(actualCount), "failed for respStatement: %v", respString) } } @@ -822,8 +844,11 @@ func TestStatusAPICombinedStatements(t *testing.T) { func TestStatusAPIStatementDetails(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // The liveness session might expire before the stress race can finish. - skip.UnderStressRace(t, "expensive tests") + // Increase the timeout for the http client if under stress. + additionalTimeout := 0 * time.Second + if skip.Stress() { + additionalTimeout = 20 * time.Second + } // Aug 30 2021 19:50:00 GMT+0000 aggregatedTs := int64(1630353000) @@ -882,7 +907,7 @@ func TestStatusAPIStatementDetails(t *testing.T) { } testPath := func(path string, expected resultValues) { - err := srvtestutils.GetStatusJSONProtoWithAdminOption(firstServerProto, path, &resp, false) + err := srvtestutils.GetStatusJSONProtoWithAdminAndTimeoutOption(firstServerProto, path, &resp, false, additionalTimeout) require.NoError(t, err) require.Equal(t, int64(expected.totalCount), resp.Statement.Stats.Count) require.Equal(t, expected.aggregatedTsCount, len(resp.StatementStatisticsPerAggregatedTs)) diff --git a/pkg/server/srvtestutils/testutils.go b/pkg/server/srvtestutils/testutils.go index 5beab3e79a39..5801088e9579 100644 --- a/pkg/server/srvtestutils/testutils.go +++ b/pkg/server/srvtestutils/testutils.go @@ -13,6 +13,7 @@ package srvtestutils import ( "encoding/json" "io" + "time" "github.com/cockroachdb/cockroach/pkg/server/apiconstants" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" @@ -85,6 +86,18 @@ func GetStatusJSONProtoWithAdminOption( return serverutils.GetJSONProtoWithAdminOption(ts, apiconstants.StatusPrefix+path, response, isAdmin) } +// GetStatusJSONProtoWithAdminAndTimeoutOption is similar to GetStatusJSONProtoWithAdminOption, but +// the caller can specify an additional timeout duration for the request. +func GetStatusJSONProtoWithAdminAndTimeoutOption( + ts serverutils.ApplicationLayerInterface, + path string, + response protoutil.Message, + isAdmin bool, + additionalTimeout time.Duration, +) error { + return serverutils.GetJSONProtoWithAdminAndTimeoutOption(ts, apiconstants.StatusPrefix+path, response, isAdmin, additionalTimeout) +} + // PostStatusJSONProtoWithAdminOption performs a RPC-over-HTTP request to // the status endpoint and unmarshals the response into the specified // proto message. It allows the caller to control whether the request diff --git a/pkg/testutils/serverutils/test_server_shim.go b/pkg/testutils/serverutils/test_server_shim.go index 6ab76fbc7653..10c8f9b0c96e 100644 --- a/pkg/testutils/serverutils/test_server_shim.go +++ b/pkg/testutils/serverutils/test_server_shim.go @@ -23,6 +23,7 @@ import ( "net/url" "strconv" "strings" + "time" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/kv" @@ -332,6 +333,28 @@ func GetJSONProtoWithAdminOption( return httputil.GetJSON(httpClient, fullURL, response) } +// GetJSONProtoWithAdminAndTimeoutOption is like GetJSONProtoWithAdminOption but +// the caller can specify an additional timeout duration for the request. The +// default is 10s. +func GetJSONProtoWithAdminAndTimeoutOption( + ts ApplicationLayerInterface, + path string, + response protoutil.Message, + isAdmin bool, + additionalTimeout time.Duration, +) error { + httpClient, err := ts.GetAuthenticatedHTTPClient(isAdmin, SingleTenantSession) + if err != nil { + return err + } + httpClient.Timeout += additionalTimeout + u := ts.AdminURL().String() + fullURL := u + path + log.Infof(context.Background(), "test retrieving protobuf over HTTP: %s", fullURL) + log.Infof(context.Background(), "set HTTP client timeout to: %s", httpClient.Timeout) + return httputil.GetJSON(httpClient, fullURL, response) +} + // PostJSONProto uses the supplied client to POST the URL specified by the parameters // and unmarshals the result into response. func PostJSONProto(