Unhook common dimensions from skill telemetry events#13364
Open
williammartin wants to merge 2 commits into
Open
Unhook common dimensions from skill telemetry events#13364williammartin wants to merge 2 commits into
williammartin wants to merge 2 commits into
Conversation
c9c1e67 to
980e098
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
980e098 to
66dc276
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the telemetry recording API so callers must explicitly opt in to attaching “common dimensions” (device_id, invocation_id, os, architecture, version, etc.), with the goal of excluding these dimensions from skill telemetry events for improved privacy.
Changes:
- Introduces a functional-options pattern for
EventRecorder.Record(...), addingIncludeCommonDimensions()as an explicit opt-in. - Updates command invocation telemetry to opt in to common dimensions; skill events remain unchanged and therefore no longer receive common dimensions.
- Updates telemetry fakes and tests to validate the new opt-in/opt-out behavior and to ensure
sample_rate/timestampbehavior is preserved.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmdutil/telemetry.go | Opts command_invocation into common dimensions via IncludeCommonDimensions(). |
| pkg/cmdutil/telemetry_test.go | Asserts command_invocation explicitly includes common dimensions. |
| pkg/cmd/skills/search/search_test.go | Asserts skill telemetry does not opt into common dimensions. |
| pkg/cmd/skills/preview/preview_test.go | Asserts skill telemetry does not opt into common dimensions. |
| pkg/cmd/skills/install/install_test.go | Asserts skill telemetry does not opt into common dimensions. |
| internal/telemetry/telemetry.go | Implements per-event opt-in for common dimensions and moves sample_rate onto each event payload. |
| internal/telemetry/telemetry_test.go | Updates tests for opt-in behavior and adds coverage for default exclusion of common dimensions. |
| internal/telemetry/fake.go | Extends spies to capture per-Record call options for assertions. |
| internal/gh/ghtelemetry/telemetry.go | Adds RecordOption/RecordOptions and updates the EventRecorder interface signature. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Member
Author
|
@babakks feel free to take ownership of getting this PR in, if there are changes you want, I empower you to just make them. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes https://github.com/github/cli/issues/1159
Skill telemetry events (
skill_install,skill_preview,skill_search_install,skill_upstream_redirect) were including common dimensions (device_id,invocation_id,os,architecture,version, etc.) that we've decided are no longer necessary for skills analytics. Since they aren't necessary, better to be privacy conscious and unhook them.Reviewer Notes
This adds a functional options pattern to
Record()so callers must explicitly opt in to common dimensions viaIncludeCommonDimensions(). Events recorded without this option only get their own dimensions plus timestamp and sample rate. Skill command code is unchanged - they callRecord(event)without options, which now correctly excludes common dimensions.I've considered removing common dimensions altogether and requiring them to be passed when
command_invocationevents are recorded but I'd like to see how future telemetry plays out before unwinding and maybe rewinding that.