sql: add sql shell telemetry counter

[e-mbrown]

Fixes #62208

This change allows us to keep track of
connections made using our internal SQl shell.

Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

I haven't looked at the change itself, but I have a drive-by comment about the PR title -- by convention, we try to prefix the PR title with the affected package. So in this case, it could look something like sql: add sql shell telemetry counter.

There's a full set of guidelines on the wiki around commit messages https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

[rafiss]

i thought a bit more about how to test this:

could you add a test to pkg/sql/telemetry_test.go. it can be named TestTelemetryRecordCockroachShell it should work like this:

1. start a test server like this:

		var test telemetryTest
		test.Start(t, []base.TestServerArgs{{}})
		defer test.Close()

(for example, see similar code in pkg/sql/sqltestutils/telemetry.go TelemetryTest)

2. create a connection string that manually sets an application_name of $ cockroach internal test

sqlutils.PGUrl(t, test.cluster.Server(0).ServingSQLAddr(), "TestTelemetryRecordCockroachShell", url.User("root"))

this gives you back a URL, and you can call url.Query().Add() to add an "application_name" option

3. create a new connection to the database with this connection string
   call gosql.Open("postgres", url.String())
   you can see an example in pkg/testutils/serverutils/test_server_shim.go OpenDBConnE

4. make sure the new counter was incremented
   use this to make telemetry get reported

reporter := test.server.DiagnosticsReporter().(*diagnostics.Reporter)
reporter.ReportDiagnostics()

then look inside of test.diagSrv.LastRequestData() to check for the sql.cockroach_cli key. this part will be tricky! you will have to read the documentation to understand how to access the data, but let me know if you aren't sure where to look.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown)

---

pkg/sql/pgwire/server.go, line 826 at r1 (raw file):

	if appName, ok := args.SessionDefaults["application_name"]; ok {
		if strings.HasPrefix(appName, "$") {

nit: instead of using the "$" directly, it's better to refer to a constant. (finding the name of that constant can be a whole challenge too..) but in this case, use catconstants.ReportableAppNamePrefix

---

pkg/sql/sqltelemetry/session.go, line 34 at r1 (raw file):

// CockroachCliExec is to be incremented every time a
//statement is executed via the SQL shell
var CockroachCliExec = telemetry.GetCounter("sql.cockroach_cli_exec")

nit: since this PR doesn't add the Exec tracking yet, cane you remove the exec counter from this commit?

[knz]

use this to make telemetry get reported

I would use another of the feature counter tests instead. ReportDiagnostics is a bit complicated to use, it's possible to look at the feature counters directly.

[rafiss]

pro-tip: in your PR body can you add a line like "Fixes #62208"

this will link your PR to the issue, and when the PR is closed, it will automatically close the issue

see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @knz)

---

pkg/sql/telemetry_test.go, line 86 at r2 (raw file):

	q.Add("application_name", "$ cockroach internal test")
	pgUrl.RawQuery = q.Encode()
	fmt.Println(pgUrl)

nit: remove this Println

---

pkg/sql/telemetry_test.go, line 90 at r2 (raw file):

	db, err := gosql.Open("postgres", pgUrl.String())
	if err != nil {
		//return nil, err

nit: remove commented line

---

pkg/sql/telemetry_test.go, line 95 at r2 (raw file):

	defer db.Close()

	var app string

nit: it would be more clear to name this appName

---

pkg/sql/telemetry_test.go, line 96 at r2 (raw file):

	var app string
	db.QueryRow("SHOW application_name").Scan(&app)

nit: this queries the variable, but doesn't do anything with it
after this you can add a line like

require.Equal("$ cockroach internal test", appName)

which will make sure the app is what you expect

---

pkg/sql/telemetry_test.go, line 103 at r2 (raw file):

	last := diagSrv.LastRequestData()
	fmt.Printf("%v", last)
	//require.Equal(t, appName, last.)

thanks to knz's helpful comment, it seems like we have a better way of checking of the feature counter was incremented

you can perform this query:

SELECT usage_count
  FROM crdb_internal.feature_usage
 WHERE feature_name = 'sql.cockroach_cli';

and verify that usage_count is equal to 1.

the way to do this query is very similar to how you did QueryRow above. except instead of using a string value in Scan() you should use an int value.

with this approach, we no longer need the diagSrv, the reporter, the tempExternalIODir, or any of the more complicated setup in the beginning of this test. you should be able to change the cluster setup to just this line

	cluster := serverutils.StartNewTestCluster(t, 1, base.TestClusterArgs{})

i didn't know we could do it in this simple way before!

---

pkg/sql/sqltelemetry/session.go, line 29 at r2 (raw file):

// CockroachShellCounter is to be incremented every time a
//client uses the Cockroach SQL shell to access the sql layer

nit: add a space in between // and client

---

pkg/sql/sqltelemetry/session.go, line 30 at r2 (raw file):

// CockroachShellCounter is to be incremented every time a
//client uses the Cockroach SQL shell to access the sql layer
var CockroachShellCounter = telemetry.GetCounterOnce("sql.cockroach_cli")

hmm maybe this should be called sql.cockroach_cli.connection. let me create a thread with Vy

[knz]

@rafiss when you chat about this with @vy-ton please integrate the following comments:

• we use different values of application_name for every CLI utility that use a SQL connection. For example cockroach node ls will connect as $ cockroach node. Do you want these values to also increment the counter? If not, you'll need a different (more specific) match.
• the application_name is a value that can be overridden by clients in the conn URL. Someone using our SQL shell can pass a different app_name. Are we OK with losing that connection from the counter? If not, see below.
• We've discussed the problem of properly separating "user-initiated queries" from "automation-initiated queries". Check out this comment and the one after. The suggestion you and Archer made was to rely on a different mechanism than application_name (a special statement issued only by the SQL shell). Maybe that's applicable here too?

[rafiss]

@knz sure -- will get answers to these as well

[rafiss]

the application_name is a value that can be overridden by clients in the conn URL. Someone using our SQL shell can pass a different app_name. Are we OK with losing that connection from the counter?

though for this point, it looks like your recent refactors means that a $ will always be prepended to whatever user-supplied appName:

cockroach/pkg/cli/sql_client.go

Line 79 in 5c56090

sqlCtx.ApplicationName = catconstants.ReportableAppNamePrefix + appName

there still is the risk that a user will add $ to their own non-cockroach-CLI app's application_name. i'm asking vy about how much this matters

[knz]

it looks like your recent refactors means that a $ will always be prepended to whatever user-supplied appName

I don't think that's true?

% ./cockroach sql --url 'postgresql://root@kenax:26257?sslmode=disable&application_name=foo' -e 'show application_name';
  application_name
--------------------
  foo
(1 row)

The "appName" that you're pointing to in sql_client.go is the one that is hardcoded by the CLI utilities. If the user provides one in the URL, that takes priority. See (c *Context) MakeConn in cli/clisqlcfg/context.go.

[rafiss]

I see! thanks for explaining

[rafiss]

Summary from talking with @vy-ton :

• Let's use the name sql.connection.cockroach_cli in case we later want to classify other telemetry features under sql.connection
• Let's increment the counter only when the full application name matches $ cockroach sql. (@e-mbrown when making this change, please use or create new constants to avoid hard-coding the app name)
• Vy is comfortable with the risk of noisy data from people overriding their app name.
• We are deprioritizing the feature counter for number of statements executed in the SQL shell
• Vy pointed out that the DB Console statements page also cannot distinguish which statements are automatic versus user-created. That can be addressed at some future point.

[e-mbrown]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)

---

pkg/sql/telemetry_test.go, line 103 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

thanks to knz's helpful comment, it seems like we have a better way of checking of the feature counter was incremented

you can perform this query:

SELECT usage_count
  FROM crdb_internal.feature_usage
 WHERE feature_name = 'sql.cockroach_cli';

and verify that usage_count is equal to 1.

the way to do this query is very similar to how you did QueryRow above. except instead of using a string value in Scan() you should use an int value.

with this approach, we no longer need the diagSrv, the reporter, the tempExternalIODir, or any of the more complicated setup in the beginning of this test. you should be able to change the cluster setup to just this line

	cluster := serverutils.StartNewTestCluster(t, 1, base.TestClusterArgs{})

i didn't know we could do it in this simple way before!

Done.

---

pkg/sql/sqltelemetry/session.go, line 30 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hmm maybe this should be called sql.cockroach_cli.connection. let me create a thread with Vy

Done.

[rafiss]

this is really close!

I see that there were some test failures, but I don't think it's related to your changes. Can you try rebasing using git pull --rebase origin master?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @knz, and @rafiss)

---

pkg/sql/telemetry_test.go, line 62 at r3 (raw file):

	q := pgUrl.Query()

	q.Add("application_name", "$ cockroach sql")

nit: update this and the require check below to use the catconstants too

---

pkg/sql/catalog/catconstants/constants.go, line 33 at r3 (raw file):

const DelegatedAppNamePrefix = "$$ "

const InternalSqlAppName = "$ cockroach sql"

small nitpick: could you make it const InternalSqlAppName = "cockroach sql"?

I think that's more technically correct. then you should also update this line to use this InternalSqlAppName constant:

cockroach/pkg/cli/sql_shell_cmd.go

Line 52 in 186313a

conn, err := makeSQLClient("cockroach sql", useDefaultDb)

---

pkg/sql/pgwire/server.go, line 827 at r3 (raw file):

	if appName, ok := args.SessionDefaults["application_name"]; ok {
		if appName == catconstants.InternalSqlAppName {

nit: can you add a comment before this if statement, like "The client might override the appName to something else, which would prevent it from being counted in telemetry, but we've decided that this noise in the data is acceptable."

also, with my earlier comment, the condition might need to change to:

if appName == catconstants.ReportableAppNamePrefix + catconstants.InternalSqlAppName {

---

pkg/sql/sqltelemetry/session.go, line 29 at r3 (raw file):

// CockroachShellCounter is to be incremented every time a
// client uses the Cockroach SQL shell to access the sql layer

nit: this comment can say "uses the SQL shell to connect to CockroachDB"

[e-mbrown]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)

---

pkg/sql/catalog/catconstants/constants.go, line 33 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

small nitpick: could you make it const InternalSqlAppName = "cockroach sql"?

I think that's more technically correct. then you should also update this line to use this InternalSqlAppName constant:

cockroach/pkg/cli/sql_shell_cmd.go

Line 52 in 186313a

conn, err := makeSQLClient("cockroach sql", useDefaultDb)

Done.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @knz, and @rafiss)

---

pkg/sql/telemetry_test.go, line 47 at r6 (raw file):

func TestTelemetryRecordCockroachShell(t *testing.T) {

the beginning of the function needs two helpers that are common for all tests (one of the lint errors mentions this)

	defer leaktest.AfterTest(t)()
	defer log.Scope(t).Close(t)

---

pkg/sql/telemetry_test.go, line 73 at r6 (raw file):

	var appName string
	db.QueryRow("SHOW application_name").Scan(&appName)

i think there's also a linter for doing error checking on QueryRow. so this needs to become:

err = db.QueryRow("SHOW application_name").Scan(&appName)
require.NoError(t, err)

---

pkg/sql/telemetry_test.go, line 79 at r6 (raw file):

	db.QueryRow(
		"SELECT usage_count FROM crdb_internal.feature_usage WHERE feature_name = 'sql.connection.cockroach_cli'",
	).Scan(&counter)

and this needs to be:

err = db.QueryRow(
		"SELECT usage_count FROM crdb_internal.feature_usage WHERE feature_name = 'sql.connection.cockroach_cli'",
	).Scan(&counter)
require.NoError(t, err)

[rafiss]

this looks good to me! feel free to merge

[e-mbrown]

bors r=rafiss,knz,arulajmani

[craig]

Build succeeded:

• GitHub CI (Cockroach)