refactor/telemetry: add debug diagnostics

Keep expected telemetry failures out of normal output while making them available through debug logging with the underlying error. Narrow the concrete recorder type, require its logger dependency, and retain the server-required parameters and numeric metadata wire format.

## Test Plan

- go test ./...

Author: BolajiOlajide

internal/telemetry/telemetry.go

@@ -18,12 +18,13 @@ import (
 
 	"github.com/sourcegraph/src-cli/internal/api"
 
+	"github.com/sourcegraph/log"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
 const (
 	// clientName identifies src-cli as the source of telemetry events.
-	clientName = "SRC_CLI"
+	clientName = "src.cli"
 
 	// eventParametersVersion is the schema version of the metadata we attach to
 	// each event. Bump it when the shape of the metadata changes.
@@ -47,37 +48,37 @@ const recordEventsMutation = `mutation RecordTelemetryEvents($events: [Telemetry
 }`
 
 // Recorder records events through an api.Client.
-type Recorder struct {
+type recorder struct {
 	client        api.Client
 	clientVersion string
 	timeout       time.Duration
+	logger        log.Logger
 }
 
 // NewRecorder returns a Recorder for the given src-cli version.
-func NewRecorder(client api.Client, clientVersion string) *Recorder {
-	return &Recorder{
+func NewRecorder(client api.Client, logger log.Logger, clientVersion string) *recorder {
+	return &recorder{
 		client:        client,
 		clientVersion: clientVersion,
 		timeout:       defaultTimeout,
+		logger:        logger,
 	}
 }
 
 // Record sends an event on a best-effort basis. Feature and action identify the
 // event (for example, "srcCli.search" and "succeeded"). Metadata must contain
-// only numeric, PII-free facts. Record never returns an error or panics: network,
-// GraphQL, timeout, and old-instance failures are silently dropped. It applies
-// its own timeout, so the caller's context need not carry a deadline.
-func (r *Recorder) Record(ctx context.Context, feature, action string, metadata map[string]float64) {
-	_ = r.record(ctx, feature, action, metadata)
+// only numeric, PII-free facts. Network, GraphQL, timeout, and old-instance
+// failures are logged at debug level. Record applies its own timeout, so the
+// caller's context need not carry a deadline.
+func (r *recorder) Record(ctx context.Context, feature, action string, metadata map[string]float64) {
+	if err := r.record(ctx, feature, action, metadata); err != nil {
+		r.logger.Debug("recording telemetry event", log.String("feature", feature), log.String("action", action), log.Error(err))
+	}
 }
 
 // record does the work behind Record and returns any error, so it can be tested
 // directly. Callers outside tests should use Record.
-func (r *Recorder) record(ctx context.Context, feature, action string, metadata map[string]float64) error {
-	if r.client == nil {
-		return errors.New("nil api client")
-	}
-
+func (r *recorder) record(ctx context.Context, feature, action string, metadata map[string]float64) error {
 	ctx, cancel := context.WithTimeout(ctx, r.timeout)
 	defer cancel()
 
@@ -124,7 +125,7 @@ func buildEventInput(clientVersion, feature, action string, metadata map[string]
 	return map[string]any{
 		"feature": feature,
 		"action":  action,
-		"source": map[string]any{
+		"source": map[string]string{
 			"client":        clientName,
 			"clientVersion": clientVersion,
 		},
@@ -139,16 +140,13 @@ func buildEventInput(clientVersion, feature, action string, metadata map[string]
 // the API expects, sorted by key for deterministic output.
 func buildMetadata(metadata map[string]float64) []any {
 	out := make([]any, 0, len(metadata))
-	if len(metadata) == 0 {
-		return out
-	}
 	keys := make([]string, 0, len(metadata))
-	for k := range metadata {
-		keys = append(keys, k)
+	for key := range metadata {
+		keys = append(keys, key)
 	}
 	sort.Strings(keys)
-	for _, k := range keys {
-		out = append(out, map[string]any{"key": k, "value": metadata[k]})
+	for _, key := range keys {
+		out = append(out, map[string]any{"key": key, "value": metadata[key]})
 	}
 	return out
 }

internal/telemetry/telemetry_test.go

@@ -18,6 +18,8 @@ import (
 	apimock "github.com/sourcegraph/src-cli/internal/api/mock"
 	"github.com/sourcegraph/src-cli/internal/oauth"
 
+	"github.com/sourcegraph/log"
+	"github.com/sourcegraph/log/logtest"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -70,7 +72,8 @@ func TestRecord_SendsWellFormedMutation(t *testing.T) {
 		Return(req, nil)
 	client.On("Do", req).Return(response(http.StatusOK, "{}"), nil)
 
-	rec := NewRecorder(client, testClientVersion)
+	logger := log.NoOp()
+	rec := NewRecorder(client, logger, testClientVersion)
 	rec.Record(context.Background(), "srcCli.search", "succeeded", map[string]float64{
 		"durationMs": 12,
 		"exitCode":   0,
@@ -90,20 +93,17 @@ func TestRecord_SendsWellFormedMutation(t *testing.T) {
 	assert.Equal(t, clientName, source["client"])
 	assert.Equal(t, testClientVersion, source["clientVersion"])
 
-	params := event["parameters"].(map[string]any)
-	assert.Equal(t, float64(eventParametersVersion), params["version"])
-
-	metadata := params["metadata"].([]any)
-	// sorted by key: durationMs, exitCode
+	parameters := event["parameters"].(map[string]any)
+	assert.Equal(t, float64(eventParametersVersion), parameters["version"])
 	assert.Equal(t, []any{
 		map[string]any{"key": "durationMs", "value": float64(12)},
 		map[string]any{"key": "exitCode", "value": float64(0)},
-	}, metadata)
+	}, parameters["metadata"])
 
 	client.AssertExpectations(t)
 }
 
-func TestRecord_EmptyMetadataSendsEmptyList(t *testing.T) {
+func TestRecord_NilMetadataSendsEmptyList(t *testing.T) {
 	client := &apimock.Client{}
 	req := httptest.NewRequest(http.MethodPost, "/.api/graphql", nil)
 
@@ -119,12 +119,14 @@ func TestRecord_EmptyMetadataSendsEmptyList(t *testing.T) {
 		Return(req, nil)
 	client.On("Do", req).Return(response(http.StatusOK, "{}"), nil)
 
-	rec := NewRecorder(client, testClientVersion)
+	logger := log.NoOp()
+	rec := NewRecorder(client, logger, testClientVersion)
 	rec.Record(context.Background(), "srcCli.version", "succeeded", nil)
 
 	event := gotPayload.Variables["events"].([]any)[0].(map[string]any)
-	params := event["parameters"].(map[string]any)
-	assert.Equal(t, []any{}, params["metadata"])
+	parameters := event["parameters"].(map[string]any)
+	assert.Equal(t, float64(eventParametersVersion), parameters["version"])
+	assert.Equal(t, []any{}, parameters["metadata"])
 }
 
 func TestRecord_NetworkErrorSwallowed(t *testing.T) {
@@ -133,12 +135,19 @@ func TestRecord_NetworkErrorSwallowed(t *testing.T) {
 	client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/graphql", mock.Anything).Return(req, nil)
 	client.On("Do", req).Return(nil, errors.New("connection refused"))
 
-	rec := NewRecorder(client, testClientVersion)
+	logger, exportLogs := logtest.CapturedWith(t, logtest.LoggerOptions{Level: log.LevelNone})
+	rec := NewRecorder(client, logger, testClientVersion)
 
 	// Must not panic and must not surface the error.
 	assert.NotPanics(t, func() {
 		rec.Record(context.Background(), "srcCli.search", "failed", nil)
 	})
+	logs := exportLogs()
+	if assert.Len(t, logs, 1) {
+		assert.Equal(t, log.LevelDebug, logs[0].Level)
+		assert.Equal(t, "recording telemetry event", logs[0].Message)
+		assert.Equal(t, "connection refused", logs[0].Fields["error"])
+	}
 
 	// record itself reports the error for callers that want it.
 	err := rec.record(context.Background(), "srcCli.search", "failed", nil)
@@ -153,14 +162,8 @@ func TestRecord_GraphQLErrorSwallowed(t *testing.T) {
 	client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/graphql", mock.Anything).Return(req, nil)
 	client.On("Do", req).Return(response(http.StatusOK, "{\"errors\":[{\"message\":\"unknown field telemetry\"}]}"), nil)
 
-	rec := NewRecorder(client, testClientVersion)
-	assert.NotPanics(t, func() {
-		rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
-	})
-}
-
-func TestRecord_NilClientDoesNotPanic(t *testing.T) {
-	rec := NewRecorder(nil, testClientVersion)
+	logger := log.NoOp()
+	rec := NewRecorder(client, logger, testClientVersion)
 	assert.NotPanics(t, func() {
 		rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
 	})
@@ -198,7 +201,8 @@ func TestRecord_OAuthUnauthorizedDoesNotWriteToStdout(t *testing.T) {
 	os.Stdout = stdoutWriter
 	t.Cleanup(func() { os.Stdout = oldStdout })
 
-	rec := NewRecorder(client, testClientVersion)
+	logger := log.NoOp()
+	rec := NewRecorder(client, logger, testClientVersion)
 	rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
 
 	if err := stdoutWriter.Close(); err != nil {
@@ -230,7 +234,8 @@ func TestRecord_AppliesTimeout(t *testing.T) {
 		Return(req, nil)
 	client.On("Do", req).Return(response(http.StatusOK, "{}"), nil)
 
-	rec := NewRecorder(client, testClientVersion)
+	logger := log.NoOp()
+	rec := NewRecorder(client, logger, testClientVersion)
 	rec.timeout = 50 * time.Millisecond
 	rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
 
@@ -240,7 +245,8 @@ func TestRecord_AppliesTimeout(t *testing.T) {
 func TestRecord_TimeoutCancelsHTTPRequest(t *testing.T) {
 	requestCanceled := make(chan struct{})
 	client := &cancellationClient{canceled: requestCanceled, release: make(chan struct{})}
-	rec := NewRecorder(client, testClientVersion)
+	logger := log.NoOp()
+	rec := NewRecorder(client, logger, testClientVersion)
 	rec.timeout = 20 * time.Millisecond
 
 	ctx, cancel := context.WithCancel(context.Background())