Merge #143711

143711: sql: Queries under the InternalAppNamePrefix attribute to internal metrics. r=alyshanjahani-crl a=alyshanjahani-crl

Previously, there did not exist a way for external connections to have their
SQL metrics (ex/ `sql.insert.count`) be counted as internal queries
(ex/ `sql.insert.count.internal`).

This is specifically problematic for cli utilities like `cockroach debug zip`
which issue SQL queries that often take much longer than queries in a user's
workload. As a result, users see a spike in their p99.99 or p99.9 SQL service
latencies causing unnecessary concern.

This commit utilizes the existing constant InternalAppNamePrefix. Specifically,
it modifies the connExecutor to set the metrics struct to the server's internal
or user facing metrics according to the app name.

This commit also modifies the debug zip to use the InternalAppNamePrefix.

Fixes: #143224

Release note (ops change): debug zip queries are attributed towards internal
sql metrics. As a result user's won't see the debug zip impact on the SQL
charts in DB console.

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

Author: craig[bot]

pkg/cli/sql_client.go

@@ -136,8 +136,14 @@ func makeSQLClientForBaseURL(
 	// If there is no user in the URL already, fill in the default user.
 	sqlCtx.User = username.RootUser
 
-	// If there is no application name already, use the provided one.
-	sqlCtx.ApplicationName = catconstants.ReportableAppNamePrefix + appName
+	// Some cli utilities (ex/ debug zip) use an InternalAppNamePrefix so that
+	// they do not affect user facing SQL metrics (and as a result the SQL charts
+	// in the DB Console).
+	if strings.HasPrefix(appName, catconstants.InternalAppNamePrefix) {
+		sqlCtx.ApplicationName = appName
+	} else {
+		sqlCtx.ApplicationName = catconstants.ReportableAppNamePrefix + appName
+	}
 
 	// How we're going to authenticate.
 	usePw, _, _ := baseURL.GetAuthnPassword()

pkg/cli/zip.go

@@ -291,7 +291,7 @@ func runDebugZip(cmd *cobra.Command, args []string) (retErr error) {
 
 			zr.sqlOutputFilenameExtension = computeSQLOutputFilenameExtension(sqlExecCtx.TableDisplayFormat)
 
-			sqlConn, err := makeTenantSQLClient(ctx, "cockroach zip", useSystemDb, tenant.TenantName)
+			sqlConn, err := makeTenantSQLClient(ctx, catconstants.InternalAppNamePrefix+" cockroach zip", useSystemDb, tenant.TenantName)
 			// The zip output is sent directly into a text file, so the results should
 			// be scanned into strings.
 			_ = sqlConn.SetAlwaysInferResultTypes(false)

pkg/sql/conn_executor.go

@@ -879,14 +879,22 @@ func (s *Server) SetupConn(
 		return ConnectionHandler{}, err
 	}
 
+	// Usage of the InternalAppNamePrefix is usually done using an InternalExecutor which already is set up
+	// to use the InternalMetrics. However, some external connections use the prefix as well, for example
+	// the debug zip cli tool.
+	metrics := &s.Metrics
+	if strings.HasPrefix(sd.ApplicationName, catconstants.InternalAppNamePrefix) {
+		metrics = &s.InternalMetrics
+	}
+
 	ex := s.newConnExecutor(
 		ctx,
 		executorTypeExec,
 		sdMutIterator,
 		stmtBuf,
 		clientComm,
 		memMetrics,
-		&s.Metrics,
+		metrics,
 		s.localSqlStats.GetApplicationStats(sd.ApplicationName),
 		sessionID,
 		false, /* underOuterTxn */
@@ -1227,6 +1235,11 @@ func (s *Server) newConnExecutor(
 	ex.dataMutatorIterator.onApplicationNameChange = func(newName string) {
 		ex.applicationName.Store(newName)
 		ex.applicationStats = ex.server.localSqlStats.GetApplicationStats(newName)
+		if strings.HasPrefix(newName, catconstants.InternalAppNamePrefix) {
+			ex.metrics = &ex.server.InternalMetrics
+		} else {
+			ex.metrics = &ex.server.Metrics
+		}
 	}
 
 	ex.extraTxnState.underOuterTxn = underOuterTxn

pkg/sql/conn_executor_test.go

@@ -37,6 +37,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowexec"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness"
@@ -2420,3 +2421,85 @@ SELECT id
 	).Scan(&id)
 	return id
 }
+
+func TestInternalAppNamePrefix(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	params := base.TestServerArgs{}
+	params.Insecure = true
+	s, sqlDB, _ := serverutils.StartServer(t, params)
+	defer s.Stopper().Stop(ctx)
+
+	// Create a test table.
+	_, err := sqlDB.Exec("CREATE TABLE test (k INT PRIMARY KEY, v INT)")
+	require.NoError(t, err)
+
+	t.Run("app name set at conn init", func(t *testing.T) {
+		// Create a connection.
+		connURL := url.URL{
+			Scheme: "postgres",
+			User:   url.User(username.RootUser),
+			Host:   s.AdvSQLAddr(),
+		}
+		q := connURL.Query()
+		q.Add("sslmode", "disable")
+		q.Add("application_name", catconstants.InternalAppNamePrefix+"mytest")
+		connURL.RawQuery = q.Encode()
+		db, err := gosql.Open("postgres", connURL.String())
+		require.NoError(t, err)
+		defer db.Close()
+		runner := sqlutils.MakeSQLRunner(db)
+
+		// Get initial metric values
+		sqlServer := s.SQLServer().(*sql.Server)
+		initialInternalMetrics := sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
+		initialUserMetrics := sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
+		runner.Exec(t, "INSERT into test values (1, 1)")
+		// Confirm only internal metrics increased.
+		finalInternalMetrics := sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
+		finalUserMetrics := sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
+		require.Equal(t, initialUserMetrics, finalUserMetrics)
+		require.Equal(t, initialInternalMetrics+1, finalInternalMetrics)
+	})
+
+	t.Run("app name set in session", func(t *testing.T) {
+		// Create a connection.
+		connURL := url.URL{
+			Scheme:   "postgres",
+			User:     url.User(username.RootUser),
+			Host:     s.AdvSQLAddr(),
+			RawQuery: "sslmode=disable",
+		}
+		db, err := gosql.Open("postgres", connURL.String())
+		require.NoError(t, err)
+		defer db.Close()
+		runner := sqlutils.MakeSQLRunner(db)
+
+		// Get initial metric values
+		sqlServer := s.SQLServer().(*sql.Server)
+		initialInternalMetrics := sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
+		initialUserMetrics := sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
+
+		// Set app name to attribute query towards internal metrics.
+		runner.Exec(t, fmt.Sprintf("set application_name='%v'", catconstants.InternalAppNamePrefix+"mytest"))
+		runner.Exec(t, "INSERT into test values (2, 1)")
+
+		// Confirm only internal metrics increased.
+		finalInternalMetrics := sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
+		finalUserMetrics := sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
+		require.Equal(t, initialUserMetrics, finalUserMetrics)
+		require.Equal(t, initialInternalMetrics+1, finalInternalMetrics)
+
+		// Reset app name.
+		runner.Exec(t, "set application_name='mytest'")
+		runner.Exec(t, "INSERT into test values (3, 1)")
+
+		// Confirm only user metrics increased.
+		finalInternalMetrics = sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
+		finalUserMetrics = sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
+		require.Equal(t, initialUserMetrics+1, finalUserMetrics)
+		require.Equal(t, initialInternalMetrics+1, finalInternalMetrics)
+	})
+}

pkg/sql/sem/catconstants/constants.go

@@ -20,7 +20,7 @@ const InternalAppNamePrefix = ReportableAppNamePrefix + "internal"
 
 // AttributedToUserInternalAppNamePrefix indicates that the application name
 // identifies an internally-executed query that should be attributed to the
-// user.
+// user. Specifically, this means having the queries show up in SQL activity pages.
 const AttributedToUserInternalAppNamePrefix = ReportableAppNamePrefix + "public-internal"
 
 // DelegatedAppNamePrefix is added to a regular client application