From 66dc276c2160ddc1da592bccec77352b29a49431 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 7 May 2026 17:42:11 +0200 Subject: [PATCH 1/2] Opt-in common dims on telemetry events Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/gh/ghtelemetry/telemetry.go | 20 +++++++++- internal/telemetry/fake.go | 18 +++++++-- internal/telemetry/telemetry.go | 28 +++++++++---- internal/telemetry/telemetry_test.go | 55 ++++++++++++++------------ pkg/cmd/skills/install/install_test.go | 5 +++ pkg/cmd/skills/preview/preview_test.go | 5 +++ pkg/cmd/skills/search/search_test.go | 5 +++ pkg/cmdutil/telemetry.go | 2 +- pkg/cmdutil/telemetry_test.go | 5 +++ 9 files changed, 105 insertions(+), 38 deletions(-) diff --git a/internal/gh/ghtelemetry/telemetry.go b/internal/gh/ghtelemetry/telemetry.go index 197b955b4c1..98648ad37e3 100644 --- a/internal/gh/ghtelemetry/telemetry.go +++ b/internal/gh/ghtelemetry/telemetry.go @@ -10,12 +10,30 @@ type Event struct { Measures Measures } +// RecordOptions holds configuration for a single Record call. +type RecordOptions struct { + IncludeCommonDimensions bool +} + +// RecordOption configures how an event is recorded. +type RecordOption func(*RecordOptions) + +// IncludeCommonDimensions returns a RecordOption that causes common dimensions +// (device_id, invocation_id, os, architecture, etc.) to be merged into the +// event at flush time. Without this option, events only carry their own +// dimensions plus a timestamp. +func IncludeCommonDimensions() RecordOption { + return func(o *RecordOptions) { + o.IncludeCommonDimensions = true + } +} + type Disabler interface { Disable() } type EventRecorder interface { - Record(event Event) + Record(event Event, opts ...RecordOption) Disabler } diff --git a/internal/telemetry/fake.go b/internal/telemetry/fake.go index 4eb22e898a5..dca7bfaafa9 100644 --- a/internal/telemetry/fake.go +++ b/internal/telemetry/fake.go @@ -3,11 +3,17 @@ package telemetry import "github.com/cli/cli/v2/internal/gh/ghtelemetry" type EventRecorderSpy struct { - Events []ghtelemetry.Event + Events []ghtelemetry.Event + Options []ghtelemetry.RecordOptions } -func (r *EventRecorderSpy) Record(event ghtelemetry.Event) { +func (r *EventRecorderSpy) Record(event ghtelemetry.Event, opts ...ghtelemetry.RecordOption) { r.Events = append(r.Events, event) + var options ghtelemetry.RecordOptions + for _, opt := range opts { + opt(&options) + } + r.Options = append(r.Options, options) } func (r *EventRecorderSpy) Disable() {} @@ -19,11 +25,17 @@ func (r *EventRecorderSpy) Flush() {} // assert on the sampling behavior commands attempt to configure. type CommandRecorderSpy struct { Events []ghtelemetry.Event + Options []ghtelemetry.RecordOptions LastSampleRate int } -func (r *CommandRecorderSpy) Record(event ghtelemetry.Event) { +func (r *CommandRecorderSpy) Record(event ghtelemetry.Event, opts ...ghtelemetry.RecordOption) { r.Events = append(r.Events, event) + var options ghtelemetry.RecordOptions + for _, opt := range opts { + opt(&options) + } + r.Options = append(r.Options, options) } func (r *CommandRecorderSpy) Disable() {} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 3943060b124..f1f8d8a0cf4 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -247,8 +247,9 @@ func NewService(flusher func(SendTelemetryPayload), opts ...telemetryServiceOpti } type recordedEvent struct { - event ghtelemetry.Event - recordedAt time.Time + event ghtelemetry.Event + recordedAt time.Time + includeCommonDimensions bool } type service struct { @@ -272,11 +273,20 @@ func (s *service) Disable() { s.disabled = true } -func (s *service) Record(event ghtelemetry.Event) { +func (s *service) Record(event ghtelemetry.Event, opts ...ghtelemetry.RecordOption) { s.mu.Lock() defer s.mu.Unlock() - s.events = append(s.events, recordedEvent{event: event, recordedAt: time.Now()}) + var options ghtelemetry.RecordOptions + for _, opt := range opts { + opt(&options) + } + + s.events = append(s.events, recordedEvent{ + event: event, + recordedAt: time.Now(), + includeCommonDimensions: options.IncludeCommonDimensions, + }) } func (s *service) SetSampleRate(rate int) { @@ -284,7 +294,6 @@ func (s *service) SetSampleRate(rate int) { defer s.mu.Unlock() s.sampleRate = rate - s.commonDimensions["sample_rate"] = strconv.Itoa(rate) } func (s *service) Flush() { @@ -316,9 +325,12 @@ func (s *service) Flush() { for i, recorded := range events { dimensions := map[string]string{ - "timestamp": recorded.recordedAt.UTC().Format("2006-01-02T15:04:05.000Z"), + "timestamp": recorded.recordedAt.UTC().Format("2006-01-02T15:04:05.000Z"), + "sample_rate": strconv.Itoa(s.sampleRate), + } + if recorded.includeCommonDimensions { + maps.Copy(dimensions, s.commonDimensions) } - maps.Copy(dimensions, s.commonDimensions) maps.Copy(dimensions, recorded.event.Dimensions) payload.Events[i] = PayloadEvent{ @@ -416,7 +428,7 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { type NoOpService struct{} -func (s *NoOpService) Record(event ghtelemetry.Event) {} +func (s *NoOpService) Record(event ghtelemetry.Event, opts ...ghtelemetry.RecordOption) {} func (s *NoOpService) Disable() {} diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index 98180a1263c..9215e5ea573 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -374,7 +374,7 @@ func TestServiceDeviceIDFallback(t *testing.T) { var captured SendTelemetryPayload svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) - svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Record(ghtelemetry.Event{Type: "test"}, ghtelemetry.IncludeCommonDimensions()) svc.Flush() require.Len(t, captured.Events, 1) @@ -397,7 +397,7 @@ func TestServiceFlush(t *testing.T) { assert.Empty(t, captured.Events, "payload should have no events") }) - t.Run("flushes events with merged dimensions", func(t *testing.T) { + t.Run("flushes events with merged dimensions when IncludeCommonDimensions is set", func(t *testing.T) { t.Cleanup(stubDeviceID("test-device")) var captured SendTelemetryPayload @@ -407,7 +407,7 @@ func TestServiceFlush(t *testing.T) { Type: "command_invocation", Dimensions: map[string]string{"command": "gh pr list"}, Measures: map[string]int64{"duration_ms": 150}, - }) + }, ghtelemetry.IncludeCommonDimensions()) svc.Flush() require.Len(t, captured.Events, 1) @@ -421,6 +421,28 @@ func TestServiceFlush(t *testing.T) { assert.Equal(t, int64(150), event.Measures["duration_ms"]) }) + t.Run("excludes common dimensions by default", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, ghtelemetry.Dimensions{"version": "2.45.0"}) + + svc.Record(ghtelemetry.Event{ + Type: "skill_install", + Dimensions: map[string]string{"skill_name": "terraform"}, + }) + svc.Flush() + + require.Len(t, captured.Events, 1) + event := captured.Events[0] + assert.Equal(t, "skill_install", event.Type) + assert.Equal(t, "terraform", event.Dimensions["skill_name"]) + assert.NotEmpty(t, event.Dimensions["timestamp"]) + assert.NotEmpty(t, event.Dimensions["sample_rate"]) + assert.Empty(t, event.Dimensions["device_id"], "common dimensions should not be included by default") + assert.Empty(t, event.Dimensions["invocation_id"], "common dimensions should not be included by default") + }) + t.Run("flushes multiple events", func(t *testing.T) { t.Cleanup(stubDeviceID("test-device")) @@ -459,7 +481,7 @@ func TestServiceFlush(t *testing.T) { svc.Record(ghtelemetry.Event{ Type: "test", Dimensions: map[string]string{"shared": "event-level"}, - }) + }, ghtelemetry.IncludeCommonDimensions()) svc.Flush() require.Len(t, captured.Events, 1) @@ -579,38 +601,21 @@ func TestServiceSampling(t *testing.T) { assert.False(t, called, "flusher should not be called after SetSampleRate reduced the rate") }) - t.Run("SetSampleRate updates sample_rate dimension", func(t *testing.T) { + t.Run("SetSampleRate is reflected in sample_rate dimension", func(t *testing.T) { t.Cleanup(stubDeviceID("test-device")) var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, ghtelemetry.Dimensions{ - "sample_rate": "1", - }) + svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) svc.sampleRate = 1 svc.sampleBucket = 0 svc.SetSampleRate(100) - svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Record(ghtelemetry.Event{Type: "test"}, ghtelemetry.IncludeCommonDimensions()) svc.Flush() require.Len(t, captured.Events, 1) assert.Equal(t, "100", captured.Events[0].Dimensions["sample_rate"]) }) - - t.Run("WithSampleRate option sets rate on construction", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - called := false - svc := NewService(func(SendTelemetryPayload) { called = true }, WithSampleRate(1)) - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - // We can't control the bucket from NewService, so we just verify - // the service was created without error and Flush doesn't panic. - // The actual sampling behavior is tested via direct struct manipulation above. - _ = called - }) } func TestWithAdditionalCommonDimensions(t *testing.T) { @@ -625,7 +630,7 @@ func TestWithAdditionalCommonDimensions(t *testing.T) { }), ) - svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Record(ghtelemetry.Event{Type: "test"}, ghtelemetry.IncludeCommonDimensions()) svc.Flush() require.Len(t, captured.Events, 1) diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index b126f1ac788..6d0f3f179c3 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -2401,6 +2401,11 @@ func TestInstallRun_TelemetryVisibility(t *testing.T) { assert.Equal(t, "skill_install", event.Type) assert.NotEmpty(t, event.Dimensions["agent_hosts"], "agent_hosts should always be present") + // Skill events must not include common dimensions (device_id, etc.) + require.Len(t, recorder.Options, 1) + assert.False(t, recorder.Options[0].IncludeCommonDimensions, + "skill_install should not include common dimensions") + // skill_host_type is always recorded (categorized, no raw hostname for enterprise/tenancy). assert.Equal(t, "github.com", event.Dimensions["skill_host_type"]) diff --git a/pkg/cmd/skills/preview/preview_test.go b/pkg/cmd/skills/preview/preview_test.go index 3bc98fece56..38f470946a0 100644 --- a/pkg/cmd/skills/preview/preview_test.go +++ b/pkg/cmd/skills/preview/preview_test.go @@ -943,6 +943,11 @@ func TestPreviewRun_InteractiveTelemetryCapturesSelectedSkillName(t *testing.T) event := recorder.Events[0] assert.Equal(t, "skill_preview", event.Type) assert.Equal(t, "beta", event.Dimensions["skill_name"], "telemetry should capture the selected skill name, not the empty opts.SkillName") + + // Skill events must not include common dimensions (device_id, etc.) + require.Len(t, recorder.Options, 1) + assert.False(t, recorder.Options[0].IncludeCommonDimensions, + "skill_preview should not include common dimensions") } func TestPreviewRun_TelemetryVisibility(t *testing.T) { diff --git a/pkg/cmd/skills/search/search_test.go b/pkg/cmd/skills/search/search_test.go index cf66ba4acb4..7ffb59b094d 100644 --- a/pkg/cmd/skills/search/search_test.go +++ b/pkg/cmd/skills/search/search_test.go @@ -650,4 +650,9 @@ func TestSearchRun_TelemetryRecordsInstallFromResults(t *testing.T) { "skill_search_install must not record the search query") assert.Empty(t, installEvent.Dimensions["owner"], "skill_search_install must not record the search owner filter") + + // Skill events must not include common dimensions (device_id, etc.) + require.Len(t, recorder.Options, 1) + assert.False(t, recorder.Options[0].IncludeCommonDimensions, + "skill_search_install should not include common dimensions") } diff --git a/pkg/cmdutil/telemetry.go b/pkg/cmdutil/telemetry.go index 42169beecec..6e97802617b 100644 --- a/pkg/cmdutil/telemetry.go +++ b/pkg/cmdutil/telemetry.go @@ -34,7 +34,7 @@ func RecordTelemetry(cmd *cobra.Command, telemetry ghtelemetry.EventRecorder) { "command": cmd.CommandPath(), "flags": strings.Join(flags, ","), }, - }) + }, ghtelemetry.IncludeCommonDimensions()) return runErr } diff --git a/pkg/cmdutil/telemetry_test.go b/pkg/cmdutil/telemetry_test.go index bfe4c420ca0..7d4093c4ad1 100644 --- a/pkg/cmdutil/telemetry_test.go +++ b/pkg/cmdutil/telemetry_test.go @@ -37,6 +37,11 @@ func TestRecordTelemetry(t *testing.T) { assert.Equal(t, "command_invocation", event.Type) assert.Equal(t, "gh pr list", event.Dimensions["command"]) assert.Equal(t, "repo,web", event.Dimensions["flags"]) + + // command_invocation should opt in to common dimensions + require.Len(t, recorder.Options, 1) + assert.True(t, recorder.Options[0].IncludeCommonDimensions, + "command_invocation should include common dimensions") }) t.Run("is a no-op when original RunE is nil", func(t *testing.T) { From 3ec99ee5d97bc22d73f26a93fe3daff6b3489a0c Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 7 May 2026 20:22:48 +0200 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/gh/ghtelemetry/telemetry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gh/ghtelemetry/telemetry.go b/internal/gh/ghtelemetry/telemetry.go index 98648ad37e3..54abbf836df 100644 --- a/internal/gh/ghtelemetry/telemetry.go +++ b/internal/gh/ghtelemetry/telemetry.go @@ -21,7 +21,7 @@ type RecordOption func(*RecordOptions) // IncludeCommonDimensions returns a RecordOption that causes common dimensions // (device_id, invocation_id, os, architecture, etc.) to be merged into the // event at flush time. Without this option, events only carry their own -// dimensions plus a timestamp. +// event-specific dimensions plus a timestamp and sample_rate. func IncludeCommonDimensions() RecordOption { return func(o *RecordOptions) { o.IncludeCommonDimensions = true