serverccl, application_api: Fix some tests relying on statement_statistics

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

Author: alyshanjahani-crl

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) {