refactor/telemetry: minimize recorder API

The telemetry package has no production callers yet, so exporting event models, source configuration, options, and test-only controls commits src-cli to an API before its integration requirements are known. Keep only the recorder construction and recording operations public, fix the client identity internally, and pass event values directly.

## Test Plan

- go test ./...

Author: BolajiOlajide

internal/telemetry/telemetry.go

@@ -12,8 +12,6 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"fmt"
-	"io"
 	"net/http"
 	"sort"
 	"time"
@@ -24,8 +22,8 @@ import (
 )
 
 const (
-	// ClientName identifies src-cli as the source of telemetry events.
-	ClientName = "SRC_CLI"
+	// clientName identifies src-cli as the source of telemetry events.
+	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.
@@ -48,82 +46,34 @@ const recordEventsMutation = `mutation RecordTelemetryEvents($events: [Telemetry
     }
 }`
 
-// Source identifies the client emitting events. It is constant for the lifetime
-// of a process.
-type Source struct {
-	// Client is the source client name, e.g. ClientName.
-	Client string
-	// ClientVersion is the src-cli version, e.g. "6.1.0" or "dev".
-	ClientVersion string
-}
-
-// Event is a single telemetry event.
-//
-// Feature and Action carry the event's identity and are always exported by
-// Sourcegraph, so command identity lives here (e.g. Feature "srcCli.search",
-// Action "succeeded"). Metadata values are numeric-only and are also always
-// exported; they must never contain user content. See .context/TELEMETRY.md.
-type Event struct {
-	// Feature is a noun describing what the event is about, e.g. "srcCli.search".
-	Feature string
-	// Action is a verb describing what happened, e.g. "succeeded" or "failed".
-	Action string
-	// Metadata holds numeric-only, PII-free facts about the event.
-	Metadata map[string]float64
-}
-
-// Recorder records events for a single Source through an api.Client.
+// Recorder records events through an api.Client.
 type Recorder struct {
-	client  api.Client
-	source  Source
-	timeout time.Duration
-	debug   io.Writer
+	client        api.Client
+	clientVersion string
+	timeout       time.Duration
 }
 
-// Option customizes a Recorder.
-type Option func(*Recorder)
-
-// WithTimeout overrides the default per-Record timeout.
-func WithTimeout(d time.Duration) Option {
-	return func(r *Recorder) {
-		if d > 0 {
-			r.timeout = d
-		}
+// NewRecorder returns a Recorder for the given src-cli version.
+func NewRecorder(client api.Client, clientVersion string) *Recorder {
+	return &Recorder{
+		client:        client,
+		clientVersion: clientVersion,
+		timeout:       defaultTimeout,
 	}
 }
 
-// WithDebug sets a writer that receives a diagnostic line whenever an event is
-// dropped. Intended to be wired to verbose (-v) output; leave unset for silence.
-func WithDebug(w io.Writer) Option {
-	return func(r *Recorder) { r.debug = w }
-}
-
-// NewRecorder returns a Recorder that records events for source through client.
-func NewRecorder(client api.Client, source Source, opts ...Option) *Recorder {
-	r := &Recorder{
-		client:  client,
-		source:  source,
-		timeout: defaultTimeout,
-	}
-	for _, opt := range opts {
-		opt(r)
-	}
-	return r
-}
-
-// Record sends event on a best-effort basis. It never returns an error and
-// never panics: network, GraphQL, timeout, and old-instance failures are all
-// silently dropped (written to the debug writer if one was set via WithDebug).
-// It applies its own timeout, so the caller's context need not carry a deadline.
-func (r *Recorder) Record(ctx context.Context, event Event) {
-	if err := r.record(ctx, event); err != nil && r.debug != nil {
-		fmt.Fprintf(r.debug, "telemetry: dropping event %q/%q: %v\n", event.Feature, event.Action, err)
-	}
+// 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)
 }
 
 // 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, event Event) error {
+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")
 	}
@@ -132,7 +82,7 @@ func (r *Recorder) record(ctx context.Context, event Event) error {
 	defer cancel()
 
 	vars := map[string]any{
-		"events": []any{buildEventInput(r.source, event)},
+		"events": []any{buildEventInput(r.clientVersion, feature, action, metadata)},
 	}
 
 	payload, err := json.Marshal(map[string]any{
@@ -170,17 +120,17 @@ func (r *Recorder) record(ctx context.Context, event Event) error {
 }
 
 // buildEventInput builds a single TelemetryEventInput as a JSON-serializable map.
-func buildEventInput(source Source, event Event) map[string]any {
+func buildEventInput(clientVersion, feature, action string, metadata map[string]float64) map[string]any {
 	return map[string]any{
-		"feature": event.Feature,
-		"action":  event.Action,
+		"feature": feature,
+		"action":  action,
 		"source": map[string]any{
-			"client":        source.Client,
-			"clientVersion": source.ClientVersion,
+			"client":        clientName,
+			"clientVersion": clientVersion,
 		},
 		"parameters": map[string]any{
 			"version":  eventParametersVersion,
-			"metadata": buildMetadata(event.Metadata),
+			"metadata": buildMetadata(metadata),
 		},
 	}
 }

internal/telemetry/telemetry_test.go

@@ -23,9 +23,7 @@ import (
 	"github.com/stretchr/testify/mock"
 )
 
-func testSource() Source {
-	return Source{Client: ClientName, ClientVersion: "6.1.0"}
-}
+const testClientVersion = "6.1.0"
 
 func response(statusCode int, body string) *http.Response {
 	return &http.Response{
@@ -72,11 +70,10 @@ func TestRecord_SendsWellFormedMutation(t *testing.T) {
 		Return(req, nil)
 	client.On("Do", req).Return(response(http.StatusOK, "{}"), nil)
 
-	rec := NewRecorder(client, testSource())
-	rec.Record(context.Background(), Event{
-		Feature:  "srcCli.search",
-		Action:   "succeeded",
-		Metadata: map[string]float64{"durationMs": 12, "exitCode": 0},
+	rec := NewRecorder(client, testClientVersion)
+	rec.Record(context.Background(), "srcCli.search", "succeeded", map[string]float64{
+		"durationMs": 12,
+		"exitCode":   0,
 	})
 
 	assert.Equal(t, recordEventsMutation, gotPayload.Query)
@@ -90,8 +87,8 @@ func TestRecord_SendsWellFormedMutation(t *testing.T) {
 	assert.Equal(t, "succeeded", event["action"])
 
 	source := event["source"].(map[string]any)
-	assert.Equal(t, ClientName, source["client"])
-	assert.Equal(t, "6.1.0", source["clientVersion"])
+	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"])
@@ -122,8 +119,8 @@ func TestRecord_EmptyMetadataSendsEmptyList(t *testing.T) {
 		Return(req, nil)
 	client.On("Do", req).Return(response(http.StatusOK, "{}"), nil)
 
-	rec := NewRecorder(client, testSource())
-	rec.Record(context.Background(), Event{Feature: "srcCli.version", Action: "succeeded"})
+	rec := NewRecorder(client, testClientVersion)
+	rec.Record(context.Background(), "srcCli.version", "succeeded", nil)
 
 	event := gotPayload.Variables["events"].([]any)[0].(map[string]any)
 	params := event["parameters"].(map[string]any)
@@ -136,17 +133,15 @@ 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"))
 
-	var debug bytes.Buffer
-	rec := NewRecorder(client, testSource(), WithDebug(&debug))
+	rec := NewRecorder(client, testClientVersion)
 
 	// Must not panic and must not surface the error.
 	assert.NotPanics(t, func() {
-		rec.Record(context.Background(), Event{Feature: "srcCli.search", Action: "failed"})
+		rec.Record(context.Background(), "srcCli.search", "failed", nil)
 	})
-	assert.Contains(t, debug.String(), "connection refused")
 
 	// record itself reports the error for callers that want it.
-	err := rec.record(context.Background(), Event{Feature: "srcCli.search", Action: "failed"})
+	err := rec.record(context.Background(), "srcCli.search", "failed", nil)
 	assert.Error(t, err)
 }
 
@@ -158,16 +153,16 @@ 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, testSource())
+	rec := NewRecorder(client, testClientVersion)
 	assert.NotPanics(t, func() {
-		rec.Record(context.Background(), Event{Feature: "srcCli.search", Action: "succeeded"})
+		rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
 	})
 }
 
 func TestRecord_NilClientDoesNotPanic(t *testing.T) {
-	rec := NewRecorder(nil, testSource())
+	rec := NewRecorder(nil, testClientVersion)
 	assert.NotPanics(t, func() {
-		rec.Record(context.Background(), Event{Feature: "srcCli.search", Action: "succeeded"})
+		rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
 	})
 }
 
@@ -203,8 +198,8 @@ func TestRecord_OAuthUnauthorizedDoesNotWriteToStdout(t *testing.T) {
 	os.Stdout = stdoutWriter
 	t.Cleanup(func() { os.Stdout = oldStdout })
 
-	rec := NewRecorder(client, testSource())
-	rec.Record(context.Background(), Event{Feature: "srcCli.search", Action: "succeeded"})
+	rec := NewRecorder(client, testClientVersion)
+	rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
 
 	if err := stdoutWriter.Close(); err != nil {
 		t.Fatal(err)
@@ -235,22 +230,24 @@ func TestRecord_AppliesTimeout(t *testing.T) {
 		Return(req, nil)
 	client.On("Do", req).Return(response(http.StatusOK, "{}"), nil)
 
-	rec := NewRecorder(client, testSource(), WithTimeout(50*time.Millisecond))
-	rec.Record(context.Background(), Event{Feature: "srcCli.search", Action: "succeeded"})
+	rec := NewRecorder(client, testClientVersion)
+	rec.timeout = 50 * time.Millisecond
+	rec.Record(context.Background(), "srcCli.search", "succeeded", nil)
 
 	assert.True(t, hadDeadline, "expected Record to apply a context deadline")
 }
 
 func TestRecord_TimeoutCancelsHTTPRequest(t *testing.T) {
 	requestCanceled := make(chan struct{})
 	client := &cancellationClient{canceled: requestCanceled, release: make(chan struct{})}
-	rec := NewRecorder(client, testSource(), WithTimeout(20*time.Millisecond))
+	rec := NewRecorder(client, testClientVersion)
+	rec.timeout = 20 * time.Millisecond
 
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	done := make(chan struct{})
 	go func() {
-		rec.Record(ctx, Event{Feature: "srcCli.search", Action: "succeeded"})
+		rec.Record(ctx, "srcCli.search", "succeeded", nil)
 		close(done)
 	}()