Merge #153366

153366: serverccl, application_api: Fix some tests relying on statement_statistics r=alyshanjahani-crl a=alyshanjahani-crl

This commit sets the SynchronousSQLStats testing knob on for TestTenantCannotSeeNonTenantStats.

Additionally, it fixes a testing bug in TestTenantCannotSeeNonTenantStats, TestStatusAPIStatements, TestStatusAPICombinedStatementsWithFullScans, and TestStatusAPICombinedStatements.

These tests involve executing a set of statements and verifying that the statement stats response contains the same set of statements. However, they filter out statements with a failure count > 0.

This was a bug introduced by pull #120236 which changed the way failed statements were collected in the SQL stats system. Previously, a stmt with the same query that failed to execute would recieve a different fingerprint and thus be included as a different entry in the statements response.

These existing tests filtered out statement entries that failed b/c they used to be an additional entry that would fail when comparing with the set of stmts executed. This filtering out from the tests should have been removed as part of pull #120236.

Fixes: #152175
Release note: None

Co-authored-by: Alyshan Jahani <[email protected]>

Author: craig[bot]

pkg/ccl/serverccl/statusccl/tenant_status_test.go

@@ -402,12 +402,16 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
 	skip.UnderStressWithIssue(t, 113984)
 
 	ctx := context.Background()
+	sqlStatsKnobs := sqlstats.CreateTestingKnobs()
+	sqlStatsKnobs.SynchronousSQLStats = true
 	testCluster := serverutils.StartCluster(t, 3 /* numNodes */, base.TestClusterArgs{
 		ServerArgs: base.TestServerArgs{
 			Knobs: base.TestingKnobs{
+				SQLStatsKnobs: sqlStatsKnobs,
 				SpanConfig: &spanconfig.TestingKnobs{
 					ManagerDisableJobCreation: true, // TODO(irfansharif): #74919.
-				}},
+				},
+			},
 			DefaultTestTenant: base.TestControlsTenantsExplicitly,
 		},
 	})
@@ -418,7 +422,7 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
 	tenant, sqlDB := serverutils.StartTenant(t, server, base.TestTenantArgs{
 		TenantID: roachpb.MustMakeTenantID(10 /* id */),
 		TestingKnobs: base.TestingKnobs{
-			SQLStatsKnobs: sqlstats.CreateTestingKnobs(),
+			SQLStatsKnobs: sqlStatsKnobs,
 		},
 	})
 
@@ -478,7 +482,7 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
 	}
 
 	conn = sqlutils.MakeSQLRunner(systemLayer.SQLConn(t))
-	sqlstatstestutil.WaitForStatementEntriesAtLeast(t, conn, len(testCaseTenant), sqlstatstestutil.StatementFilter{
+	sqlstatstestutil.WaitForStatementEntriesAtLeast(t, conn, len(testCaseNonTenant), sqlstatstestutil.StatementFilter{
 		App: appName,
 	})
 
@@ -524,11 +528,6 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
 
 		var actualStatements []string
 		for _, respStatement := range actual.Statements {
-			if respStatement.Stats.FailureCount > 0 {
-				// We ignore failed statements here as the INSERT statement can fail and
-				// be automatically retried, confusing the test success check.
-				continue
-			}
 			if respStatement.Key.KeyData.App != appName {
 				continue
 			}

pkg/server/application_api/sql_stats_test.go

@@ -464,11 +464,6 @@ func TestStatusAPIStatements(t *testing.T) {
 		// See if the statements returned are what we executed.
 		var statementsInResponse []string
 		for _, respStatement := range resp.Statements {
-			if respStatement.Stats.FailureCount > 0 {
-				// We ignore failed statements here as the INSERT statement can fail and
-				// be automatically retried, confusing the test success check.
-				continue
-			}
 			if respStatement.Key.KeyData.App != "test" {
 				continue
 			}
@@ -807,14 +802,7 @@ func TestStatusAPICombinedStatementsWithFullScans(t *testing.T) {
 		// statement statistics received from the server response.
 		actualResponseStatsMap := make(map[string]serverpb.StatementsResponse_CollectedStatementStatistics)
 		for _, respStatement := range resp.Statements {
-			// Skip failed statements: The test app may encounter transient 40001
-			// errors that are automatically retried. Thus, we only consider
-			// statements that were that were successfully executed by the test app
-			// to avoid counting such failures. If a statement that we expect to be
-			// successful is not found in the response, the test will fail later.
-			if respStatement.Key.KeyData.App == testAppName && respStatement.Stats.FailureCount == 0 {
-				actualResponseStatsMap[respStatement.Key.KeyData.Query] = respStatement
-			}
+			actualResponseStatsMap[respStatement.Key.KeyData.Query] = respStatement
 		}
 
 		for respQuery, expectedData := range expectedStatementStatsMap {
@@ -927,11 +915,6 @@ func TestStatusAPICombinedStatements(t *testing.T) {
 		var statementsInResponse []string
 		expectedTxnFingerprints := map[appstatspb.TransactionFingerprintID]struct{}{}
 		for _, respStatement := range resp.Statements {
-			if respStatement.Stats.FailureCount > 0 {
-				// We ignore failed statements here as the INSERT statement can fail and
-				// be automatically retried, confusing the test success check.
-				continue
-			}
 			if respStatement.Key.KeyData.App != "test" {
 				// CombinedStatementStats should filter out internal queries.
 				if strings.HasPrefix(respStatement.Key.KeyData.Query, catconstants.InternalAppNamePrefix) {