telemetry/resolvers, internal/telemetry, telemetry-gateway: validate feature/action (#62214)

As suggested by @willdollman [here](https://sourcegraph.slack.com/archives/C1JH2BEHZ/p1713950660542809?thread_ts=1713950290.063969&cid=C1JH2BEHZ), we should do some more robust validation when telemetry is submitted by clients, to reduce the noise that ends up in BigQuery. This is also added to telemetry-gateway and backend, where we already do basic validation on feature and action. --------- Co-authored-by: Will Dollman <will.dollman@sourcegraph.com>
2026-02-06 18:51:59 +00:00 · 2024-04-29 12:34:50 -07:00 · 2024-04-29 12:34:50 -07:00 · 4933540c1d
commit 4933540c1d
parent 4909533715
10 changed files with 199 additions and 63 deletions
--- a/cmd/frontend/internal/telemetry/resolvers/resolvers_test.go
+++ b/cmd/frontend/internal/telemetry/resolvers/resolvers_test.go
@ -24,24 +24,24 @@ import (
 func TestExportedEvents(t *testing.T) {
 	exportedEvents := []*telemetrygatewayv1.Event{{
 		Id:      "1",
-		Feature: "Feature",
-		Action:  "View",
+		Feature: "feature",
+		Action:  "view",
 		// Most recent event
 		Timestamp: timestamppb.New(time.Date(2022, 11, 3, 1, 0, 0, 0, time.UTC)),
 	}, {
 		Id:        "2",
-		Feature:   "Feature",
-		Action:    "Click",
+		Feature:   "feature",
+		Action:    "click",
 		Timestamp: timestamppb.New(time.Date(2022, 11, 3, 2, 0, 0, 0, time.UTC)),
 	}, {
 		Id:        "3",
-		Feature:   "Feature",
-		Action:    "Show",
+		Feature:   "feature",
+		Action:    "show",
 		Timestamp: timestamppb.New(time.Date(2022, 11, 3, 3, 0, 0, 0, time.UTC)),
 	}, {
 		Id:        "4",
-		Feature:   "Feature",
-		Action:    "Dance",
+		Feature:   "feature",
+		Action:    "dance",
 		Timestamp: timestamppb.New(time.Date(2022, 11, 3, 4, 0, 0, 0, time.UTC)),
 	}}
 	var exportedEventIDs []string
--- a/cmd/frontend/internal/telemetry/resolvers/telemetrygateway.go
+++ b/cmd/frontend/internal/telemetry/resolvers/telemetrygateway.go
@ -25,11 +25,9 @@ func newTelemetryGatewayEvents(
 ) ([]*telemetrygatewayv1.Event, error) {
 	gatewayEvents := make([]*telemetrygatewayv1.Event, len(gqlEvents))
 	for i, gqlEvent := range gqlEvents {
-		if gqlEvent.Feature == "" {
-			return nil, errors.Newf("feature is required for event %d", i)
-		}
-		if gqlEvent.Action == "" {
-			return nil, errors.Newf("action is required for event %d", i)
+		if err := telemetrygatewayv1.ValidateEventFeatureAction(gqlEvent.Feature, gqlEvent.Action); err != nil {
+			return nil, errors.Wrapf(err, "invalid feature/action for event %d: %s/%s",
+				i, gqlEvent.Feature, gqlEvent.Action)
 		}

 		event := telemetrygatewayevent.New(ctx, now, newUUID)
--- a/cmd/frontend/internal/telemetry/resolvers/telemetrygateway_test.go
+++ b/cmd/frontend/internal/telemetry/resolvers/telemetrygateway_test.go
@ -29,12 +29,12 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 			name: "basic",
 			ctx:  context.Background(),
 			event: graphqlbackend.TelemetryEventInput{
-				Feature: "Feature",
-				Action:  "Example",
+				Feature: "feature",
+				Action:  "example",
 			},
 			expect: autogold.Expect(`{
-  "action": "Example",
-  "feature": "Feature",
+  "action": "example",
+  "feature": "feature",
  "id": "basic",
  "parameters": {},
  "source": {
@ -50,12 +50,12 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 			name: "with anonymous user",
 			ctx:  actor.WithActor(context.Background(), actor.FromAnonymousUser("1234")),
 			event: graphqlbackend.TelemetryEventInput{
-				Feature: "Feature",
-				Action:  "Example",
+				Feature: "feature",
+				Action:  "example",
 			},
 			expect: autogold.Expect(`{
-  "action": "Example",
-  "feature": "Feature",
+  "action": "example",
+  "feature": "feature",
  "id": "with anonymous user",
  "parameters": {},
  "source": {
@ -74,12 +74,12 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 			name: "with authenticated user",
 			ctx:  actor.WithActor(context.Background(), actor.FromMockUser(1234)),
 			event: graphqlbackend.TelemetryEventInput{
-				Feature: "Feature",
-				Action:  "Example",
+				Feature: "feature",
+				Action:  "example",
 			},
 			expect: autogold.Expect(`{
-  "action": "Example",
-  "feature": "Feature",
+  "action": "example",
+  "feature": "feature",
  "id": "with authenticated user",
  "parameters": {},
  "source": {
@ -98,8 +98,8 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 			name: "with parameters",
 			ctx:  context.Background(),
 			event: graphqlbackend.TelemetryEventInput{
-				Feature: "Feature",
-				Action:  "Example",
+				Feature: "feature",
+				Action:  "example",
 				Parameters: graphqlbackend.TelemetryEventParametersInput{
 					Version: 0,
 					Metadata: &[]graphqlbackend.TelemetryEventMetadataInput{
@ -118,8 +118,8 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 				},
 			},
 			expect: autogold.Expect(`{
-  "action": "Example",
-  "feature": "Feature",
+  "action": "example",
+  "feature": "feature",
  "id": "with parameters",
  "parameters": {
    "billingMetadata": {
@ -146,8 +146,8 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 			name: "with string PrivateMetadata",
 			ctx:  context.Background(),
 			event: graphqlbackend.TelemetryEventInput{
-				Feature: "Feature",
-				Action:  "Example",
+				Feature: "feature",
+				Action:  "example",
 				Parameters: graphqlbackend.TelemetryEventParametersInput{
 					Version: 0,
 					PrivateMetadata: &graphqlbackend.JSONValue{
@ -156,8 +156,8 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 				},
 			},
 			expect: autogold.Expect(`{
-  "action": "Example",
-  "feature": "Feature",
+  "action": "example",
+  "feature": "feature",
  "id": "with string PrivateMetadata",
  "parameters": {
    "privateMetadata": {
@ -177,8 +177,8 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 			name: "with numeric PrivateMetadata",
 			ctx:  context.Background(),
 			event: graphqlbackend.TelemetryEventInput{
-				Feature: "Feature",
-				Action:  "Example",
+				Feature: "feature",
+				Action:  "example",
 				Parameters: graphqlbackend.TelemetryEventParametersInput{
 					Version: 0,
 					PrivateMetadata: &graphqlbackend.JSONValue{
@ -187,8 +187,8 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 				},
 			},
 			expect: autogold.Expect(`{
-  "action": "Example",
-  "feature": "Feature",
+  "action": "example",
+  "feature": "feature",
  "id": "with numeric PrivateMetadata",
  "parameters": {
    "privateMetadata": {
@ -209,12 +209,12 @@ func TestNewTelemetryGatewayEvents(t *testing.T) {
 			ctx:  context.Background(),
 			event: graphqlbackend.TelemetryEventInput{
 				Timestamp: &gqlutil.DateTime{Time: staticTime.Add(48 * time.Hour)},
-				Feature:   "Feature",
-				Action:    "Example",
+				Feature:   "feature",
+				Action:    "example",
 			},
 			expect: autogold.Expect(`{
-  "action": "Example",
-  "feature": "Feature",
+  "action": "example",
+  "feature": "feature",
  "id": "with custom timestamp",
  "parameters": {},
  "source": {
--- a/cmd/telemetry-gateway/internal/events/events.go
+++ b/cmd/telemetry-gateway/internal/events/events.go
@ -142,11 +142,8 @@ func (p *Publisher) Publish(ctx context.Context, events []*telemetrygatewayv1.Ev
 			if event.Id == "" {
 				return errors.New("event ID is required"), false
 			}
-			if event.Feature == "" {
-				return errors.New("event feature is required"), false
-			}
-			if event.Action == "" {
-				return errors.New("event action is required"), false
+			if err := telemetrygatewayv1.ValidateEventFeatureAction(event.Feature, event.Action); err != nil {
+				return errors.Wrap(err, "invalid event 'feature' or 'action'"), false
 			}
 			if event.Timestamp == nil {
 				return errors.New("event timestamp is required"), false
--- a/cmd/telemetry-gateway/internal/events/events_test.go
+++ b/cmd/telemetry-gateway/internal/events/events_test.go
@ -56,9 +56,10 @@ func TestPublish(t *testing.T) {
 	for i := range events {
 		events[i] = &telemetrygatewayv1.Event{
 			Id:        strconv.Itoa(i),
-			Feature:   t.Name(),
-			Action:    strconv.Itoa(i),
 			Timestamp: timestamppb.Now(),
+			// Feature, Action must pass validation
+			Feature: "testPublish",
+			Action:  "action",
 		}
 	}

@ -81,6 +82,9 @@ func TestPublish(t *testing.T) {

 	// Collect all the results we got
 	for _, r := range results {
+		assert.Nil(t, r.PublishError)
+		assert.Equal(t, r.EventFeature, "testPublish")
+		assert.Equal(t, r.EventAction, "action")
 		eventResults[r.EventID] = true
 	}

--- a/internal/telemetry/telemetry.go
+++ b/internal/telemetry/telemetry.go
@ -108,11 +108,8 @@ func (r *EventRecorder) Record(ctx context.Context, feature eventFeature, action
 	if ctx == nil {
 		return errors.New("context is required")
 	}
-	if feature == "" {
-		return errors.New("feature is required")
-	}
-	if action == "" {
-		return errors.New("action is required")
+	if err := telemetrygatewayv1.ValidateEventFeatureAction(string(feature), string(action)); err != nil {
+		return errors.Wrap(err, "invalid event feature or action")
 	}
 	return r.store.StoreEvents(ctx, []*telemetrygatewayv1.Event{
 		newTelemetryGatewayEvent(ctx, time.Now(), telemetrygatewayv1.DefaultEventIDFunc, feature, action, parameters),
--- a/internal/telemetry/telemetry_test.go
+++ b/internal/telemetry/telemetry_test.go
@ -22,7 +22,7 @@ func TestRecorder(t *testing.T) {
 	store := telemetrytest.NewMockEventsStore()
 	recorder := telemetry.NewEventRecorder(store)

-	err := recorder.Record(context.Background(), "Feature", "Action", nil)
+	err := recorder.Record(context.Background(), "feature", "action", nil)
 	require.NoError(t, err)

 	// stored once
@ -30,7 +30,7 @@ func TestRecorder(t *testing.T) {
 	// called with 1 event
 	require.Len(t, store.StoreEventsFunc.History()[0].Arg1, 1)
 	// stored event has 1 event
-	require.Equal(t, "Feature", store.StoreEventsFunc.History()[0].Arg1[0].Feature)
+	require.Equal(t, "feature", store.StoreEventsFunc.History()[0].Arg1[0].Feature)
 }

 func TestRecorderEndToEnd(t *testing.T) {
@ -52,7 +52,7 @@ func TestRecorderEndToEnd(t *testing.T) {
 	wantEvents := 3
 	t.Run("Record and BatchRecord", func(t *testing.T) {
 		assert.NoError(t, recorder.Record(ctx,
-			"Test", "Action1",
+			"test", "actionOne",
 			&telemetry.EventParameters{
 				Metadata: telemetry.EventMetadata{
 					"metadata": 1,
@ -63,12 +63,12 @@ func TestRecorderEndToEnd(t *testing.T) {
 			}))
 		assert.NoError(t, recorder.BatchRecord(ctx,
 			telemetry.Event{
-				Feature: "Test",
-				Action:  "Action2",
+				Feature: "test",
+				Action:  "actionTwo",
 			},
 			telemetry.Event{
-				Feature: "Test",
-				Action:  "Action3",
+				Feature: "test",
+				Action:  "actionThree",
 			}))
 	})

@ -86,7 +86,7 @@ func TestRecorderEndToEnd(t *testing.T) {

 	t.Run("record without v1", func(t *testing.T) {
 		ctx := teestore.WithoutV1(ctx)
-		assert.NoError(t, recorder.Record(ctx, "Test", "Action1", &telemetry.EventParameters{}))
+		assert.NoError(t, recorder.Record(ctx, "test", "actionOne", &telemetry.EventParameters{}))

 		telemetryEvents, err := db.TelemetryEventsExportQueue().ListForExport(ctx, 999)
 		require.NoError(t, err)
--- a/lib/telemetrygateway/v1/BUILD.bazel
+++ b/lib/telemetrygateway/v1/BUILD.bazel
@ -1,3 +1,4 @@
+load("//dev:go_defs.bzl", "go_test")
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@rules_buf//buf:defs.bzl", "buf_lint_test")
@ -53,7 +54,11 @@ go_library(
        "//internal/telemetrygateway/integration_tests:__pkg__",
        "//internal/telemetrygateway/protocol:__pkg__",
    ],
-    deps = ["@com_github_google_uuid//:uuid"],
+    deps = [
+        "//lib/errors",
+        "@com_github_google_uuid//:uuid",
+        "@com_github_grafana_regexp//:regexp",
+    ],
 )

 # See https://github.com/sourcegraph/sourcegraph/issues/50032
@ -76,3 +81,14 @@ doc_template_compile(
    template = "protoc-gen-doc.tmpl",
    visibility = ["//doc/dev/background-information/telemetry:__pkg__"],
 )
+
+go_test(
+    name = "telemetrygateway_test",
+    srcs = ["v1_test.go"],
+    embed = [":telemetrygateway"],
+    deps = [
+        "@com_github_hexops_autogold_v2//:autogold",
+        "@com_github_stretchr_testify//assert",
+        "@com_github_stretchr_testify//require",
+    ],
+)
--- a/lib/telemetrygateway/v1/v1.go
+++ b/lib/telemetrygateway/v1/v1.go
@ -4,7 +4,12 @@
 // helpers for Telemetry Gateway integrations.
 package v1

-import "github.com/google/uuid"
+import (
+	"github.com/google/uuid"
+	"github.com/grafana/regexp"
+
+	"github.com/sourcegraph/sourcegraph/lib/errors"
+)

 // DefaultEventIDFunc is the default generator for telemetry event IDs.
 // We currently use V7, which is time-ordered, making them useful for event IDs.
@ -12,3 +17,33 @@ import "github.com/google/uuid"
 var DefaultEventIDFunc = func() string {
 	return uuid.Must(uuid.NewV7()).String()
 }
+
+// featureActionRegex is used to validate feature and action names. Values must:
+// - Start with a lowercase letter
+// - Contain only letters, and dashes and dots as delimters
+// - Not contain any whitespace
+var featureActionRegex = regexp.MustCompile(`^[a-z][a-zA-Z-\.]+$`)
+
+// featureActionMaxLength is the maximum length of a feature or action name.
+const featureActionMaxLength = 64
+
+// ValidateEventFeatureAction validates the given feature and action names. It
+// should be used where event features and actions are provided by a client.
+func ValidateEventFeatureAction(feature, action string) error {
+	if feature == "" || action == "" {
+		return errors.New("'feature', 'action' must both be provided")
+	}
+	if len(feature) > featureActionMaxLength {
+		return errors.New("'feature' must be less than 64 characters")
+	}
+	if len(action) > featureActionMaxLength {
+		return errors.New("'action' must be less than 64 characters")
+	}
+	if !featureActionRegex.MatchString(feature) {
+		return errors.New("'feature' must start with a lowercase letter and contain only letters, dashes, and dots")
+	}
+	if !featureActionRegex.MatchString(action) {
+		return errors.New("'action' must start with a lowercase letter and contain only letters, dashes, and dots")
+	}
+	return nil
+}
--- a/lib/telemetrygateway/v1/v1_test.go
+++ b/lib/telemetrygateway/v1/v1_test.go
@ -0,0 +1,89 @@
+package v1
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/hexops/autogold/v2"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateEventFeatureAction(t *testing.T) {
+	tests := []struct {
+		name    string
+		feature string
+		action  string
+		wantErr autogold.Value
+	}{
+		{
+			name:    "feature is empty",
+			feature: "",
+			action:  "valid",
+			wantErr: autogold.Expect("'feature', 'action' must both be provided"),
+		},
+		{
+			name:    "action is empty",
+			feature: "valid",
+			action:  "",
+			wantErr: autogold.Expect("'feature', 'action' must both be provided"),
+		},
+		{
+			name:    "feature too long",
+			feature: strings.Repeat("a", featureActionMaxLength+3),
+			action:  "valid",
+			wantErr: autogold.Expect("'feature' must be less than 64 characters"),
+		},
+		{
+			name:    "action too long",
+			feature: "valid",
+			action:  strings.Repeat("a", featureActionMaxLength+3),
+			wantErr: autogold.Expect("'action' must be less than 64 characters"),
+		},
+		{
+			name:    "feature starts with uppercase",
+			feature: "Invalid",
+			action:  "valid",
+			wantErr: autogold.Expect("'feature' must start with a lowercase letter and contain only letters, dashes, and dots"),
+		},
+		{
+			name:    "action starts with uppercase",
+			feature: "valid",
+			action:  "Invalid",
+			wantErr: autogold.Expect("'action' must start with a lowercase letter and contain only letters, dashes, and dots"),
+		},
+		{
+			name:    "feature contains invalid characters",
+			feature: "invalid_feature!",
+			action:  "valid",
+			wantErr: autogold.Expect("'feature' must start with a lowercase letter and contain only letters, dashes, and dots"),
+		},
+		{
+			name:    "action contains invalid characters",
+			feature: "valid",
+			action:  "invalid_action!",
+			wantErr: autogold.Expect("'action' must start with a lowercase letter and contain only letters, dashes, and dots"),
+		},
+		{
+			name:    "valid feature and action 1",
+			feature: "valid.feature",
+			action:  "valid-action",
+		},
+		{
+			name:    "valid feature and action 2",
+			feature: "validFeature.foobar",
+			action:  "valid.action",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateEventFeatureAction(tt.feature, tt.action)
+			if tt.wantErr != nil {
+				require.Error(t, err)
+				tt.wantErr.Equal(t, err.Error())
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}