From 18dc5e77f0d74c83978705b5640c6e4912b18bad Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 14 Apr 2026 17:52:05 +0200 Subject: [PATCH 1/4] Add sampled command telemetry --- .gitignore | 4 + Makefile | 4 + acceptance/acceptance_test.go | 11 + .../telemetry/command-invocation.txtar | 9 + .../no-telemetry-for-completion.txtar | 7 + .../no-telemetry-for-extension.txtar | 27 + .../no-telemetry-for-send-telemetry.txtar | 14 + ...metry-failure-does-not-break-command.txtar | 8 + cmd/gen-docs/main.go | 3 +- go.mod | 3 +- go.sum | 2 + .../barista/observability/telemetry.pb.go | 289 +++++ .../barista/observability/telemetry.twirp.go | 1117 +++++++++++++++++ internal/config/config.go | 15 + internal/config/config_test.go | 31 + internal/config/stub.go | 3 + internal/gh/gh.go | 2 + internal/gh/ghtelemetry/telemetry.go | 27 + internal/gh/mock/config.go | 40 +- internal/ghcmd/cmd.go | 56 +- internal/telemetry/detach_unix.go | 12 + internal/telemetry/detach_windows.go | 16 + internal/telemetry/fake.go | 13 + internal/telemetry/telemetry.go | 384 ++++++ internal/telemetry/telemetry_test.go | 624 +++++++++ pkg/cmd/auth/auth.go | 2 + pkg/cmd/completion/completion.go | 1 + pkg/cmd/config/list/list_test.go | 1 + pkg/cmd/root/extension.go | 13 +- pkg/cmd/root/extension_registration_test.go | 3 +- pkg/cmd/root/help_test.go | 3 +- pkg/cmd/root/help_topic.go | 6 + pkg/cmd/root/root.go | 7 +- pkg/cmd/send-telemetry/send_telemetry.go | 135 ++ pkg/cmd/send-telemetry/send_telemetry_test.go | 226 ++++ pkg/cmd/version/version.go | 3 +- pkg/cmdutil/telemetry.go | 66 + pkg/cmdutil/telemetry_test.go | 168 +++ 38 files changed, 3337 insertions(+), 18 deletions(-) create mode 100644 acceptance/testdata/telemetry/command-invocation.txtar create mode 100644 acceptance/testdata/telemetry/no-telemetry-for-completion.txtar create mode 100644 acceptance/testdata/telemetry/no-telemetry-for-extension.txtar create mode 100644 acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar create mode 100644 acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar create mode 100644 internal/barista/observability/telemetry.pb.go create mode 100644 internal/barista/observability/telemetry.twirp.go create mode 100644 internal/gh/ghtelemetry/telemetry.go create mode 100644 internal/telemetry/detach_unix.go create mode 100644 internal/telemetry/detach_windows.go create mode 100644 internal/telemetry/fake.go create mode 100644 internal/telemetry/telemetry.go create mode 100644 internal/telemetry/telemetry_test.go create mode 100644 pkg/cmd/send-telemetry/send_telemetry.go create mode 100644 pkg/cmd/send-telemetry/send_telemetry_test.go create mode 100644 pkg/cmdutil/telemetry.go create mode 100644 pkg/cmdutil/telemetry_test.go diff --git a/.gitignore b/.gitignore index ffcbbb6c505..461b6a5a0e2 100644 --- a/.gitignore +++ b/.gitignore @@ -39,3 +39,7 @@ vendor/ gh + +# Test coverage artifacts +coverage.out +lcov.info diff --git a/Makefile b/Makefile index 4efdbfbed1b..fb8bf40911b 100644 --- a/Makefile +++ b/Makefile @@ -38,6 +38,10 @@ completions: bin/gh$(EXE) bin/gh$(EXE) completion -s fish > ./share/fish/vendor_completions.d/gh.fish bin/gh$(EXE) completion -s zsh > ./share/zsh/site-functions/_gh +.PHONY: lint +lint: + golangci-lint run ./... + # just convenience tasks around `go test` .PHONY: test test: diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 7c3c6f6ce2e..f978cc732be 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -182,6 +182,15 @@ func TestWorkflows(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "workflow")) } +func TestTelemetry(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "telemetry")) +} + func testScriptParamsFor(tsEnv testScriptEnv, command string) testscript.Params { var files []string if tsEnv.script != "" { @@ -226,6 +235,8 @@ func sharedSetup(tsEnv testScriptEnv) func(ts *testscript.Env) error { ts.Setenv("RANDOM_STRING", randomString(10)) + ts.Setenv("GH_TELEMETRY", "false") + // The sandbox overrides HOME, so git cannot find the user's global // config. Write a minimal identity so commits inside the sandbox // don't fail with "Author identity unknown". diff --git a/acceptance/testdata/telemetry/command-invocation.txtar b/acceptance/testdata/telemetry/command-invocation.txtar new file mode 100644 index 00000000000..86d668da5bf --- /dev/null +++ b/acceptance/testdata/telemetry/command-invocation.txtar @@ -0,0 +1,9 @@ +# Telemetry log mode outputs command invocation event to stderr +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 + +exec gh version +stderr 'Telemetry payload:' +stderr '"type": "command_invocation"' +stderr '"command": "gh version"' diff --git a/acceptance/testdata/telemetry/no-telemetry-for-completion.txtar b/acceptance/testdata/telemetry/no-telemetry-for-completion.txtar new file mode 100644 index 00000000000..ffde6e6059f --- /dev/null +++ b/acceptance/testdata/telemetry/no-telemetry-for-completion.txtar @@ -0,0 +1,7 @@ +# The completion command should not generate a telemetry event +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 + +exec gh completion -s bash +! stderr 'Telemetry payload:' diff --git a/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar b/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar new file mode 100644 index 00000000000..b87b3e252ed --- /dev/null +++ b/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar @@ -0,0 +1,27 @@ +# Extensions should not generate telemetry events +[!exec:bash] skip + +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 + +# Create a local shell extension repository +exec git init gh-hello +cp gh-hello.sh gh-hello/gh-hello +chmod 755 gh-hello/gh-hello +exec git -C gh-hello add gh-hello +exec git -C gh-hello commit -m 'init' + +# Install it locally +cd gh-hello +exec gh ext install . +cd $WORK + +# Run the extension and verify no telemetry is logged +exec gh hello +stdout 'hello from extension' +! stderr 'Telemetry payload:' + +-- gh-hello.sh -- +#!/usr/bin/env bash +echo "hello from extension" diff --git a/acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar b/acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar new file mode 100644 index 00000000000..7f9d0457aa9 --- /dev/null +++ b/acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar @@ -0,0 +1,14 @@ +# The send-telemetry command should not itself generate a telemetry event +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 +env GH_TELEMETRY_ENDPOINT_URL=http://localhost:1 + +# Provide a minimal valid payload on stdin so the command can run. +# It will fail to connect but that's fine — we only care about telemetry logging. +stdin payload.json +! exec gh send-telemetry +! stderr 'Telemetry payload:' + +-- payload.json -- +{"events":[{"type":"test","dimensions":{},"measures":{}}]} diff --git a/acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar b/acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar new file mode 100644 index 00000000000..ca1fc4b4ad2 --- /dev/null +++ b/acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar @@ -0,0 +1,8 @@ +# Command completes successfully even when telemetry endpoint is unreachable +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=enabled +env GH_TELEMETRY_SAMPLE_RATE=100 +env GH_TELEMETRY_ENDPOINT_URL=http://localhost:1 + +exec gh version +stdout 'gh version' diff --git a/cmd/gen-docs/main.go b/cmd/gen-docs/main.go index 60fd8af58d6..cb76f422087 100644 --- a/cmd/gen-docs/main.go +++ b/cmd/gen-docs/main.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/internal/docs" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmd/root" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/extensions" @@ -53,7 +54,7 @@ func run(args []string) error { return config.NewFromString(""), nil }, ExtensionManager: &em{}, - }, "", "") + }, &telemetry.NoOpService{}, "", "") rootCmd.InitDefaultHelpCmd() if err := os.MkdirAll(*dir, 0755); err != nil { diff --git a/go.mod b/go.mod index 0fc0b1a5e68..78f0185df8a 100644 --- a/go.mod +++ b/go.mod @@ -31,6 +31,7 @@ require ( github.com/google/go-cmp v0.7.0 github.com/google/go-containerregistry v0.21.4 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 + github.com/google/uuid v1.6.0 github.com/gorilla/websocket v1.5.3 github.com/hashicorp/go-version v1.9.0 github.com/hinshun/vt10x v0.0.0-20220119200601-820417d04eec @@ -52,6 +53,7 @@ require ( github.com/spf13/pflag v1.0.10 github.com/stretchr/testify v1.11.1 github.com/theupdateframework/go-tuf/v2 v2.4.1 + github.com/twitchtv/twirp v8.1.3+incompatible github.com/vmihailenco/msgpack/v5 v5.4.1 github.com/yuin/goldmark v1.8.2 github.com/zalando/go-keyring v0.2.8 @@ -129,7 +131,6 @@ require ( github.com/go-viper/mapstructure/v2 v2.5.0 // indirect github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/google/certificate-transparency-go v1.3.2 // indirect - github.com/google/uuid v1.6.0 // indirect github.com/gorilla/css v1.0.1 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.3 // indirect github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542 // indirect diff --git a/go.sum b/go.sum index b6e6f64ed51..681ee5b463c 100644 --- a/go.sum +++ b/go.sum @@ -526,6 +526,8 @@ github.com/transparency-dev/formats v0.0.0-20251017110053-404c0d5b696c h1:5a2XDQ github.com/transparency-dev/formats v0.0.0-20251017110053-404c0d5b696c/go.mod h1:g85IafeFJZLxlzZCDRu4JLpfS7HKzR+Hw9qRh3bVzDI= github.com/transparency-dev/merkle v0.0.2 h1:Q9nBoQcZcgPamMkGn7ghV8XiTZ/kRxn1yCG81+twTK4= github.com/transparency-dev/merkle v0.0.2/go.mod h1:pqSy+OXefQ1EDUVmAJ8MUhHB9TXGuzVAT58PqBoHz1A= +github.com/twitchtv/twirp v8.1.3+incompatible h1:+F4TdErPgSUbMZMwp13Q/KgDVuI7HJXP61mNV3/7iuU= +github.com/twitchtv/twirp v8.1.3+incompatible/go.mod h1:RRJoFSAmTEh2weEqWtpPE3vFK5YBhA6bqp2l1kfCC5A= github.com/vbatts/tar-split v0.12.2 h1:w/Y6tjxpeiFMR47yzZPlPj/FcPLpXbTUi/9H7d3CPa4= github.com/vbatts/tar-split v0.12.2/go.mod h1:eF6B6i6ftWQcDqEn3/iGFRFRo8cBIMSJVOpnNdfTMFA= github.com/vmihailenco/msgpack/v5 v5.4.1 h1:cQriyiUvjTwOHg8QZaPihLWeRAAVoCpE00IUPn0Bjt8= diff --git a/internal/barista/observability/telemetry.pb.go b/internal/barista/observability/telemetry.pb.go new file mode 100644 index 00000000000..db5a7d8f31c --- /dev/null +++ b/internal/barista/observability/telemetry.pb.go @@ -0,0 +1,289 @@ +// Code generated by protoc-gen-go. DO NOT EDIT. +// versions: +// protoc-gen-go v1.36.4 +// protoc v5.29.3 +// source: observability/v1/telemetry.proto + +package observability + +import ( + protoreflect "google.golang.org/protobuf/reflect/protoreflect" + protoimpl "google.golang.org/protobuf/runtime/protoimpl" + reflect "reflect" + sync "sync" + unsafe "unsafe" +) + +const ( + // Verify that this generated code is sufficiently up-to-date. + _ = protoimpl.EnforceVersion(20 - protoimpl.MinVersion) + // Verify that runtime/protoimpl is sufficiently up-to-date. + _ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20) +) + +// TelemetryEvent represents a single telemetry event from a client application. +type TelemetryEvent struct { + state protoimpl.MessageState `protogen:"open.v1"` + // Required. The client application that generated the event (e.g. "github-cli", "vscode"). + App string `protobuf:"bytes,1,opt,name=app,proto3" json:"app,omitempty"` + // Required. The type of event (e.g. "usage", "lifecycle", "error"). + EventType string `protobuf:"bytes,2,opt,name=event_type,json=eventType,proto3" json:"event_type,omitempty"` + // Key-value string dimensions describing the event (e.g. command, os, architecture). + Dimensions map[string]string `protobuf:"bytes,3,rep,name=dimensions,proto3" json:"dimensions,omitempty" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"bytes,2,opt,name=value"` + // Key-value numeric measures associated with the event (e.g. duration_ms, api_calls). + Measures map[string]int64 `protobuf:"bytes,4,rep,name=measures,proto3" json:"measures,omitempty" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"varint,2,opt,name=value"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *TelemetryEvent) Reset() { + *x = TelemetryEvent{} + mi := &file_observability_v1_telemetry_proto_msgTypes[0] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *TelemetryEvent) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*TelemetryEvent) ProtoMessage() {} + +func (x *TelemetryEvent) ProtoReflect() protoreflect.Message { + mi := &file_observability_v1_telemetry_proto_msgTypes[0] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +// Deprecated: Use TelemetryEvent.ProtoReflect.Descriptor instead. +func (*TelemetryEvent) Descriptor() ([]byte, []int) { + return file_observability_v1_telemetry_proto_rawDescGZIP(), []int{0} +} + +func (x *TelemetryEvent) GetApp() string { + if x != nil { + return x.App + } + return "" +} + +func (x *TelemetryEvent) GetEventType() string { + if x != nil { + return x.EventType + } + return "" +} + +func (x *TelemetryEvent) GetDimensions() map[string]string { + if x != nil { + return x.Dimensions + } + return nil +} + +func (x *TelemetryEvent) GetMeasures() map[string]int64 { + if x != nil { + return x.Measures + } + return nil +} + +// RecordEventsRequest contains a batch of telemetry events. +type RecordEventsRequest struct { + state protoimpl.MessageState `protogen:"open.v1"` + // Required. One or more telemetry events to record. + Events []*TelemetryEvent `protobuf:"bytes,1,rep,name=events,proto3" json:"events,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *RecordEventsRequest) Reset() { + *x = RecordEventsRequest{} + mi := &file_observability_v1_telemetry_proto_msgTypes[1] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *RecordEventsRequest) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*RecordEventsRequest) ProtoMessage() {} + +func (x *RecordEventsRequest) ProtoReflect() protoreflect.Message { + mi := &file_observability_v1_telemetry_proto_msgTypes[1] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +// Deprecated: Use RecordEventsRequest.ProtoReflect.Descriptor instead. +func (*RecordEventsRequest) Descriptor() ([]byte, []int) { + return file_observability_v1_telemetry_proto_rawDescGZIP(), []int{1} +} + +func (x *RecordEventsRequest) GetEvents() []*TelemetryEvent { + if x != nil { + return x.Events + } + return nil +} + +// RecordEventsResponse is intentionally empty. +type RecordEventsResponse struct { + state protoimpl.MessageState `protogen:"open.v1"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *RecordEventsResponse) Reset() { + *x = RecordEventsResponse{} + mi := &file_observability_v1_telemetry_proto_msgTypes[2] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *RecordEventsResponse) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*RecordEventsResponse) ProtoMessage() {} + +func (x *RecordEventsResponse) ProtoReflect() protoreflect.Message { + mi := &file_observability_v1_telemetry_proto_msgTypes[2] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +// Deprecated: Use RecordEventsResponse.ProtoReflect.Descriptor instead. +func (*RecordEventsResponse) Descriptor() ([]byte, []int) { + return file_observability_v1_telemetry_proto_rawDescGZIP(), []int{2} +} + +var File_observability_v1_telemetry_proto protoreflect.FileDescriptor + +var file_observability_v1_telemetry_proto_rawDesc = string([]byte{ + 0x0a, 0x20, 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, 0x61, 0x62, 0x69, 0x6c, 0x69, 0x74, 0x79, 0x2f, + 0x76, 0x31, 0x2f, 0x74, 0x65, 0x6c, 0x65, 0x6d, 0x65, 0x74, 0x72, 0x79, 0x2e, 0x70, 0x72, 0x6f, + 0x74, 0x6f, 0x12, 0x1d, 0x63, 0x6c, 0x69, 0x65, 0x6e, 0x74, 0x61, 0x70, 0x70, 0x73, 0x66, 0x65, + 0x2e, 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, 0x61, 0x62, 0x69, 0x6c, 0x69, 0x74, 0x79, 0x2e, 0x76, + 0x31, 0x22, 0xf5, 0x02, 0x0a, 0x0e, 0x54, 0x65, 0x6c, 0x65, 0x6d, 0x65, 0x74, 0x72, 0x79, 0x45, + 0x76, 0x65, 0x6e, 0x74, 0x12, 0x10, 0x0a, 0x03, 0x61, 0x70, 0x70, 0x18, 0x01, 0x20, 0x01, 0x28, + 0x09, 0x52, 0x03, 0x61, 0x70, 0x70, 0x12, 0x1d, 0x0a, 0x0a, 0x65, 0x76, 0x65, 0x6e, 0x74, 0x5f, + 0x74, 0x79, 0x70, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x09, 0x65, 0x76, 0x65, 0x6e, + 0x74, 0x54, 0x79, 0x70, 0x65, 0x12, 0x5d, 0x0a, 0x0a, 0x64, 0x69, 0x6d, 0x65, 0x6e, 0x73, 0x69, + 0x6f, 0x6e, 0x73, 0x18, 0x03, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x3d, 0x2e, 0x63, 0x6c, 0x69, 0x65, + 0x6e, 0x74, 0x61, 0x70, 0x70, 0x73, 0x66, 0x65, 0x2e, 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, 0x61, + 0x62, 0x69, 0x6c, 0x69, 0x74, 0x79, 0x2e, 0x76, 0x31, 0x2e, 0x54, 0x65, 0x6c, 0x65, 0x6d, 0x65, + 0x74, 0x72, 0x79, 0x45, 0x76, 0x65, 0x6e, 0x74, 0x2e, 0x44, 0x69, 0x6d, 0x65, 0x6e, 0x73, 0x69, + 0x6f, 0x6e, 0x73, 0x45, 0x6e, 0x74, 0x72, 0x79, 0x52, 0x0a, 0x64, 0x69, 0x6d, 0x65, 0x6e, 0x73, + 0x69, 0x6f, 0x6e, 0x73, 0x12, 0x57, 0x0a, 0x08, 0x6d, 0x65, 0x61, 0x73, 0x75, 0x72, 0x65, 0x73, + 0x18, 0x04, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x3b, 0x2e, 0x63, 0x6c, 0x69, 0x65, 0x6e, 0x74, 0x61, + 0x70, 0x70, 0x73, 0x66, 0x65, 0x2e, 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, 0x61, 0x62, 0x69, 0x6c, + 0x69, 0x74, 0x79, 0x2e, 0x76, 0x31, 0x2e, 0x54, 0x65, 0x6c, 0x65, 0x6d, 0x65, 0x74, 0x72, 0x79, + 0x45, 0x76, 0x65, 0x6e, 0x74, 0x2e, 0x4d, 0x65, 0x61, 0x73, 0x75, 0x72, 0x65, 0x73, 0x45, 0x6e, + 0x74, 0x72, 0x79, 0x52, 0x08, 0x6d, 0x65, 0x61, 0x73, 0x75, 0x72, 0x65, 0x73, 0x1a, 0x3d, 0x0a, + 0x0f, 0x44, 0x69, 0x6d, 0x65, 0x6e, 0x73, 0x69, 0x6f, 0x6e, 0x73, 0x45, 0x6e, 0x74, 0x72, 0x79, + 0x12, 0x10, 0x0a, 0x03, 0x6b, 0x65, 0x79, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x03, 0x6b, + 0x65, 0x79, 0x12, 0x14, 0x0a, 0x05, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, + 0x09, 0x52, 0x05, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x3a, 0x02, 0x38, 0x01, 0x1a, 0x3b, 0x0a, 0x0d, + 0x4d, 0x65, 0x61, 0x73, 0x75, 0x72, 0x65, 0x73, 0x45, 0x6e, 0x74, 0x72, 0x79, 0x12, 0x10, 0x0a, + 0x03, 0x6b, 0x65, 0x79, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x03, 0x6b, 0x65, 0x79, 0x12, + 0x14, 0x0a, 0x05, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x03, 0x52, 0x05, + 0x76, 0x61, 0x6c, 0x75, 0x65, 0x3a, 0x02, 0x38, 0x01, 0x22, 0x5c, 0x0a, 0x13, 0x52, 0x65, 0x63, + 0x6f, 0x72, 0x64, 0x45, 0x76, 0x65, 0x6e, 0x74, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, + 0x12, 0x45, 0x0a, 0x06, 0x65, 0x76, 0x65, 0x6e, 0x74, 0x73, 0x18, 0x01, 0x20, 0x03, 0x28, 0x0b, + 0x32, 0x2d, 0x2e, 0x63, 0x6c, 0x69, 0x65, 0x6e, 0x74, 0x61, 0x70, 0x70, 0x73, 0x66, 0x65, 0x2e, + 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, 0x61, 0x62, 0x69, 0x6c, 0x69, 0x74, 0x79, 0x2e, 0x76, 0x31, + 0x2e, 0x54, 0x65, 0x6c, 0x65, 0x6d, 0x65, 0x74, 0x72, 0x79, 0x45, 0x76, 0x65, 0x6e, 0x74, 0x52, + 0x06, 0x65, 0x76, 0x65, 0x6e, 0x74, 0x73, 0x22, 0x16, 0x0a, 0x14, 0x52, 0x65, 0x63, 0x6f, 0x72, + 0x64, 0x45, 0x76, 0x65, 0x6e, 0x74, 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x32, + 0x87, 0x01, 0x0a, 0x0c, 0x54, 0x65, 0x6c, 0x65, 0x6d, 0x65, 0x74, 0x72, 0x79, 0x41, 0x50, 0x49, + 0x12, 0x77, 0x0a, 0x0c, 0x52, 0x65, 0x63, 0x6f, 0x72, 0x64, 0x45, 0x76, 0x65, 0x6e, 0x74, 0x73, + 0x12, 0x32, 0x2e, 0x63, 0x6c, 0x69, 0x65, 0x6e, 0x74, 0x61, 0x70, 0x70, 0x73, 0x66, 0x65, 0x2e, + 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, 0x61, 0x62, 0x69, 0x6c, 0x69, 0x74, 0x79, 0x2e, 0x76, 0x31, + 0x2e, 0x52, 0x65, 0x63, 0x6f, 0x72, 0x64, 0x45, 0x76, 0x65, 0x6e, 0x74, 0x73, 0x52, 0x65, 0x71, + 0x75, 0x65, 0x73, 0x74, 0x1a, 0x33, 0x2e, 0x63, 0x6c, 0x69, 0x65, 0x6e, 0x74, 0x61, 0x70, 0x70, + 0x73, 0x66, 0x65, 0x2e, 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, 0x61, 0x62, 0x69, 0x6c, 0x69, 0x74, + 0x79, 0x2e, 0x76, 0x31, 0x2e, 0x52, 0x65, 0x63, 0x6f, 0x72, 0x64, 0x45, 0x76, 0x65, 0x6e, 0x74, + 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x42, 0x4d, 0x5a, 0x4b, 0x67, 0x69, 0x74, + 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2f, 0x63, + 0x6c, 0x69, 0x65, 0x6e, 0x74, 0x61, 0x70, 0x70, 0x73, 0x66, 0x65, 0x2f, 0x70, 0x6b, 0x67, 0x2f, + 0x61, 0x70, 0x69, 0x2f, 0x74, 0x77, 0x69, 0x72, 0x70, 0x2f, 0x6f, 0x62, 0x73, 0x65, 0x72, 0x76, + 0x61, 0x62, 0x69, 0x6c, 0x69, 0x74, 0x79, 0x2f, 0x76, 0x31, 0x3b, 0x6f, 0x62, 0x73, 0x65, 0x72, + 0x76, 0x61, 0x62, 0x69, 0x6c, 0x69, 0x74, 0x79, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, +}) + +var ( + file_observability_v1_telemetry_proto_rawDescOnce sync.Once + file_observability_v1_telemetry_proto_rawDescData []byte +) + +func file_observability_v1_telemetry_proto_rawDescGZIP() []byte { + file_observability_v1_telemetry_proto_rawDescOnce.Do(func() { + file_observability_v1_telemetry_proto_rawDescData = protoimpl.X.CompressGZIP(unsafe.Slice(unsafe.StringData(file_observability_v1_telemetry_proto_rawDesc), len(file_observability_v1_telemetry_proto_rawDesc))) + }) + return file_observability_v1_telemetry_proto_rawDescData +} + +var file_observability_v1_telemetry_proto_msgTypes = make([]protoimpl.MessageInfo, 5) +var file_observability_v1_telemetry_proto_goTypes = []any{ + (*TelemetryEvent)(nil), // 0: clientappsfe.observability.v1.TelemetryEvent + (*RecordEventsRequest)(nil), // 1: clientappsfe.observability.v1.RecordEventsRequest + (*RecordEventsResponse)(nil), // 2: clientappsfe.observability.v1.RecordEventsResponse + nil, // 3: clientappsfe.observability.v1.TelemetryEvent.DimensionsEntry + nil, // 4: clientappsfe.observability.v1.TelemetryEvent.MeasuresEntry +} +var file_observability_v1_telemetry_proto_depIdxs = []int32{ + 3, // 0: clientappsfe.observability.v1.TelemetryEvent.dimensions:type_name -> clientappsfe.observability.v1.TelemetryEvent.DimensionsEntry + 4, // 1: clientappsfe.observability.v1.TelemetryEvent.measures:type_name -> clientappsfe.observability.v1.TelemetryEvent.MeasuresEntry + 0, // 2: clientappsfe.observability.v1.RecordEventsRequest.events:type_name -> clientappsfe.observability.v1.TelemetryEvent + 1, // 3: clientappsfe.observability.v1.TelemetryAPI.RecordEvents:input_type -> clientappsfe.observability.v1.RecordEventsRequest + 2, // 4: clientappsfe.observability.v1.TelemetryAPI.RecordEvents:output_type -> clientappsfe.observability.v1.RecordEventsResponse + 4, // [4:5] is the sub-list for method output_type + 3, // [3:4] is the sub-list for method input_type + 3, // [3:3] is the sub-list for extension type_name + 3, // [3:3] is the sub-list for extension extendee + 0, // [0:3] is the sub-list for field type_name +} + +func init() { file_observability_v1_telemetry_proto_init() } +func file_observability_v1_telemetry_proto_init() { + if File_observability_v1_telemetry_proto != nil { + return + } + type x struct{} + out := protoimpl.TypeBuilder{ + File: protoimpl.DescBuilder{ + GoPackagePath: reflect.TypeOf(x{}).PkgPath(), + RawDescriptor: unsafe.Slice(unsafe.StringData(file_observability_v1_telemetry_proto_rawDesc), len(file_observability_v1_telemetry_proto_rawDesc)), + NumEnums: 0, + NumMessages: 5, + NumExtensions: 0, + NumServices: 1, + }, + GoTypes: file_observability_v1_telemetry_proto_goTypes, + DependencyIndexes: file_observability_v1_telemetry_proto_depIdxs, + MessageInfos: file_observability_v1_telemetry_proto_msgTypes, + }.Build() + File_observability_v1_telemetry_proto = out.File + file_observability_v1_telemetry_proto_goTypes = nil + file_observability_v1_telemetry_proto_depIdxs = nil +} diff --git a/internal/barista/observability/telemetry.twirp.go b/internal/barista/observability/telemetry.twirp.go new file mode 100644 index 00000000000..0068d6ca212 --- /dev/null +++ b/internal/barista/observability/telemetry.twirp.go @@ -0,0 +1,1117 @@ +// Code generated by protoc-gen-twirp v8.1.3, DO NOT EDIT. +// source: observability/v1/telemetry.proto + +package observability + +import context "context" +import fmt "fmt" +import http "net/http" +import io "io" +import json "encoding/json" +import strconv "strconv" +import strings "strings" + +import protojson "google.golang.org/protobuf/encoding/protojson" +import proto "google.golang.org/protobuf/proto" +import twirp "github.com/twitchtv/twirp" +import ctxsetters "github.com/twitchtv/twirp/ctxsetters" + +import bytes "bytes" +import errors "errors" +import path "path" +import url "net/url" + +// Version compatibility assertion. +// If the constant is not defined in the package, that likely means +// the package needs to be updated to work with this generated code. +// See https://twitchtv.github.io/twirp/docs/version_matrix.html +const _ = twirp.TwirpPackageMinVersion_8_1_0 + +// ====================== +// TelemetryAPI Interface +// ====================== + +// TelemetryAPI receives telemetry events from client applications. +// This endpoint is unauthenticated to support anonymous telemetry collection. +type TelemetryAPI interface { + // RecordEvents records a batch of telemetry events from a client application. + RecordEvents(context.Context, *RecordEventsRequest) (*RecordEventsResponse, error) +} + +// ============================ +// TelemetryAPI Protobuf Client +// ============================ + +type telemetryAPIProtobufClient struct { + client HTTPClient + urls [1]string + interceptor twirp.Interceptor + opts twirp.ClientOptions +} + +// NewTelemetryAPIProtobufClient creates a Protobuf client that implements the TelemetryAPI interface. +// It communicates using Protobuf and can be configured with a custom HTTPClient. +func NewTelemetryAPIProtobufClient(baseURL string, client HTTPClient, opts ...twirp.ClientOption) TelemetryAPI { + if c, ok := client.(*http.Client); ok { + client = withoutRedirects(c) + } + + clientOpts := twirp.ClientOptions{} + for _, o := range opts { + o(&clientOpts) + } + + // Using ReadOpt allows backwards and forwards compatibility with new options in the future + literalURLs := false + _ = clientOpts.ReadOpt("literalURLs", &literalURLs) + var pathPrefix string + if ok := clientOpts.ReadOpt("pathPrefix", &pathPrefix); !ok { + pathPrefix = "/twirp" // default prefix + } + + // Build method URLs: []/./ + serviceURL := sanitizeBaseURL(baseURL) + serviceURL += baseServicePath(pathPrefix, "clientappsfe.observability.v1", "TelemetryAPI") + urls := [1]string{ + serviceURL + "RecordEvents", + } + + return &telemetryAPIProtobufClient{ + client: client, + urls: urls, + interceptor: twirp.ChainInterceptors(clientOpts.Interceptors...), + opts: clientOpts, + } +} + +func (c *telemetryAPIProtobufClient) RecordEvents(ctx context.Context, in *RecordEventsRequest) (*RecordEventsResponse, error) { + ctx = ctxsetters.WithPackageName(ctx, "clientappsfe.observability.v1") + ctx = ctxsetters.WithServiceName(ctx, "TelemetryAPI") + ctx = ctxsetters.WithMethodName(ctx, "RecordEvents") + caller := c.callRecordEvents + if c.interceptor != nil { + caller = func(ctx context.Context, req *RecordEventsRequest) (*RecordEventsResponse, error) { + resp, err := c.interceptor( + func(ctx context.Context, req interface{}) (interface{}, error) { + typedReq, ok := req.(*RecordEventsRequest) + if !ok { + return nil, twirp.InternalError("failed type assertion req.(*RecordEventsRequest) when calling interceptor") + } + return c.callRecordEvents(ctx, typedReq) + }, + )(ctx, req) + if resp != nil { + typedResp, ok := resp.(*RecordEventsResponse) + if !ok { + return nil, twirp.InternalError("failed type assertion resp.(*RecordEventsResponse) when calling interceptor") + } + return typedResp, err + } + return nil, err + } + } + return caller(ctx, in) +} + +func (c *telemetryAPIProtobufClient) callRecordEvents(ctx context.Context, in *RecordEventsRequest) (*RecordEventsResponse, error) { + out := new(RecordEventsResponse) + ctx, err := doProtobufRequest(ctx, c.client, c.opts.Hooks, c.urls[0], in, out) + if err != nil { + twerr, ok := err.(twirp.Error) + if !ok { + twerr = twirp.InternalErrorWith(err) + } + callClientError(ctx, c.opts.Hooks, twerr) + return nil, err + } + + callClientResponseReceived(ctx, c.opts.Hooks) + + return out, nil +} + +// ======================== +// TelemetryAPI JSON Client +// ======================== + +type telemetryAPIJSONClient struct { + client HTTPClient + urls [1]string + interceptor twirp.Interceptor + opts twirp.ClientOptions +} + +// NewTelemetryAPIJSONClient creates a JSON client that implements the TelemetryAPI interface. +// It communicates using JSON and can be configured with a custom HTTPClient. +func NewTelemetryAPIJSONClient(baseURL string, client HTTPClient, opts ...twirp.ClientOption) TelemetryAPI { + if c, ok := client.(*http.Client); ok { + client = withoutRedirects(c) + } + + clientOpts := twirp.ClientOptions{} + for _, o := range opts { + o(&clientOpts) + } + + // Using ReadOpt allows backwards and forwards compatibility with new options in the future + literalURLs := false + _ = clientOpts.ReadOpt("literalURLs", &literalURLs) + var pathPrefix string + if ok := clientOpts.ReadOpt("pathPrefix", &pathPrefix); !ok { + pathPrefix = "/twirp" // default prefix + } + + // Build method URLs: []/./ + serviceURL := sanitizeBaseURL(baseURL) + serviceURL += baseServicePath(pathPrefix, "clientappsfe.observability.v1", "TelemetryAPI") + urls := [1]string{ + serviceURL + "RecordEvents", + } + + return &telemetryAPIJSONClient{ + client: client, + urls: urls, + interceptor: twirp.ChainInterceptors(clientOpts.Interceptors...), + opts: clientOpts, + } +} + +func (c *telemetryAPIJSONClient) RecordEvents(ctx context.Context, in *RecordEventsRequest) (*RecordEventsResponse, error) { + ctx = ctxsetters.WithPackageName(ctx, "clientappsfe.observability.v1") + ctx = ctxsetters.WithServiceName(ctx, "TelemetryAPI") + ctx = ctxsetters.WithMethodName(ctx, "RecordEvents") + caller := c.callRecordEvents + if c.interceptor != nil { + caller = func(ctx context.Context, req *RecordEventsRequest) (*RecordEventsResponse, error) { + resp, err := c.interceptor( + func(ctx context.Context, req interface{}) (interface{}, error) { + typedReq, ok := req.(*RecordEventsRequest) + if !ok { + return nil, twirp.InternalError("failed type assertion req.(*RecordEventsRequest) when calling interceptor") + } + return c.callRecordEvents(ctx, typedReq) + }, + )(ctx, req) + if resp != nil { + typedResp, ok := resp.(*RecordEventsResponse) + if !ok { + return nil, twirp.InternalError("failed type assertion resp.(*RecordEventsResponse) when calling interceptor") + } + return typedResp, err + } + return nil, err + } + } + return caller(ctx, in) +} + +func (c *telemetryAPIJSONClient) callRecordEvents(ctx context.Context, in *RecordEventsRequest) (*RecordEventsResponse, error) { + out := new(RecordEventsResponse) + ctx, err := doJSONRequest(ctx, c.client, c.opts.Hooks, c.urls[0], in, out) + if err != nil { + twerr, ok := err.(twirp.Error) + if !ok { + twerr = twirp.InternalErrorWith(err) + } + callClientError(ctx, c.opts.Hooks, twerr) + return nil, err + } + + callClientResponseReceived(ctx, c.opts.Hooks) + + return out, nil +} + +// =========================== +// TelemetryAPI Server Handler +// =========================== + +type telemetryAPIServer struct { + TelemetryAPI + interceptor twirp.Interceptor + hooks *twirp.ServerHooks + pathPrefix string // prefix for routing + jsonSkipDefaults bool // do not include unpopulated fields (default values) in the response + jsonCamelCase bool // JSON fields are serialized as lowerCamelCase rather than keeping the original proto names +} + +// NewTelemetryAPIServer builds a TwirpServer that can be used as an http.Handler to handle +// HTTP requests that are routed to the right method in the provided svc implementation. +// The opts are twirp.ServerOption modifiers, for example twirp.WithServerHooks(hooks). +func NewTelemetryAPIServer(svc TelemetryAPI, opts ...interface{}) TwirpServer { + serverOpts := newServerOpts(opts) + + // Using ReadOpt allows backwards and forwards compatibility with new options in the future + jsonSkipDefaults := false + _ = serverOpts.ReadOpt("jsonSkipDefaults", &jsonSkipDefaults) + jsonCamelCase := false + _ = serverOpts.ReadOpt("jsonCamelCase", &jsonCamelCase) + var pathPrefix string + if ok := serverOpts.ReadOpt("pathPrefix", &pathPrefix); !ok { + pathPrefix = "/twirp" // default prefix + } + + return &telemetryAPIServer{ + TelemetryAPI: svc, + hooks: serverOpts.Hooks, + interceptor: twirp.ChainInterceptors(serverOpts.Interceptors...), + pathPrefix: pathPrefix, + jsonSkipDefaults: jsonSkipDefaults, + jsonCamelCase: jsonCamelCase, + } +} + +// writeError writes an HTTP response with a valid Twirp error format, and triggers hooks. +// If err is not a twirp.Error, it will get wrapped with twirp.InternalErrorWith(err) +func (s *telemetryAPIServer) writeError(ctx context.Context, resp http.ResponseWriter, err error) { + writeError(ctx, resp, err, s.hooks) +} + +// handleRequestBodyError is used to handle error when the twirp server cannot read request +func (s *telemetryAPIServer) handleRequestBodyError(ctx context.Context, resp http.ResponseWriter, msg string, err error) { + if context.Canceled == ctx.Err() { + s.writeError(ctx, resp, twirp.NewError(twirp.Canceled, "failed to read request: context canceled")) + return + } + if context.DeadlineExceeded == ctx.Err() { + s.writeError(ctx, resp, twirp.NewError(twirp.DeadlineExceeded, "failed to read request: deadline exceeded")) + return + } + s.writeError(ctx, resp, twirp.WrapError(malformedRequestError(msg), err)) +} + +// TelemetryAPIPathPrefix is a convenience constant that may identify URL paths. +// Should be used with caution, it only matches routes generated by Twirp Go clients, +// with the default "/twirp" prefix and default CamelCase service and method names. +// More info: https://twitchtv.github.io/twirp/docs/routing.html +const TelemetryAPIPathPrefix = "/twirp/clientappsfe.observability.v1.TelemetryAPI/" + +func (s *telemetryAPIServer) ServeHTTP(resp http.ResponseWriter, req *http.Request) { + ctx := req.Context() + ctx = ctxsetters.WithPackageName(ctx, "clientappsfe.observability.v1") + ctx = ctxsetters.WithServiceName(ctx, "TelemetryAPI") + ctx = ctxsetters.WithResponseWriter(ctx, resp) + + var err error + ctx, err = callRequestReceived(ctx, s.hooks) + if err != nil { + s.writeError(ctx, resp, err) + return + } + + if req.Method != "POST" { + msg := fmt.Sprintf("unsupported method %q (only POST is allowed)", req.Method) + s.writeError(ctx, resp, badRouteError(msg, req.Method, req.URL.Path)) + return + } + + // Verify path format: []/./ + prefix, pkgService, method := parseTwirpPath(req.URL.Path) + if pkgService != "clientappsfe.observability.v1.TelemetryAPI" { + msg := fmt.Sprintf("no handler for path %q", req.URL.Path) + s.writeError(ctx, resp, badRouteError(msg, req.Method, req.URL.Path)) + return + } + if prefix != s.pathPrefix { + msg := fmt.Sprintf("invalid path prefix %q, expected %q, on path %q", prefix, s.pathPrefix, req.URL.Path) + s.writeError(ctx, resp, badRouteError(msg, req.Method, req.URL.Path)) + return + } + + switch method { + case "RecordEvents": + s.serveRecordEvents(ctx, resp, req) + return + default: + msg := fmt.Sprintf("no handler for path %q", req.URL.Path) + s.writeError(ctx, resp, badRouteError(msg, req.Method, req.URL.Path)) + return + } +} + +func (s *telemetryAPIServer) serveRecordEvents(ctx context.Context, resp http.ResponseWriter, req *http.Request) { + header := req.Header.Get("Content-Type") + i := strings.Index(header, ";") + if i == -1 { + i = len(header) + } + switch strings.TrimSpace(strings.ToLower(header[:i])) { + case "application/json": + s.serveRecordEventsJSON(ctx, resp, req) + case "application/protobuf": + s.serveRecordEventsProtobuf(ctx, resp, req) + default: + msg := fmt.Sprintf("unexpected Content-Type: %q", req.Header.Get("Content-Type")) + twerr := badRouteError(msg, req.Method, req.URL.Path) + s.writeError(ctx, resp, twerr) + } +} + +func (s *telemetryAPIServer) serveRecordEventsJSON(ctx context.Context, resp http.ResponseWriter, req *http.Request) { + var err error + ctx = ctxsetters.WithMethodName(ctx, "RecordEvents") + ctx, err = callRequestRouted(ctx, s.hooks) + if err != nil { + s.writeError(ctx, resp, err) + return + } + + d := json.NewDecoder(req.Body) + rawReqBody := json.RawMessage{} + if err := d.Decode(&rawReqBody); err != nil { + s.handleRequestBodyError(ctx, resp, "the json request could not be decoded", err) + return + } + reqContent := new(RecordEventsRequest) + unmarshaler := protojson.UnmarshalOptions{DiscardUnknown: true} + if err = unmarshaler.Unmarshal(rawReqBody, reqContent); err != nil { + s.handleRequestBodyError(ctx, resp, "the json request could not be decoded", err) + return + } + + handler := s.TelemetryAPI.RecordEvents + if s.interceptor != nil { + handler = func(ctx context.Context, req *RecordEventsRequest) (*RecordEventsResponse, error) { + resp, err := s.interceptor( + func(ctx context.Context, req interface{}) (interface{}, error) { + typedReq, ok := req.(*RecordEventsRequest) + if !ok { + return nil, twirp.InternalError("failed type assertion req.(*RecordEventsRequest) when calling interceptor") + } + return s.TelemetryAPI.RecordEvents(ctx, typedReq) + }, + )(ctx, req) + if resp != nil { + typedResp, ok := resp.(*RecordEventsResponse) + if !ok { + return nil, twirp.InternalError("failed type assertion resp.(*RecordEventsResponse) when calling interceptor") + } + return typedResp, err + } + return nil, err + } + } + + // Call service method + var respContent *RecordEventsResponse + func() { + defer ensurePanicResponses(ctx, resp, s.hooks) + respContent, err = handler(ctx, reqContent) + }() + + if err != nil { + s.writeError(ctx, resp, err) + return + } + if respContent == nil { + s.writeError(ctx, resp, twirp.InternalError("received a nil *RecordEventsResponse and nil error while calling RecordEvents. nil responses are not supported")) + return + } + + ctx = callResponsePrepared(ctx, s.hooks) + + marshaler := &protojson.MarshalOptions{UseProtoNames: !s.jsonCamelCase, EmitUnpopulated: !s.jsonSkipDefaults} + respBytes, err := marshaler.Marshal(respContent) + if err != nil { + s.writeError(ctx, resp, wrapInternal(err, "failed to marshal json response")) + return + } + + ctx = ctxsetters.WithStatusCode(ctx, http.StatusOK) + resp.Header().Set("Content-Type", "application/json") + resp.Header().Set("Content-Length", strconv.Itoa(len(respBytes))) + resp.WriteHeader(http.StatusOK) + + if n, err := resp.Write(respBytes); err != nil { + msg := fmt.Sprintf("failed to write response, %d of %d bytes written: %s", n, len(respBytes), err.Error()) + twerr := twirp.NewError(twirp.Unknown, msg) + ctx = callError(ctx, s.hooks, twerr) + } + callResponseSent(ctx, s.hooks) +} + +func (s *telemetryAPIServer) serveRecordEventsProtobuf(ctx context.Context, resp http.ResponseWriter, req *http.Request) { + var err error + ctx = ctxsetters.WithMethodName(ctx, "RecordEvents") + ctx, err = callRequestRouted(ctx, s.hooks) + if err != nil { + s.writeError(ctx, resp, err) + return + } + + buf, err := io.ReadAll(req.Body) + if err != nil { + s.handleRequestBodyError(ctx, resp, "failed to read request body", err) + return + } + reqContent := new(RecordEventsRequest) + if err = proto.Unmarshal(buf, reqContent); err != nil { + s.writeError(ctx, resp, malformedRequestError("the protobuf request could not be decoded")) + return + } + + handler := s.TelemetryAPI.RecordEvents + if s.interceptor != nil { + handler = func(ctx context.Context, req *RecordEventsRequest) (*RecordEventsResponse, error) { + resp, err := s.interceptor( + func(ctx context.Context, req interface{}) (interface{}, error) { + typedReq, ok := req.(*RecordEventsRequest) + if !ok { + return nil, twirp.InternalError("failed type assertion req.(*RecordEventsRequest) when calling interceptor") + } + return s.TelemetryAPI.RecordEvents(ctx, typedReq) + }, + )(ctx, req) + if resp != nil { + typedResp, ok := resp.(*RecordEventsResponse) + if !ok { + return nil, twirp.InternalError("failed type assertion resp.(*RecordEventsResponse) when calling interceptor") + } + return typedResp, err + } + return nil, err + } + } + + // Call service method + var respContent *RecordEventsResponse + func() { + defer ensurePanicResponses(ctx, resp, s.hooks) + respContent, err = handler(ctx, reqContent) + }() + + if err != nil { + s.writeError(ctx, resp, err) + return + } + if respContent == nil { + s.writeError(ctx, resp, twirp.InternalError("received a nil *RecordEventsResponse and nil error while calling RecordEvents. nil responses are not supported")) + return + } + + ctx = callResponsePrepared(ctx, s.hooks) + + respBytes, err := proto.Marshal(respContent) + if err != nil { + s.writeError(ctx, resp, wrapInternal(err, "failed to marshal proto response")) + return + } + + ctx = ctxsetters.WithStatusCode(ctx, http.StatusOK) + resp.Header().Set("Content-Type", "application/protobuf") + resp.Header().Set("Content-Length", strconv.Itoa(len(respBytes))) + resp.WriteHeader(http.StatusOK) + if n, err := resp.Write(respBytes); err != nil { + msg := fmt.Sprintf("failed to write response, %d of %d bytes written: %s", n, len(respBytes), err.Error()) + twerr := twirp.NewError(twirp.Unknown, msg) + ctx = callError(ctx, s.hooks, twerr) + } + callResponseSent(ctx, s.hooks) +} + +func (s *telemetryAPIServer) ServiceDescriptor() ([]byte, int) { + return twirpFileDescriptor0, 0 +} + +func (s *telemetryAPIServer) ProtocGenTwirpVersion() string { + return "v8.1.3" +} + +// PathPrefix returns the base service path, in the form: "//./" +// that is everything in a Twirp route except for the . This can be used for routing, +// for example to identify the requests that are targeted to this service in a mux. +func (s *telemetryAPIServer) PathPrefix() string { + return baseServicePath(s.pathPrefix, "clientappsfe.observability.v1", "TelemetryAPI") +} + +// ===== +// Utils +// ===== + +// HTTPClient is the interface used by generated clients to send HTTP requests. +// It is fulfilled by *(net/http).Client, which is sufficient for most users. +// Users can provide their own implementation for special retry policies. +// +// HTTPClient implementations should not follow redirects. Redirects are +// automatically disabled if *(net/http).Client is passed to client +// constructors. See the withoutRedirects function in this file for more +// details. +type HTTPClient interface { + Do(req *http.Request) (*http.Response, error) +} + +// TwirpServer is the interface generated server structs will support: they're +// HTTP handlers with additional methods for accessing metadata about the +// service. Those accessors are a low-level API for building reflection tools. +// Most people can think of TwirpServers as just http.Handlers. +type TwirpServer interface { + http.Handler + + // ServiceDescriptor returns gzipped bytes describing the .proto file that + // this service was generated from. Once unzipped, the bytes can be + // unmarshalled as a + // google.golang.org/protobuf/types/descriptorpb.FileDescriptorProto. + // + // The returned integer is the index of this particular service within that + // FileDescriptorProto's 'Service' slice of ServiceDescriptorProtos. This is a + // low-level field, expected to be used for reflection. + ServiceDescriptor() ([]byte, int) + + // ProtocGenTwirpVersion is the semantic version string of the version of + // twirp used to generate this file. + ProtocGenTwirpVersion() string + + // PathPrefix returns the HTTP URL path prefix for all methods handled by this + // service. This can be used with an HTTP mux to route Twirp requests. + // The path prefix is in the form: "//./" + // that is, everything in a Twirp route except for the at the end. + PathPrefix() string +} + +func newServerOpts(opts []interface{}) *twirp.ServerOptions { + serverOpts := &twirp.ServerOptions{} + for _, opt := range opts { + switch o := opt.(type) { + case twirp.ServerOption: + o(serverOpts) + case *twirp.ServerHooks: // backwards compatibility, allow to specify hooks as an argument + twirp.WithServerHooks(o)(serverOpts) + case nil: // backwards compatibility, allow nil value for the argument + continue + default: + panic(fmt.Sprintf("Invalid option type %T, please use a twirp.ServerOption", o)) + } + } + return serverOpts +} + +// WriteError writes an HTTP response with a valid Twirp error format (code, msg, meta). +// Useful outside of the Twirp server (e.g. http middleware), but does not trigger hooks. +// If err is not a twirp.Error, it will get wrapped with twirp.InternalErrorWith(err) +func WriteError(resp http.ResponseWriter, err error) { + writeError(context.Background(), resp, err, nil) +} + +// writeError writes Twirp errors in the response and triggers hooks. +func writeError(ctx context.Context, resp http.ResponseWriter, err error, hooks *twirp.ServerHooks) { + // Convert to a twirp.Error. Non-twirp errors are converted to internal errors. + var twerr twirp.Error + if !errors.As(err, &twerr) { + twerr = twirp.InternalErrorWith(err) + } + + statusCode := twirp.ServerHTTPStatusFromErrorCode(twerr.Code()) + ctx = ctxsetters.WithStatusCode(ctx, statusCode) + ctx = callError(ctx, hooks, twerr) + + respBody := marshalErrorToJSON(twerr) + + resp.Header().Set("Content-Type", "application/json") // Error responses are always JSON + resp.Header().Set("Content-Length", strconv.Itoa(len(respBody))) + resp.WriteHeader(statusCode) // set HTTP status code and send response + + _, writeErr := resp.Write(respBody) + if writeErr != nil { + // We have three options here. We could log the error, call the Error + // hook, or just silently ignore the error. + // + // Logging is unacceptable because we don't have a user-controlled + // logger; writing out to stderr without permission is too rude. + // + // Calling the Error hook would confuse users: it would mean the Error + // hook got called twice for one request, which is likely to lead to + // duplicated log messages and metrics, no matter how well we document + // the behavior. + // + // Silently ignoring the error is our least-bad option. It's highly + // likely that the connection is broken and the original 'err' says + // so anyway. + _ = writeErr + } + + callResponseSent(ctx, hooks) +} + +// sanitizeBaseURL parses the the baseURL, and adds the "http" scheme if needed. +// If the URL is unparsable, the baseURL is returned unchanged. +func sanitizeBaseURL(baseURL string) string { + u, err := url.Parse(baseURL) + if err != nil { + return baseURL // invalid URL will fail later when making requests + } + if u.Scheme == "" { + u.Scheme = "http" + } + return u.String() +} + +// baseServicePath composes the path prefix for the service (without ). +// e.g.: baseServicePath("/twirp", "my.pkg", "MyService") +// +// returns => "/twirp/my.pkg.MyService/" +// +// e.g.: baseServicePath("", "", "MyService") +// +// returns => "/MyService/" +func baseServicePath(prefix, pkg, service string) string { + fullServiceName := service + if pkg != "" { + fullServiceName = pkg + "." + service + } + return path.Join("/", prefix, fullServiceName) + "/" +} + +// parseTwirpPath extracts path components form a valid Twirp route. +// Expected format: "[]/./" +// e.g.: prefix, pkgService, method := parseTwirpPath("/twirp/pkg.Svc/MakeHat") +func parseTwirpPath(path string) (string, string, string) { + parts := strings.Split(path, "/") + if len(parts) < 2 { + return "", "", "" + } + method := parts[len(parts)-1] + pkgService := parts[len(parts)-2] + prefix := strings.Join(parts[0:len(parts)-2], "/") + return prefix, pkgService, method +} + +// getCustomHTTPReqHeaders retrieves a copy of any headers that are set in +// a context through the twirp.WithHTTPRequestHeaders function. +// If there are no headers set, or if they have the wrong type, nil is returned. +func getCustomHTTPReqHeaders(ctx context.Context) http.Header { + header, ok := twirp.HTTPRequestHeaders(ctx) + if !ok || header == nil { + return nil + } + copied := make(http.Header) + for k, vv := range header { + if vv == nil { + copied[k] = nil + continue + } + copied[k] = make([]string, len(vv)) + copy(copied[k], vv) + } + return copied +} + +// newRequest makes an http.Request from a client, adding common headers. +func newRequest(ctx context.Context, url string, reqBody io.Reader, contentType string) (*http.Request, error) { + req, err := http.NewRequest("POST", url, reqBody) + if err != nil { + return nil, err + } + req = req.WithContext(ctx) + if customHeader := getCustomHTTPReqHeaders(ctx); customHeader != nil { + req.Header = customHeader + } + req.Header.Set("Accept", contentType) + req.Header.Set("Content-Type", contentType) + req.Header.Set("Twirp-Version", "v8.1.3") + return req, nil +} + +// JSON serialization for errors +type twerrJSON struct { + Code string `json:"code"` + Msg string `json:"msg"` + Meta map[string]string `json:"meta,omitempty"` +} + +// marshalErrorToJSON returns JSON from a twirp.Error, that can be used as HTTP error response body. +// If serialization fails, it will use a descriptive Internal error instead. +func marshalErrorToJSON(twerr twirp.Error) []byte { + // make sure that msg is not too large + msg := twerr.Msg() + if len(msg) > 1e6 { + msg = msg[:1e6] + } + + tj := twerrJSON{ + Code: string(twerr.Code()), + Msg: msg, + Meta: twerr.MetaMap(), + } + + buf, err := json.Marshal(&tj) + if err != nil { + buf = []byte("{\"type\": \"" + twirp.Internal + "\", \"msg\": \"There was an error but it could not be serialized into JSON\"}") // fallback + } + + return buf +} + +// errorFromResponse builds a twirp.Error from a non-200 HTTP response. +// If the response has a valid serialized Twirp error, then it's returned. +// If not, the response status code is used to generate a similar twirp +// error. See twirpErrorFromIntermediary for more info on intermediary errors. +func errorFromResponse(resp *http.Response) twirp.Error { + statusCode := resp.StatusCode + statusText := http.StatusText(statusCode) + + if isHTTPRedirect(statusCode) { + // Unexpected redirect: it must be an error from an intermediary. + // Twirp clients don't follow redirects automatically, Twirp only handles + // POST requests, redirects should only happen on GET and HEAD requests. + location := resp.Header.Get("Location") + msg := fmt.Sprintf("unexpected HTTP status code %d %q received, Location=%q", statusCode, statusText, location) + return twirpErrorFromIntermediary(statusCode, msg, location) + } + + respBodyBytes, err := io.ReadAll(resp.Body) + if err != nil { + return wrapInternal(err, "failed to read server error response body") + } + + var tj twerrJSON + dec := json.NewDecoder(bytes.NewReader(respBodyBytes)) + dec.DisallowUnknownFields() + if err := dec.Decode(&tj); err != nil || tj.Code == "" { + // Invalid JSON response; it must be an error from an intermediary. + msg := fmt.Sprintf("Error from intermediary with HTTP status code %d %q", statusCode, statusText) + return twirpErrorFromIntermediary(statusCode, msg, string(respBodyBytes)) + } + + errorCode := twirp.ErrorCode(tj.Code) + if !twirp.IsValidErrorCode(errorCode) { + msg := "invalid type returned from server error response: " + tj.Code + return twirp.InternalError(msg).WithMeta("body", string(respBodyBytes)) + } + + twerr := twirp.NewError(errorCode, tj.Msg) + for k, v := range tj.Meta { + twerr = twerr.WithMeta(k, v) + } + return twerr +} + +// twirpErrorFromIntermediary maps HTTP errors from non-twirp sources to twirp errors. +// The mapping is similar to gRPC: https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. +// Returned twirp Errors have some additional metadata for inspection. +func twirpErrorFromIntermediary(status int, msg string, bodyOrLocation string) twirp.Error { + var code twirp.ErrorCode + if isHTTPRedirect(status) { // 3xx + code = twirp.Internal + } else { + switch status { + case 400: // Bad Request + code = twirp.Internal + case 401: // Unauthorized + code = twirp.Unauthenticated + case 403: // Forbidden + code = twirp.PermissionDenied + case 404: // Not Found + code = twirp.BadRoute + case 429: // Too Many Requests + code = twirp.ResourceExhausted + case 502, 503, 504: // Bad Gateway, Service Unavailable, Gateway Timeout + code = twirp.Unavailable + default: // All other codes + code = twirp.Unknown + } + } + + twerr := twirp.NewError(code, msg) + twerr = twerr.WithMeta("http_error_from_intermediary", "true") // to easily know if this error was from intermediary + twerr = twerr.WithMeta("status_code", strconv.Itoa(status)) + if isHTTPRedirect(status) { + twerr = twerr.WithMeta("location", bodyOrLocation) + } else { + twerr = twerr.WithMeta("body", bodyOrLocation) + } + return twerr +} + +func isHTTPRedirect(status int) bool { + return status >= 300 && status <= 399 +} + +// wrapInternal wraps an error with a prefix as an Internal error. +// The original error cause is accessible by github.com/pkg/errors.Cause. +func wrapInternal(err error, prefix string) twirp.Error { + return twirp.InternalErrorWith(&wrappedError{prefix: prefix, cause: err}) +} + +type wrappedError struct { + prefix string + cause error +} + +func (e *wrappedError) Error() string { return e.prefix + ": " + e.cause.Error() } +func (e *wrappedError) Unwrap() error { return e.cause } // for go1.13 + errors.Is/As +func (e *wrappedError) Cause() error { return e.cause } // for github.com/pkg/errors + +// ensurePanicResponses makes sure that rpc methods causing a panic still result in a Twirp Internal +// error response (status 500), and error hooks are properly called with the panic wrapped as an error. +// The panic is re-raised so it can be handled normally with middleware. +func ensurePanicResponses(ctx context.Context, resp http.ResponseWriter, hooks *twirp.ServerHooks) { + if r := recover(); r != nil { + // Wrap the panic as an error so it can be passed to error hooks. + // The original error is accessible from error hooks, but not visible in the response. + err := errFromPanic(r) + twerr := &internalWithCause{msg: "Internal service panic", cause: err} + // Actually write the error + writeError(ctx, resp, twerr, hooks) + // If possible, flush the error to the wire. + f, ok := resp.(http.Flusher) + if ok { + f.Flush() + } + + panic(r) + } +} + +// errFromPanic returns the typed error if the recovered panic is an error, otherwise formats as error. +func errFromPanic(p interface{}) error { + if err, ok := p.(error); ok { + return err + } + return fmt.Errorf("panic: %v", p) +} + +// internalWithCause is a Twirp Internal error wrapping an original error cause, +// but the original error message is not exposed on Msg(). The original error +// can be checked with go1.13+ errors.Is/As, and also by (github.com/pkg/errors).Unwrap +type internalWithCause struct { + msg string + cause error +} + +func (e *internalWithCause) Unwrap() error { return e.cause } // for go1.13 + errors.Is/As +func (e *internalWithCause) Cause() error { return e.cause } // for github.com/pkg/errors +func (e *internalWithCause) Error() string { return e.msg + ": " + e.cause.Error() } +func (e *internalWithCause) Code() twirp.ErrorCode { return twirp.Internal } +func (e *internalWithCause) Msg() string { return e.msg } +func (e *internalWithCause) Meta(key string) string { return "" } +func (e *internalWithCause) MetaMap() map[string]string { return nil } +func (e *internalWithCause) WithMeta(key string, val string) twirp.Error { return e } + +// malformedRequestError is used when the twirp server cannot unmarshal a request +func malformedRequestError(msg string) twirp.Error { + return twirp.NewError(twirp.Malformed, msg) +} + +// badRouteError is used when the twirp server cannot route a request +func badRouteError(msg string, method, url string) twirp.Error { + err := twirp.NewError(twirp.BadRoute, msg) + err = err.WithMeta("twirp_invalid_route", method+" "+url) + return err +} + +// withoutRedirects makes sure that the POST request can not be redirected. +// The standard library will, by default, redirect requests (including POSTs) if it gets a 302 or +// 303 response, and also 301s in go1.8. It redirects by making a second request, changing the +// method to GET and removing the body. This produces very confusing error messages, so instead we +// set a redirect policy that always errors. This stops Go from executing the redirect. +// +// We have to be a little careful in case the user-provided http.Client has its own CheckRedirect +// policy - if so, we'll run through that policy first. +// +// Because this requires modifying the http.Client, we make a new copy of the client and return it. +func withoutRedirects(in *http.Client) *http.Client { + copy := *in + copy.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if in.CheckRedirect != nil { + // Run the input's redirect if it exists, in case it has side effects, but ignore any error it + // returns, since we want to use ErrUseLastResponse. + err := in.CheckRedirect(req, via) + _ = err // Silly, but this makes sure generated code passes errcheck -blank, which some people use. + } + return http.ErrUseLastResponse + } + return © +} + +// doProtobufRequest makes a Protobuf request to the remote Twirp service. +func doProtobufRequest(ctx context.Context, client HTTPClient, hooks *twirp.ClientHooks, url string, in, out proto.Message) (_ context.Context, err error) { + reqBodyBytes, err := proto.Marshal(in) + if err != nil { + return ctx, wrapInternal(err, "failed to marshal proto request") + } + reqBody := bytes.NewBuffer(reqBodyBytes) + if err = ctx.Err(); err != nil { + return ctx, wrapInternal(err, "aborted because context was done") + } + + req, err := newRequest(ctx, url, reqBody, "application/protobuf") + if err != nil { + return ctx, wrapInternal(err, "could not build request") + } + ctx, err = callClientRequestPrepared(ctx, hooks, req) + if err != nil { + return ctx, err + } + + req = req.WithContext(ctx) + resp, err := client.Do(req) + if err != nil { + return ctx, wrapInternal(err, "failed to do request") + } + defer func() { _ = resp.Body.Close() }() + + if err = ctx.Err(); err != nil { + return ctx, wrapInternal(err, "aborted because context was done") + } + + if resp.StatusCode != 200 { + return ctx, errorFromResponse(resp) + } + + respBodyBytes, err := io.ReadAll(resp.Body) + if err != nil { + return ctx, wrapInternal(err, "failed to read response body") + } + if err = ctx.Err(); err != nil { + return ctx, wrapInternal(err, "aborted because context was done") + } + + if err = proto.Unmarshal(respBodyBytes, out); err != nil { + return ctx, wrapInternal(err, "failed to unmarshal proto response") + } + return ctx, nil +} + +// doJSONRequest makes a JSON request to the remote Twirp service. +func doJSONRequest(ctx context.Context, client HTTPClient, hooks *twirp.ClientHooks, url string, in, out proto.Message) (_ context.Context, err error) { + marshaler := &protojson.MarshalOptions{UseProtoNames: true} + reqBytes, err := marshaler.Marshal(in) + if err != nil { + return ctx, wrapInternal(err, "failed to marshal json request") + } + if err = ctx.Err(); err != nil { + return ctx, wrapInternal(err, "aborted because context was done") + } + + req, err := newRequest(ctx, url, bytes.NewReader(reqBytes), "application/json") + if err != nil { + return ctx, wrapInternal(err, "could not build request") + } + ctx, err = callClientRequestPrepared(ctx, hooks, req) + if err != nil { + return ctx, err + } + + req = req.WithContext(ctx) + resp, err := client.Do(req) + if err != nil { + return ctx, wrapInternal(err, "failed to do request") + } + + defer func() { + cerr := resp.Body.Close() + if err == nil && cerr != nil { + err = wrapInternal(cerr, "failed to close response body") + } + }() + + if err = ctx.Err(); err != nil { + return ctx, wrapInternal(err, "aborted because context was done") + } + + if resp.StatusCode != 200 { + return ctx, errorFromResponse(resp) + } + + d := json.NewDecoder(resp.Body) + rawRespBody := json.RawMessage{} + if err := d.Decode(&rawRespBody); err != nil { + return ctx, wrapInternal(err, "failed to unmarshal json response") + } + unmarshaler := protojson.UnmarshalOptions{DiscardUnknown: true} + if err = unmarshaler.Unmarshal(rawRespBody, out); err != nil { + return ctx, wrapInternal(err, "failed to unmarshal json response") + } + if err = ctx.Err(); err != nil { + return ctx, wrapInternal(err, "aborted because context was done") + } + return ctx, nil +} + +// Call twirp.ServerHooks.RequestReceived if the hook is available +func callRequestReceived(ctx context.Context, h *twirp.ServerHooks) (context.Context, error) { + if h == nil || h.RequestReceived == nil { + return ctx, nil + } + return h.RequestReceived(ctx) +} + +// Call twirp.ServerHooks.RequestRouted if the hook is available +func callRequestRouted(ctx context.Context, h *twirp.ServerHooks) (context.Context, error) { + if h == nil || h.RequestRouted == nil { + return ctx, nil + } + return h.RequestRouted(ctx) +} + +// Call twirp.ServerHooks.ResponsePrepared if the hook is available +func callResponsePrepared(ctx context.Context, h *twirp.ServerHooks) context.Context { + if h == nil || h.ResponsePrepared == nil { + return ctx + } + return h.ResponsePrepared(ctx) +} + +// Call twirp.ServerHooks.ResponseSent if the hook is available +func callResponseSent(ctx context.Context, h *twirp.ServerHooks) { + if h == nil || h.ResponseSent == nil { + return + } + h.ResponseSent(ctx) +} + +// Call twirp.ServerHooks.Error if the hook is available +func callError(ctx context.Context, h *twirp.ServerHooks, err twirp.Error) context.Context { + if h == nil || h.Error == nil { + return ctx + } + return h.Error(ctx, err) +} + +func callClientResponseReceived(ctx context.Context, h *twirp.ClientHooks) { + if h == nil || h.ResponseReceived == nil { + return + } + h.ResponseReceived(ctx) +} + +func callClientRequestPrepared(ctx context.Context, h *twirp.ClientHooks, req *http.Request) (context.Context, error) { + if h == nil || h.RequestPrepared == nil { + return ctx, nil + } + return h.RequestPrepared(ctx, req) +} + +func callClientError(ctx context.Context, h *twirp.ClientHooks, err twirp.Error) { + if h == nil || h.Error == nil { + return + } + h.Error(ctx, err) +} + +var twirpFileDescriptor0 = []byte{ + // 353 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x94, 0x92, 0x4d, 0x4b, 0x02, 0x41, + 0x18, 0xc7, 0x59, 0xb7, 0x24, 0x9f, 0xec, 0x85, 0x49, 0x62, 0x11, 0x04, 0xf1, 0xe4, 0xa5, 0x1d, + 0xd4, 0x4b, 0x24, 0x1e, 0x8a, 0x3c, 0x44, 0x08, 0xb1, 0x08, 0x41, 0x14, 0xb1, 0xab, 0x4f, 0x36, + 0xb8, 0x2f, 0xd3, 0xce, 0xec, 0xca, 0x7c, 0x82, 0x3e, 0x71, 0xf7, 0x70, 0x56, 0x65, 0x57, 0x22, + 0xf1, 0x36, 0x3b, 0x33, 0xbf, 0xdf, 0xff, 0xf9, 0x2f, 0x03, 0xcd, 0xc8, 0x13, 0x18, 0xa7, 0xae, + 0xc7, 0x7c, 0x26, 0x15, 0x4d, 0x3b, 0x54, 0xa2, 0x8f, 0x01, 0xca, 0x58, 0xd9, 0x3c, 0x8e, 0x64, + 0x44, 0x1a, 0x13, 0x9f, 0x61, 0x28, 0x5d, 0xce, 0xc5, 0x07, 0xda, 0x85, 0xeb, 0x76, 0xda, 0x69, + 0xfd, 0x94, 0xe0, 0x74, 0xbc, 0x46, 0x86, 0x29, 0x86, 0x92, 0x9c, 0x83, 0xe9, 0x72, 0x6e, 0x19, + 0x4d, 0xa3, 0x5d, 0x71, 0x96, 0x4b, 0xd2, 0x00, 0xc0, 0xe5, 0xd1, 0xbb, 0x54, 0x1c, 0xad, 0x92, + 0x3e, 0xa8, 0xe8, 0x9d, 0xb1, 0xe2, 0x48, 0xde, 0x00, 0xa6, 0x2c, 0xc0, 0x50, 0xb0, 0x28, 0x14, + 0x96, 0xd9, 0x34, 0xdb, 0xc7, 0xdd, 0x81, 0xfd, 0x6f, 0xae, 0x5d, 0xcc, 0xb4, 0xef, 0x37, 0xfc, + 0x30, 0x94, 0xb1, 0x72, 0x72, 0x42, 0xf2, 0x0c, 0x47, 0x01, 0xba, 0x22, 0x89, 0x51, 0x58, 0x07, + 0x5a, 0xde, 0xdf, 0x4f, 0x3e, 0x5a, 0xd1, 0x99, 0x7a, 0x23, 0xab, 0x0f, 0xe0, 0x6c, 0x2b, 0x77, + 0xd9, 0x7d, 0x8e, 0x6a, 0xdd, 0x7d, 0x8e, 0x8a, 0xd4, 0xe0, 0x30, 0x75, 0xfd, 0x64, 0x5d, 0x3b, + 0xfb, 0xb8, 0x29, 0x5d, 0x1b, 0xf5, 0x3e, 0x9c, 0x14, 0xcc, 0xbb, 0x60, 0x33, 0x07, 0xb7, 0x5e, + 0xe1, 0xc2, 0xc1, 0x49, 0x14, 0x4f, 0xf5, 0x88, 0xc2, 0xc1, 0xaf, 0x04, 0x85, 0x24, 0x43, 0x28, + 0xeb, 0xff, 0x2a, 0x2c, 0x43, 0x37, 0xbd, 0xda, 0xab, 0xa9, 0xb3, 0x82, 0x5b, 0x97, 0x50, 0x2b, + 0xda, 0x05, 0x8f, 0x42, 0x81, 0xdd, 0x6f, 0x03, 0xaa, 0x1b, 0xe4, 0xf6, 0xe9, 0x81, 0x2c, 0xa0, + 0x9a, 0xbf, 0x48, 0xba, 0x3b, 0xf2, 0xfe, 0x98, 0xb9, 0xde, 0xdb, 0x8b, 0xc9, 0x26, 0xb9, 0x1b, + 0xbd, 0x3c, 0xce, 0x98, 0xfc, 0x4c, 0x3c, 0x7b, 0x12, 0x05, 0x34, 0x5b, 0xd2, 0xbc, 0x87, 0xf2, + 0xf9, 0x8c, 0xba, 0x9c, 0x51, 0xb9, 0x60, 0x31, 0xa7, 0xdb, 0xef, 0xbc, 0x5f, 0xd8, 0xf0, 0xca, + 0xfa, 0xb1, 0xf7, 0x7e, 0x03, 0x00, 0x00, 0xff, 0xff, 0xff, 0x5b, 0x87, 0x22, 0x10, 0x03, 0x00, + 0x00, +} diff --git a/internal/config/config.go b/internal/config/config.go index 4d83bd4e5a1..a694ca65461 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -31,6 +31,7 @@ const ( promptKey = "prompt" preferEditorPromptKey = "prefer_editor_prompt" spinnerKey = "spinner" + telemetryKey = "telemetry" userKey = "user" usersKey = "users" versionKey = "version" @@ -169,6 +170,11 @@ func (c *cfg) Spinner(hostname string) gh.ConfigEntry { return c.GetOrDefault(hostname, spinnerKey).Unwrap() } +func (c *cfg) Telemetry() gh.ConfigEntry { + // Intentionally panic if there is no user provided value or default value (which would be a programmer error) + return c.GetOrDefault("", telemetryKey).Unwrap() +} + func (c *cfg) Version() o.Option[string] { return c.get("", versionKey) } @@ -682,6 +688,15 @@ var Options = []ConfigOption{ return c.Spinner(hostname).Value }, }, + { + Key: telemetryKey, + Description: "whether telemetry is enabled, disabled, or logging", + DefaultValue: "enabled", + AllowedValues: []string{"enabled", "disabled", "log"}, + CurrentValue: func(c gh.Config, hostname string) string { + return c.Telemetry().Value + }, + }, } func HomeDirPath(subdir string) (string, error) { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 67a9a98d1ab..57cca23740f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -182,3 +182,34 @@ func TestSetUserSpecificKeyNoUserPresent(t *testing.T) { requireKeyWithValue(t, c.cfg, []string{hostsKey, host, key}, val) requireNoKey(t, c.cfg, []string{hostsKey, host, usersKey}) } + +func TestTelemetry(t *testing.T) { + t.Run("returns default when not configured", func(t *testing.T) { + c := newTestConfig() + + entry := c.Telemetry() + + require.Equal(t, "enabled", entry.Value) + require.Equal(t, gh.ConfigDefaultProvided, entry.Source) + }) + + t.Run("returns user configured value", func(t *testing.T) { + c := newTestConfig() + c.Set("", telemetryKey, "disabled") + + entry := c.Telemetry() + + require.Equal(t, "disabled", entry.Value) + require.Equal(t, gh.ConfigUserProvided, entry.Source) + }) + + t.Run("returns log when configured", func(t *testing.T) { + c := newTestConfig() + c.Set("", telemetryKey, "log") + + entry := c.Telemetry() + + require.Equal(t, "log", entry.Value) + require.Equal(t, gh.ConfigUserProvided, entry.Source) + }) +} diff --git a/internal/config/stub.go b/internal/config/stub.go index ea60254db85..fe5e277b62b 100644 --- a/internal/config/stub.go +++ b/internal/config/stub.go @@ -61,6 +61,9 @@ func NewFromString(cfgStr string) *ghmock.ConfigMock { mock.BrowserFunc = func(hostname string) gh.ConfigEntry { return cfg.Browser(hostname) } + mock.TelemetryFunc = func() gh.ConfigEntry { + return cfg.Telemetry() + } mock.ColorLabelsFunc = func(hostname string) gh.ConfigEntry { return cfg.ColorLabels(hostname) } diff --git a/internal/gh/gh.go b/internal/gh/gh.go index aa90a5268b6..759a931f2b7 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -57,6 +57,8 @@ type Config interface { PreferEditorPrompt(hostname string) ConfigEntry // Spinner returns the configured spinner setting, optionally scoped by host. Spinner(hostname string) ConfigEntry + // Telemetry returns the configured telemetry setting, ignoring host scoping since telemetry is a global setting. + Telemetry() ConfigEntry // Aliases provides persistent storage and modification of command aliases. Aliases() AliasConfig diff --git a/internal/gh/ghtelemetry/telemetry.go b/internal/gh/ghtelemetry/telemetry.go new file mode 100644 index 00000000000..c9256361b59 --- /dev/null +++ b/internal/gh/ghtelemetry/telemetry.go @@ -0,0 +1,27 @@ +package ghtelemetry + +type Dimensions map[string]string + +type Measures map[string]int64 + +type Event struct { + Type string + Dimensions Dimensions + Measures Measures +} + +type EventRecorder interface { + Record(event Event) +} + +type CommandRecorder interface { + EventRecorder + SetSampleRate(rate int) +} + +type Service interface { + CommandRecorder + Flush() +} + +const SAMPLE_ALL = 100 diff --git a/internal/gh/mock/config.go b/internal/gh/mock/config.go index 9f3f807993b..31e35cb1899 100644 --- a/internal/gh/mock/config.go +++ b/internal/gh/mock/config.go @@ -4,9 +4,10 @@ package ghmock import ( + "sync" + "github.com/cli/cli/v2/internal/gh" o "github.com/cli/cli/v2/pkg/option" - "sync" ) // Ensure, that ConfigMock does implement gh.Config. @@ -70,6 +71,9 @@ var _ gh.Config = &ConfigMock{} // SpinnerFunc: func(hostname string) gh.ConfigEntry { // panic("mock out the Spinner method") // }, +// TelemetryFunc: func() gh.ConfigEntry { +// panic("mock out the Telemetry method") +// }, // VersionFunc: func() o.Option[string] { // panic("mock out the Version method") // }, @@ -134,6 +138,9 @@ type ConfigMock struct { // SpinnerFunc mocks the Spinner method. SpinnerFunc func(hostname string) gh.ConfigEntry + // TelemetryFunc mocks the Telemetry method. + TelemetryFunc func() gh.ConfigEntry + // VersionFunc mocks the Version method. VersionFunc func() o.Option[string] @@ -227,6 +234,9 @@ type ConfigMock struct { // Hostname is the hostname argument value. Hostname string } + // Telemetry holds details about calls to the Telemetry method. + Telemetry []struct { + } // Version holds details about calls to the Version method. Version []struct { } @@ -251,6 +261,7 @@ type ConfigMock struct { lockPrompt sync.RWMutex lockSet sync.RWMutex lockSpinner sync.RWMutex + lockTelemetry sync.RWMutex lockVersion sync.RWMutex lockWrite sync.RWMutex } @@ -796,6 +807,33 @@ func (mock *ConfigMock) SpinnerCalls() []struct { return calls } +// Telemetry calls TelemetryFunc. +func (mock *ConfigMock) Telemetry() gh.ConfigEntry { + if mock.TelemetryFunc == nil { + panic("ConfigMock.TelemetryFunc: method is nil but Config.Telemetry was just called") + } + callInfo := struct { + }{} + mock.lockTelemetry.Lock() + mock.calls.Telemetry = append(mock.calls.Telemetry, callInfo) + mock.lockTelemetry.Unlock() + return mock.TelemetryFunc() +} + +// TelemetryCalls gets all the calls that were made to Telemetry. +// Check the length with: +// +// len(mockedConfig.TelemetryCalls()) +func (mock *ConfigMock) TelemetryCalls() []struct { +} { + var calls []struct { + } + mock.lockTelemetry.RLock() + calls = mock.calls.Telemetry + mock.lockTelemetry.RUnlock() + return calls +} + // Version calls VersionFunc. func (mock *ConfigMock) Version() o.Option[string] { if mock.VersionFunc == nil { diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index 8690078c66e..9112d428372 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" "time" @@ -19,6 +20,8 @@ import ( "github.com/cli/cli/v2/internal/build" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/config/migration" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/internal/update" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmd/root" @@ -48,16 +51,57 @@ func Main() exitCode { cmdFactory := factory.New(buildVersion, string(agents.Detect())) stderr := cmdFactory.IOStreams.ErrOut - ctx := context.Background() + cfg, err := cmdFactory.Config() + if err != nil { + fmt.Fprintf(stderr, "failed to load config: %s\n", err) + return exitError + } - if cfg, err := cmdFactory.Config(); err == nil { - var m migration.MultiAccount - if err := cfg.Migrate(m); err != nil { - fmt.Fprintln(stderr, err) + additionalCommonDimensions := ghtelemetry.Dimensions{ + "version": strings.TrimPrefix(buildVersion, "v"), + "is_tty": strconv.FormatBool(cmdFactory.IOStreams.IsStdoutTTY()), + "agent": string(agents.Detect()), + } + + var telemetryService ghtelemetry.Service + if os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" { + telemetryService = &telemetry.NoOpService{} + } else { + + telemetryState := telemetry.ParseTelemetryState(cfg.Telemetry().Value) + switch telemetryState { + case telemetry.Disabled: + telemetryService = &telemetry.NoOpService{} + case telemetry.Logged: + telemetryService = telemetry.NewService( + telemetry.LogFlusher(cmdFactory.IOStreams.ErrOut, cmdFactory.IOStreams.ColorEnabled()), + telemetry.WithAdditionalCommonDimensions(additionalCommonDimensions), + ) + case telemetry.Enabled: + sampleRate := 1 + if v, err := strconv.Atoi(os.Getenv("GH_TELEMETRY_SAMPLE_RATE")); err == nil && v >= 0 && v <= 100 { + sampleRate = v + } + additionalCommonDimensions["sample_rate"] = strconv.Itoa(sampleRate) + telemetryService = telemetry.NewService( + telemetry.GitHubFlusher(cmdFactory.Executable()), + telemetry.WithAdditionalCommonDimensions(additionalCommonDimensions), + telemetry.WithSampleRate(sampleRate), + ) + default: + fmt.Fprintf(stderr, "invalid telemetry configuration: %q\n", cfg.Telemetry().Value) return exitError } } + defer telemetryService.Flush() + + var m migration.MultiAccount + if err := cfg.Migrate(m); err != nil { + fmt.Fprintln(stderr, err) + return exitError + } + ctx := context.Background() updateCtx, updateCancel := context.WithCancel(ctx) defer updateCancel() updateMessageChan := make(chan *update.ReleaseInfo) @@ -90,7 +134,7 @@ func Main() exitCode { cobra.MousetrapHelpText = "" } - rootCmd, err := root.NewCmdRoot(cmdFactory, buildVersion, buildDate) + rootCmd, err := root.NewCmdRoot(cmdFactory, telemetryService, buildVersion, buildDate) if err != nil { fmt.Fprintf(stderr, "failed to create root command: %s\n", err) return exitError diff --git a/internal/telemetry/detach_unix.go b/internal/telemetry/detach_unix.go new file mode 100644 index 00000000000..f2f6011bcd9 --- /dev/null +++ b/internal/telemetry/detach_unix.go @@ -0,0 +1,12 @@ +//go:build !windows + +package telemetry + +import "syscall" + +// detachAttrs returns SysProcAttr configured to place the child in its own +// process group so that terminal signals delivered to the parent's group +// (SIGINT, SIGHUP) are not forwarded to the child. +func detachAttrs() *syscall.SysProcAttr { + return &syscall.SysProcAttr{Setpgid: true} +} diff --git a/internal/telemetry/detach_windows.go b/internal/telemetry/detach_windows.go new file mode 100644 index 00000000000..eb610163b5d --- /dev/null +++ b/internal/telemetry/detach_windows.go @@ -0,0 +1,16 @@ +//go:build windows + +package telemetry + +import ( + "syscall" + + "golang.org/x/sys/windows" +) + +// detachAttrs returns SysProcAttr configured to place the child in its own +// process group so that console signals (Ctrl+C) delivered to the parent's +// group are not forwarded to the child. +func detachAttrs() *syscall.SysProcAttr { + return &syscall.SysProcAttr{CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.DETACHED_PROCESS} +} diff --git a/internal/telemetry/fake.go b/internal/telemetry/fake.go new file mode 100644 index 00000000000..ee38262d9d5 --- /dev/null +++ b/internal/telemetry/fake.go @@ -0,0 +1,13 @@ +package telemetry + +import "github.com/cli/cli/v2/internal/gh/ghtelemetry" + +type EventRecorderSpy struct { + Events []ghtelemetry.Event +} + +func (r *EventRecorderSpy) Record(event ghtelemetry.Event) { + r.Events = append(r.Events, event) +} + +func (r *EventRecorderSpy) Flush() {} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go new file mode 100644 index 00000000000..f8698706ac3 --- /dev/null +++ b/internal/telemetry/telemetry.go @@ -0,0 +1,384 @@ +// Package telemetry provides best-effort usage telemetry for gh commands. +package telemetry + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "maps" + "os" + "os/exec" + "path/filepath" + "runtime" + "slices" + "strings" + "sync" + "time" + + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/cli/cli/v2/pkg/jsoncolor" + "github.com/google/uuid" + "github.com/mgutz/ansi" +) + +const deviceIDFileName = "device-id" + +// stateDirFunc returns the state directory path. Can be replaced in tests. +var stateDirFunc = config.StateDir + +// deviceIDFunc returns a per-user device identifier stored in the state directory. +// It generates and persists a UUID on first call. Can be replaced in tests. +var deviceIDFunc = getOrCreateDeviceID + +func getOrCreateDeviceID() (string, error) { + stateDir := stateDirFunc() + idPath := filepath.Join(stateDir, deviceIDFileName) + + data, err := os.ReadFile(idPath) + if err == nil { + return strings.TrimSpace(string(data)), nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + + id := uuid.New().String() + if err := os.MkdirAll(stateDir, 0o755); err != nil { + return "", err + } + + // Write the ID to a temp file in the same directory, then hard-link it + // to the target path. os.Link fails atomically if the target already + // exists, so exactly one concurrent caller wins. Losers read the + // winner's ID. The temp file is always cleaned up. + tmpFile, err := os.CreateTemp(stateDir, deviceIDFileName+".tmp.*") + if err != nil { + return "", err + } + tmpPath := tmpFile.Name() + + if _, err := tmpFile.WriteString(id); err != nil { + tmpFile.Close() + os.Remove(tmpPath) + return "", err + } + if err := tmpFile.Close(); err != nil { + os.Remove(tmpPath) + return "", err + } + + linkErr := os.Link(tmpPath, idPath) + os.Remove(tmpPath) + + if linkErr != nil { + // Another caller won — read their ID. + data, readErr := os.ReadFile(idPath) + if readErr != nil { + return "", linkErr + } + return strings.TrimSpace(string(data)), nil + } + + return id, nil +} + +var falseyValues = []string{"", "0", "false", "no", "disabled", "off"} + +// lookupEnvFunc wraps os.LookupEnv. Can be replaced in tests. +var lookupEnvFunc = os.LookupEnv + +type TelemetryState string + +const ( + Enabled TelemetryState = "enabled" + Disabled TelemetryState = "disabled" + Logged TelemetryState = "log" +) + +// ParseTelemetryState determines the telemetry state based on environment variables and configuration values. +// The GH_TELEMETRY environment variable takes precedence, followed by DO_NOT_TRACK, then the configuration value. +// Recognized values for GH_TELEMETRY and config are "enabled", "disabled", "log", or any falsey value (e.g. "0", "false", "no") to disable telemetry. +func ParseTelemetryState(configValue string) TelemetryState { + // GH_TELEMETRY env var takes highest precedence + if envVal, ok := lookupEnvFunc("GH_TELEMETRY"); ok { + envVal = strings.TrimSpace(strings.ToLower(envVal)) + + // If falsey, telemetry is disabled. + if slices.Contains(falseyValues, envVal) { + return Disabled + } + + // If logged, telemetry is logged instead of sent. + if envVal == "log" { + return Logged + } + + // Any other value (including "enabled") is treated as enabled. + return Enabled + } + + // DO_NOT_TRACK takes precedence over config + if envVal, ok := lookupEnvFunc("DO_NOT_TRACK"); ok { + envVal = strings.TrimSpace(strings.ToLower(envVal)) + if envVal == "1" || envVal == "true" { + return Disabled + } + } + + // Then check the config values with the same rules. + configValue = strings.TrimSpace(strings.ToLower(configValue)) + + if slices.Contains(falseyValues, configValue) { + return Disabled + } + + if configValue == "log" { + return Logged + } + + return Enabled +} + +type telemetryServiceOpts struct { + additionalDimensions ghtelemetry.Dimensions + sampleRate int +} + +type telemetryServiceOption func(*telemetryServiceOpts) + +// WithAdditionalCommonDimensions allows setting additional common dimensions that will be included with every telemetry event recorded by the service. +func WithAdditionalCommonDimensions(dimensions ghtelemetry.Dimensions) telemetryServiceOption { + return func(s *telemetryServiceOpts) { + maps.Copy(s.additionalDimensions, dimensions) + } +} + +// WithSampleRate allows setting a sample rate (0-100) for telemetry events. Events recorded with the Unsampled option will be sent regardless of the sample rate. +// Sampling is based on invocation ID, so an entire invocation will be included or excluded as a whole. This ensures that related events are not split between sampled and unsampled, +// which could lead to incomplete data and incorrect assumptions. +func WithSampleRate(rate int) telemetryServiceOption { + return func(s *telemetryServiceOpts) { + s.sampleRate = rate + } +} + +// LogFlusher returns a flush function that writes telemetry payloads to the provided log writer. This is used for the "log" telemetry mode, which is intended for debugging and development. +var LogFlusher = func(log io.Writer, colorEnabled bool) func(payload SendTelemetryPayload) { + return func(payload SendTelemetryPayload) { + payloadBytes, err := json.Marshal(payload) + if err != nil { + return + } + + header := "Telemetry payload:" + if colorEnabled { + header = ansi.Color(header, "cyan+b") + } + fmt.Fprintf(log, "%s\n", header) + + if colorEnabled { + _ = jsoncolor.Write(log, bytes.NewReader(payloadBytes), " ") + } else { + var indented bytes.Buffer + _ = json.Indent(&indented, payloadBytes, "", " ") + fmt.Fprintln(log, indented.String()) + } + } +} + +// GitHubFlusher returns a flush function that sends telemetry payloads to a child `gh send-telemetry` process. This is used for the "enabled" telemetry mode. +var GitHubFlusher = func(executable string) func(payload SendTelemetryPayload) { + return func(payload SendTelemetryPayload) { + SpawnSendTelemetry(executable, payload) + } +} + +// NewService creates a new telemetry service with the provided flush function and options. +func NewService(flusher func(SendTelemetryPayload), opts ...telemetryServiceOption) ghtelemetry.Service { + telemetryServiceOpts := telemetryServiceOpts{ + additionalDimensions: make(ghtelemetry.Dimensions), + } + for _, opt := range opts { + opt(&telemetryServiceOpts) + } + + deviceID, err := deviceIDFunc() + if err != nil { + deviceID = "" + } + + invocationID := uuid.NewString() + + var commonDimensions = ghtelemetry.Dimensions{ + "device_id": deviceID, + "invocation_id": invocationID, + "os": runtime.GOOS, + "architecture": runtime.GOARCH, + } + maps.Copy(commonDimensions, telemetryServiceOpts.additionalDimensions) + + hash := uuid.NewSHA1(uuid.Nil, []byte(invocationID)) + sampleBucket := hash[0] % 100 + + s := &service{ + flush: flusher, + commonDimensions: commonDimensions, + sampleRate: telemetryServiceOpts.sampleRate, + sampleBucket: sampleBucket, + } + + return s +} + +type recordedEvent struct { + event ghtelemetry.Event + recordedAt time.Time +} + +type service struct { + mu sync.RWMutex + flush func(payload SendTelemetryPayload) + previouslyCalled bool + + commonDimensions ghtelemetry.Dimensions + sampleRate int + sampleBucket byte + + events []recordedEvent +} + +func (s *service) Record(event ghtelemetry.Event) { + s.mu.Lock() + defer s.mu.Unlock() + + s.events = append(s.events, recordedEvent{event: event, recordedAt: time.Now()}) +} + +func (s *service) SetSampleRate(rate int) { + s.mu.Lock() + defer s.mu.Unlock() + + s.sampleRate = rate +} + +func (s *service) Flush() { + // This shouldn't really be required since flush should only be called once, but just in case... + s.mu.Lock() + defer s.mu.Unlock() + + if s.previouslyCalled { + return + } + s.previouslyCalled = true + + if len(s.events) == 0 { + return + } + + if s.sampleRate > 0 && s.sampleRate < 100 && int(s.sampleBucket) >= s.sampleRate { + return + } + + payload := SendTelemetryPayload{ + Events: make([]PayloadEvent, len(s.events)), + } + + for i, recorded := range s.events { + dimensions := map[string]string{ + "timestamp": recorded.recordedAt.UTC().Format("2006-01-02T15:04:05.000Z"), + } + maps.Copy(dimensions, s.commonDimensions) + maps.Copy(dimensions, recorded.event.Dimensions) + + payload.Events[i] = PayloadEvent{ + Type: recorded.event.Type, + Dimensions: dimensions, + Measures: recorded.event.Measures, + } + } + + s.flush(payload) +} + +// maxPayloadSize is a safety limit for the telemetry payload written to the +// child process stdin pipe. This bounds the data transferred to a reasonable +// size and avoids blocking on pipe buffer capacity (typically 16-64 KB). +const maxPayloadSize = 16 * 1024 + +// PayloadEvent represents a single telemetry event in the wire format. +type PayloadEvent struct { + Type string `json:"type"` + Dimensions map[string]string `json:"dimensions,omitempty"` + Measures map[string]int64 `json:"measures,omitempty"` +} + +type SendTelemetryPayload struct { + Events []PayloadEvent `json:"events"` +} + +// SpawnSendTelemetry spawns a detached subprocess to send telemetry. +// The payload is written to the child's stdin via a pipe so that it is not +// visible to other users through process argument inspection (e.g. ps aux). +// The parent writes the full payload and closes the pipe before returning, +// so no long-lived pipe is needed and the parent can exit immediately. +// +// Note: the payload is bounded by maxPayloadSize (16 KB). On macOS the +// default pipe buffer is also 16 KB, so in theory a write could block +// briefly if the child hasn't started reading yet. In practice the child +// is already running after cmd.Start(), so this is unlikely. +// +// All errors are silently ignored since telemetry is best-effort. +func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { + payloadBytes, err := json.Marshal(payload) + if err != nil { + return + } + + if len(payloadBytes) > maxPayloadSize { + return + } + + cmd := exec.Command(executable, "send-telemetry") + + cmd.Stdout = io.Discard + cmd.Stderr = io.Discard + + // Set the working directory to a stable directory elsewhere so that the subprocess doesn't + // hold a reference to the parent's current working directory, avoiding any weirdness around + // deleting the parent process's current working directory while the child is still running. + cmd.Dir = os.TempDir() + + // Configure the child process to be detached from the parent so that it can continue running + // after the parent exits, and so that it doesn't receive any signals sent to the parent. + cmd.SysProcAttr = detachAttrs() + + // Get the write end of the stdin pipe before starting. + stdin, err := cmd.StdinPipe() + if err != nil { + return + } + + if err := cmd.Start(); err != nil { + _ = stdin.Close() + return + } + + // Write the payload synchronously into the kernel pipe buffer, then close + // the pipe to signal EOF. The child reads the complete payload from stdin. + _, _ = stdin.Write(payloadBytes) + _ = stdin.Close() + + // Release resources associated with the child process since we will never Wait for it. + _ = cmd.Process.Release() +} + +type NoOpService struct{} + +func (s *NoOpService) Record(event ghtelemetry.Event) {} + +func (s *NoOpService) SetSampleRate(rate int) {} + +func (s *NoOpService) Flush() {} diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go new file mode 100644 index 00000000000..0142d4d1611 --- /dev/null +++ b/internal/telemetry/telemetry_test.go @@ -0,0 +1,624 @@ +package telemetry + +import ( + "bytes" + "errors" + "maps" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func stubStateDir(dir string) func() { + orig := stateDirFunc + stateDirFunc = func() string { return dir } + return func() { stateDirFunc = orig } +} + +func stubDeviceID(id string) func() { + orig := deviceIDFunc + deviceIDFunc = func() (string, error) { return id, nil } + return func() { deviceIDFunc = orig } +} + +func stubDeviceIDError(err error) func() { + orig := deviceIDFunc + deviceIDFunc = func() (string, error) { return "", err } + return func() { deviceIDFunc = orig } +} + +func stubLookupEnv(fn func(string) (string, bool)) func() { + orig := lookupEnvFunc + lookupEnvFunc = fn + return func() { lookupEnvFunc = orig } +} + +// newService is a test helper that constructs the internal service struct +// directly, bypassing the config/env parsing of NewService but still +// resolving common dimensions like device_id and invocation_id. +func newService(flusher func(SendTelemetryPayload), additionalDimensions ghtelemetry.Dimensions) *service { + deviceID, err := deviceIDFunc() + if err != nil { + deviceID = "" + } + + commonDimensions := ghtelemetry.Dimensions{ + "device_id": deviceID, + "invocation_id": uuid.NewString(), + } + maps.Copy(commonDimensions, additionalDimensions) + + return &service{ + flush: flusher, + commonDimensions: commonDimensions, + } +} + +func TestGetOrCreateDeviceID(t *testing.T) { + t.Run("creates new ID on first call", func(t *testing.T) { + tmpDir := t.TempDir() + t.Cleanup(stubStateDir(tmpDir)) + + id, err := getOrCreateDeviceID() + require.NoError(t, err) + require.NotEmpty(t, id) + + data, err := os.ReadFile(filepath.Join(tmpDir, deviceIDFileName)) + require.NoError(t, err) + assert.Equal(t, id, string(data)) + }) + + t.Run("returns same ID on subsequent calls", func(t *testing.T) { + tmpDir := t.TempDir() + t.Cleanup(stubStateDir(tmpDir)) + + id1, err := getOrCreateDeviceID() + require.NoError(t, err) + + id2, err := getOrCreateDeviceID() + require.NoError(t, err) + + assert.Equal(t, id1, id2) + }) + + t.Run("trims whitespace from stored ID", func(t *testing.T) { + tmpDir := t.TempDir() + t.Cleanup(stubStateDir(tmpDir)) + + err := os.WriteFile(filepath.Join(tmpDir, deviceIDFileName), []byte(" some-device-id\n"), 0o600) + require.NoError(t, err) + + id, err := getOrCreateDeviceID() + require.NoError(t, err) + assert.Equal(t, "some-device-id", id) + }) + + t.Run("returns error for non-ErrNotExist read failures", func(t *testing.T) { + tmpDir := t.TempDir() + t.Cleanup(stubStateDir(tmpDir)) + + // Create device-id as a directory so ReadFile fails with a non-ErrNotExist error. + err := os.Mkdir(filepath.Join(tmpDir, deviceIDFileName), 0o755) + require.NoError(t, err) + + _, err = getOrCreateDeviceID() + require.Error(t, err) + assert.False(t, errors.Is(err, os.ErrNotExist)) + }) + + t.Run("creates state directory if missing", func(t *testing.T) { + tmpDir := t.TempDir() + nestedDir := filepath.Join(tmpDir, "nested", "state") + t.Cleanup(stubStateDir(nestedDir)) + + id, err := getOrCreateDeviceID() + require.NoError(t, err) + require.NotEmpty(t, id) + + data, err := os.ReadFile(filepath.Join(nestedDir, deviceIDFileName)) + require.NoError(t, err) + assert.Equal(t, id, string(data)) + }) + + t.Run("concurrent callers converge on the same ID", func(t *testing.T) { + tmpDir := t.TempDir() + t.Cleanup(stubStateDir(tmpDir)) + + const goroutines = 10 + ids := make([]string, goroutines) + errs := make([]error, goroutines) + var wg sync.WaitGroup + wg.Add(goroutines) + for i := range goroutines { + go func() { + defer wg.Done() + ids[i], errs[i] = getOrCreateDeviceID() + }() + } + wg.Wait() + + for i := range goroutines { + require.NoError(t, errs[i]) + } + for i := 1; i < goroutines; i++ { + assert.Equal(t, ids[0], ids[i], "goroutine %d returned a different ID", i) + } + }) +} + +func TestParseTelemetryState(t *testing.T) { + envSet := func(val string) func(string) (string, bool) { + return func(string) (string, bool) { return val, true } + } + envUnset := func(string) (string, bool) { return "", false } + + // envMap allows setting multiple environment variables for testing DO_NOT_TRACK + GH_TELEMETRY interactions. + envMap := func(m map[string]string) func(string) (string, bool) { + return func(key string) (string, bool) { + val, ok := m[key] + return val, ok + } + } + + tests := []struct { + name string + lookupEnv func(string) (string, bool) + configValue string + want TelemetryState + }{ + { + name: "env unset, config empty string disables", + lookupEnv: envUnset, + configValue: "", + want: Disabled, + }, + { + name: "env unset, config enabled", + lookupEnv: envUnset, + configValue: "enabled", + want: Enabled, + }, + { + name: "env unset, config disabled", + lookupEnv: envUnset, + configValue: "disabled", + want: Disabled, + }, + { + name: "env unset, config log", + lookupEnv: envUnset, + configValue: "log", + want: Logged, + }, + { + name: "env unset, config false", + lookupEnv: envUnset, + configValue: "false", + want: Disabled, + }, + { + name: "env unset, config any truthy value", + lookupEnv: envUnset, + configValue: "anything", + want: Enabled, + }, + { + name: "env enabled takes precedence over config disabled", + lookupEnv: envSet("enabled"), + configValue: "disabled", + want: Enabled, + }, + { + name: "env disabled takes precedence over config enabled", + lookupEnv: envSet("disabled"), + configValue: "enabled", + want: Disabled, + }, + { + name: "env log takes precedence over config enabled", + lookupEnv: envSet("log"), + configValue: "enabled", + want: Logged, + }, + { + name: "env false disables", + lookupEnv: envSet("false"), + configValue: "enabled", + want: Disabled, + }, + { + name: "env empty string disables", + lookupEnv: envSet(""), + configValue: "enabled", + want: Disabled, + }, + { + name: "env any truthy value enables", + lookupEnv: envSet("yes"), + configValue: "disabled", + want: Enabled, + }, + { + name: "env FALSE (uppercase) disables", + lookupEnv: envSet("FALSE"), + configValue: "enabled", + want: Disabled, + }, + { + name: "env LOG (uppercase) logs", + lookupEnv: envSet("LOG"), + configValue: "enabled", + want: Logged, + }, + { + name: "env value with whitespace is trimmed", + lookupEnv: envSet(" false "), + configValue: "enabled", + want: Disabled, + }, + { + name: "DO_NOT_TRACK=1 disables telemetry", + lookupEnv: envMap(map[string]string{"DO_NOT_TRACK": "1"}), + configValue: "enabled", + want: Disabled, + }, + { + name: "DO_NOT_TRACK=true disables telemetry", + lookupEnv: envMap(map[string]string{"DO_NOT_TRACK": "true"}), + configValue: "enabled", + want: Disabled, + }, + { + name: "DO_NOT_TRACK=TRUE disables telemetry (case insensitive)", + lookupEnv: envMap(map[string]string{"DO_NOT_TRACK": "TRUE"}), + configValue: "enabled", + want: Disabled, + }, + { + name: "DO_NOT_TRACK=0 does not disable telemetry", + lookupEnv: envMap(map[string]string{"DO_NOT_TRACK": "0"}), + configValue: "enabled", + want: Enabled, + }, + { + name: "DO_NOT_TRACK with whitespace is trimmed", + lookupEnv: envMap(map[string]string{"DO_NOT_TRACK": " 1 "}), + configValue: "enabled", + want: Disabled, + }, + { + name: "GH_TELEMETRY takes precedence over DO_NOT_TRACK", + lookupEnv: envMap(map[string]string{"GH_TELEMETRY": "enabled", "DO_NOT_TRACK": "1"}), + configValue: "", + want: Enabled, + }, + { + name: "DO_NOT_TRACK takes precedence over config", + lookupEnv: envMap(map[string]string{"DO_NOT_TRACK": "1"}), + configValue: "log", + want: Disabled, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Cleanup(stubLookupEnv(tt.lookupEnv)) + got := ParseTelemetryState(tt.configValue) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestNewServiceLogModeFlushesToWriter(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var buf bytes.Buffer + svc := NewService(LogFlusher(&buf, false)) + + svc.Record(ghtelemetry.Event{ + Type: "test_event", + Dimensions: map[string]string{"key": "value"}, + }) + svc.Flush() + + output := buf.String() + assert.Contains(t, output, "Telemetry payload:") + assert.Contains(t, output, "test_event") + assert.Contains(t, output, `"key"`) + assert.Contains(t, output, `"value"`) +} + +func TestNewServiceLogModeWithColorLogsToWriter(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var buf bytes.Buffer + svc := NewService(LogFlusher(&buf, true)) + + svc.Record(ghtelemetry.Event{Type: "color_event"}) + svc.Flush() + + output := buf.String() + assert.Contains(t, output, "color_event") + // Verify ANSI color codes are present in the output + assert.Contains(t, output, "\033[", "expected ANSI escape sequences when color is enabled") +} + +func TestServiceDeviceIDFallback(t *testing.T) { + t.Cleanup(stubDeviceIDError(errors.New("no device id"))) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + require.Len(t, captured.Events, 1) + assert.Equal(t, "", captured.Events[0].Dimensions["device_id"]) +} + +func TestServiceFlush(t *testing.T) { + t.Run("does nothing when no events recorded", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + called := false + svc := newService(func(SendTelemetryPayload) { called = true }, nil) + svc.Flush() + + assert.False(t, called, "flusher should not be called with no events") + }) + + t.Run("flushes events with merged dimensions", 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: "command_invocation", + Dimensions: map[string]string{"command": "gh pr list"}, + Measures: map[string]int64{"duration_ms": 150}, + }) + svc.Flush() + + require.Len(t, captured.Events, 1) + event := captured.Events[0] + assert.Equal(t, "command_invocation", event.Type) + assert.Equal(t, "gh pr list", event.Dimensions["command"]) + assert.Equal(t, "2.45.0", event.Dimensions["version"]) + assert.Equal(t, "test-device", event.Dimensions["device_id"]) + assert.NotEmpty(t, event.Dimensions["timestamp"]) + assert.NotEmpty(t, event.Dimensions["invocation_id"]) + assert.Equal(t, int64(150), event.Measures["duration_ms"]) + }) + + t.Run("flushes multiple events", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) + + svc.Record(ghtelemetry.Event{Type: "event1"}) + svc.Record(ghtelemetry.Event{Type: "event2"}) + svc.Flush() + + require.Len(t, captured.Events, 2) + assert.Equal(t, "event1", captured.Events[0].Type) + assert.Equal(t, "event2", captured.Events[1].Type) + }) + + t.Run("is idempotent", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + callCount := 0 + svc := newService(func(SendTelemetryPayload) { callCount++ }, nil) + svc.Record(ghtelemetry.Event{Type: "test"}) + + svc.Flush() + svc.Flush() + svc.Flush() + + assert.Equal(t, 1, callCount, "flusher should only be called once") + }) + + t.Run("event dimensions override common dimensions", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, ghtelemetry.Dimensions{"shared": "common"}) + + svc.Record(ghtelemetry.Event{ + Type: "test", + Dimensions: map[string]string{"shared": "event-level"}, + }) + svc.Flush() + + require.Len(t, captured.Events, 1) + // Event dimensions are copied last via maps.Copy, so they override common + assert.Equal(t, "event-level", captured.Events[0].Dimensions["shared"]) + }) + + t.Run("timestamps reflect record time not flush time", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) + + svc.Record(ghtelemetry.Event{Type: "early"}) + time.Sleep(50 * time.Millisecond) + svc.Record(ghtelemetry.Event{Type: "late"}) + svc.Flush() + + require.Len(t, captured.Events, 2) + ts1 := captured.Events[0].Dimensions["timestamp"] + ts2 := captured.Events[1].Dimensions["timestamp"] + require.NotEmpty(t, ts1) + require.NotEmpty(t, ts2) + + t1, err := time.Parse("2006-01-02T15:04:05.000Z", ts1) + require.NoError(t, err) + t2, err := time.Parse("2006-01-02T15:04:05.000Z", ts2) + require.NoError(t, err) + + assert.True(t, t2.After(t1), "second event timestamp %s should be after first %s", ts2, ts1) + }) +} + +func TestServiceSampling(t *testing.T) { + t.Run("sampleRate 0 sends all events", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) + svc.sampleRate = 0 + svc.sampleBucket = 99 + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + require.Len(t, captured.Events, 1) + }) + + t.Run("sampleRate 100 sends all events regardless of bucket", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) + svc.sampleRate = 100 + svc.sampleBucket = 99 + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + require.Len(t, captured.Events, 1) + }) + + t.Run("bucket below sampleRate sends events", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) + svc.sampleRate = 50 + svc.sampleBucket = 49 // below rate, should be included + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + require.Len(t, captured.Events, 1) + }) + + t.Run("bucket at sampleRate drops events", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + called := false + svc := newService(func(SendTelemetryPayload) { called = true }, nil) + svc.sampleRate = 50 + svc.sampleBucket = 50 // at rate boundary, should be excluded + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + assert.False(t, called, "flusher should not be called when bucket >= sampleRate") + }) + + t.Run("bucket above sampleRate drops events", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + called := false + svc := newService(func(SendTelemetryPayload) { called = true }, nil) + svc.sampleRate = 1 + svc.sampleBucket = 50 + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + assert.False(t, called, "flusher should not be called when bucket >= sampleRate") + }) + + t.Run("SetSampleRate changes flush behavior", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + called := false + svc := newService(func(SendTelemetryPayload) { called = true }, nil) + svc.sampleBucket = 50 + + // Initially rate=0, which sends everything + svc.SetSampleRate(10) // Now bucket=50 >= rate=10, should drop + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + assert.False(t, called, "flusher should not be called after SetSampleRate reduced the 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) { + t.Cleanup(stubDeviceID("test-device")) + + var captured SendTelemetryPayload + svc := NewService( + func(p SendTelemetryPayload) { captured = p }, + WithAdditionalCommonDimensions(ghtelemetry.Dimensions{ + "version": "2.45.0", + "agent": "none", + }), + ) + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + require.Len(t, captured.Events, 1) + assert.Equal(t, "2.45.0", captured.Events[0].Dimensions["version"]) + assert.Equal(t, "none", captured.Events[0].Dimensions["agent"]) + // Standard common dimensions should also be present + assert.Equal(t, "test-device", captured.Events[0].Dimensions["device_id"]) + assert.NotEmpty(t, captured.Events[0].Dimensions["invocation_id"]) + assert.NotEmpty(t, captured.Events[0].Dimensions["os"]) + assert.NotEmpty(t, captured.Events[0].Dimensions["architecture"]) +} + +func TestNoOpService(t *testing.T) { + svc := &NoOpService{} + // All methods should be safe to call without panicking + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.SetSampleRate(50) + svc.Flush() +} + +func TestSpawnSendTelemetryRejectsOversizedPayload(t *testing.T) { + // Build a payload larger than maxPayloadSize (16KB) + largeDimensions := map[string]string{ + "data": strings.Repeat("x", maxPayloadSize), + } + payload := SendTelemetryPayload{ + Events: []PayloadEvent{ + {Type: "test", Dimensions: largeDimensions}, + }, + } + + // This should not panic or spawn a process - it silently returns. + // We can't easily assert the subprocess wasn't started, but we verify + // the function doesn't crash. + SpawnSendTelemetry("/nonexistent/binary", payload) +} diff --git a/pkg/cmd/auth/auth.go b/pkg/cmd/auth/auth.go index 70e01653f14..e8154f42495 100644 --- a/pkg/cmd/auth/auth.go +++ b/pkg/cmd/auth/auth.go @@ -31,5 +31,7 @@ func NewCmdAuth(f *cmdutil.Factory) *cobra.Command { cmd.AddCommand(authTokenCmd.NewCmdToken(f, nil)) cmd.AddCommand(authSwitchCmd.NewCmdSwitch(f, nil)) + cmdutil.DisableTelemetryForSubcommands(cmd) + return cmd } diff --git a/pkg/cmd/completion/completion.go b/pkg/cmd/completion/completion.go index b34bf7abfb5..b703fa24952 100644 --- a/pkg/cmd/completion/completion.go +++ b/pkg/cmd/completion/completion.go @@ -93,6 +93,7 @@ func NewCmdCompletion(io *iostreams.IOStreams) *cobra.Command { cmdutil.DisableAuthCheck(cmd) cmdutil.StringEnumFlag(cmd, &shellType, "shell", "s", "", []string{"bash", "zsh", "fish", "powershell"}, "Shell type") + cmdutil.DisableTelemetry(cmd) return cmd } diff --git a/pkg/cmd/config/list/list_test.go b/pkg/cmd/config/list/list_test.go index 27260e85783..61d3db35981 100644 --- a/pkg/cmd/config/list/list_test.go +++ b/pkg/cmd/config/list/list_test.go @@ -104,6 +104,7 @@ func Test_listRun(t *testing.T) { accessible_colors=disabled accessible_prompter=disabled spinner=enabled + telemetry=enabled `), }, } diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index 7e2d7aca75f..45a60332b8d 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -8,6 +8,7 @@ import ( "time" "github.com/cli/cli/v2/internal/update" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/utils" @@ -26,7 +27,7 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex checkExtensionReleaseInfo = checkForExtensionUpdate } - return &cobra.Command{ + cmd := &cobra.Command{ Use: ext.Name(), Short: fmt.Sprintf("Extension %s", ext.Name()), // PreRun handles looking up whether extension has a latest version only when the command is ran. @@ -73,12 +74,14 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex // This is being handled in non-blocking default as there is no context to cancel like in gh update checks. } }, - GroupID: "extension", - Annotations: map[string]string{ - "skipAuthCheck": "true", - }, + GroupID: "extension", DisableFlagParsing: true, } + + cmdutil.DisableAuthCheck(cmd) + cmdutil.DisableTelemetry(cmd) + + return cmd } func checkForExtensionUpdate(em extensions.ExtensionManager, ext extensions.Extension) (*update.ReleaseInfo, error) { diff --git a/pkg/cmd/root/extension_registration_test.go b/pkg/cmd/root/extension_registration_test.go index 90b836e4a47..828f51ca066 100644 --- a/pkg/cmd/root/extension_registration_test.go +++ b/pkg/cmd/root/extension_registration_test.go @@ -6,6 +6,7 @@ import ( "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" @@ -74,7 +75,7 @@ func TestNewCmdRoot_ExtensionRegistration(t *testing.T) { ExtensionManager: em, } - cmd, err := NewCmdRoot(f, "", "") + cmd, err := NewCmdRoot(f, &telemetry.NoOpService{}, "", "") require.NoError(t, err) // Verify skipped extensions (should find core command registered, not extension) diff --git a/pkg/cmd/root/help_test.go b/pkg/cmd/root/help_test.go index 40f333159de..e7f04375845 100644 --- a/pkg/cmd/root/help_test.go +++ b/pkg/cmd/root/help_test.go @@ -7,6 +7,7 @@ import ( "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/iostreams" @@ -74,7 +75,7 @@ func TestKramdownCompatibleDocs(t *testing.T) { }, } - cmd, err := NewCmdRoot(f, "N/A", "") + cmd, err := NewCmdRoot(f, &telemetry.NoOpService{}, "N/A", "") require.NoError(t, err) var walk func(*cobra.Command) diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index a375d9e2050..fbacef356cc 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -117,6 +117,12 @@ var HelpTopics = []helpTopic{ %[1]sGH_ACCESSIBLE_PROMPTER%[1]s (preview): set to a truthy value to enable prompts that are more compatible with speech synthesis and braille screen readers. + %[1]sGH_TELEMETRY%[1]s: set to %[1]slog%[1]s to print telemetry data to standard error instead of sending it. + Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry that would have been printed when set to %[1]slog%[1]s. + + %[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry that would have been printed + when %[1]sGH_TELEMETRY%[1]s is set to %[1]slog%[1]s. %[1]sGH_TELEMETRY%[1]s takes precedence if both are set. + %[1]sGH_SPINNER_DISABLED%[1]s: set to a truthy value to replace the spinner animation with a textual progress indicator. `, "`"), diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 37684b40c8c..7df9d2986ea 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" accessibilityCmd "github.com/cli/cli/v2/pkg/cmd/accessibility" actionsCmd "github.com/cli/cli/v2/pkg/cmd/actions" agentTaskCmd "github.com/cli/cli/v2/pkg/cmd/agent-task" @@ -38,6 +39,7 @@ import ( runCmd "github.com/cli/cli/v2/pkg/cmd/run" searchCmd "github.com/cli/cli/v2/pkg/cmd/search" secretCmd "github.com/cli/cli/v2/pkg/cmd/secret" + sendTelemetryCmd "github.com/cli/cli/v2/pkg/cmd/send-telemetry" skillsCmd "github.com/cli/cli/v2/pkg/cmd/skills" sshKeyCmd "github.com/cli/cli/v2/pkg/cmd/ssh-key" statusCmd "github.com/cli/cli/v2/pkg/cmd/status" @@ -58,7 +60,7 @@ func (ae *AuthError) Error() string { return ae.err.Error() } -func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, error) { +func NewCmdRoot(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, version, buildDate string) (*cobra.Command, error) { io := f.IOStreams cfg, err := f.Config() if err != nil { @@ -88,6 +90,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, } return &AuthError{} } + return nil }, } @@ -153,6 +156,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, cmd.AddCommand(statusCmd.NewCmdStatus(f, nil)) cmd.AddCommand(creditsCmd.NewCmdCredits(f, nil)) cmd.AddCommand(licensesCmd.NewCmdLicenses(f)) + cmd.AddCommand(sendTelemetryCmd.NewCmdSendTelemetry(f)) // below here at the commands that require the "intelligent" BaseRepo resolver repoResolvingCmdFactory := *f @@ -244,6 +248,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) (*cobra.Command, } cmdutil.DisableAuthCheck(cmd) + cmdutil.RecordTelemetryForSubcommands(cmd, telemetry) // The reference command produces paged output that displays information on every other command. // Therefore, we explicitly set the Long text and HelpFunc here after all other commands are registered. diff --git a/pkg/cmd/send-telemetry/send_telemetry.go b/pkg/cmd/send-telemetry/send_telemetry.go new file mode 100644 index 00000000000..fce6dff8391 --- /dev/null +++ b/pkg/cmd/send-telemetry/send_telemetry.go @@ -0,0 +1,135 @@ +package sendtelemetry + +import ( + "cmp" + "context" + "encoding/json" + "fmt" + "io" + "net" + "net/http" + "os" + "time" + + "github.com/cli/cli/v2/internal/barista/observability" + "github.com/cli/cli/v2/internal/build" + "github.com/cli/cli/v2/internal/telemetry" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/spf13/cobra" +) + +const defaultTelemetryEndpointURL = "https://cafe.github.com" + +type SendTelemetryOptions struct { + TelemetryEndpointURL string + PayloadJSON string + HTTPUnixSocket string +} + +func NewCmdSendTelemetry(f *cmdutil.Factory) *cobra.Command { + return newCmdSendTelemetry(f, nil) +} + +func newCmdSendTelemetry(f *cmdutil.Factory, runF func(*SendTelemetryOptions) error) *cobra.Command { + cmd := &cobra.Command{ + Use: "send-telemetry", + Short: "Send telemetry event to GitHub", + Hidden: true, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + cfg, err := f.Config() + if err != nil { + return err + } + + payloadJSON, err := io.ReadAll(cmd.InOrStdin()) + if err != nil { + return fmt.Errorf("reading payload from stdin: %w", err) + } + if len(payloadJSON) == 0 { + return fmt.Errorf("no payload provided on stdin") + } + + opts := &SendTelemetryOptions{ + TelemetryEndpointURL: cmp.Or(os.Getenv("GH_TELEMETRY_ENDPOINT_URL"), defaultTelemetryEndpointURL), + PayloadJSON: string(payloadJSON), + // This is a best effort to use a Unix Socket if configured. In most cases, if there is one configured + // it will be at the global level. However, since the telemetry service is not related to a specific host, we can't + // know that the socket we choose will work. + HTTPUnixSocket: cfg.HTTPUnixSocket("").Value, + } + + if runF != nil { + return runF(opts) + } + + return runSendTelemetry(cmd.Context(), opts) + }, + } + + cmdutil.DisableAuthCheck(cmd) + cmdutil.DisableTelemetry(cmd) + + return cmd +} + +func runSendTelemetry(ctx context.Context, opts *SendTelemetryOptions) error { + httpClient := &http.Client{ + Timeout: 2 * time.Second, + Transport: &userAgentTransport{ + base: handleUnixDomainSocket(opts.HTTPUnixSocket), + userAgent: fmt.Sprintf("GitHub CLI %s", build.Version), + }, + } + + client := observability.NewTelemetryAPIProtobufClient(opts.TelemetryEndpointURL, httpClient) + + var payload telemetry.SendTelemetryPayload + if err := json.Unmarshal([]byte(opts.PayloadJSON), &payload); err != nil { + return fmt.Errorf("parsing payload JSON: %w", err) + } + + if len(payload.Events) == 0 { + return nil + } + + events := make([]*observability.TelemetryEvent, len(payload.Events)) + for i, event := range payload.Events { + events[i] = &observability.TelemetryEvent{ + App: "github-cli", + EventType: event.Type, + Dimensions: event.Dimensions, + Measures: event.Measures, + } + } + + _, err := client.RecordEvents(ctx, &observability.RecordEventsRequest{ + Events: events, + }) + return err +} + +type userAgentTransport struct { + base http.RoundTripper + userAgent string +} + +func (t *userAgentTransport) RoundTrip(req *http.Request) (*http.Response, error) { + req.Header.Set("User-Agent", t.userAgent) + return t.base.RoundTrip(req) +} + +func handleUnixDomainSocket(socketPath string) http.RoundTripper { + if socketPath == "" { + return http.DefaultTransport + } + + dialContext := func(ctx context.Context, network, addr string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, "unix", socketPath) + } + + return &http.Transport{ + DialContext: dialContext, + DisableKeepAlives: true, + } +} diff --git a/pkg/cmd/send-telemetry/send_telemetry_test.go b/pkg/cmd/send-telemetry/send_telemetry_test.go new file mode 100644 index 00000000000..8ec2f83c555 --- /dev/null +++ b/pkg/cmd/send-telemetry/send_telemetry_test.go @@ -0,0 +1,226 @@ +package sendtelemetry + +import ( + "context" + "encoding/json" + "io" + "net/http/httptest" + "strings" + "testing" + + "github.com/cli/cli/v2/internal/barista/observability" + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/telemetry" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type mockTelemetryAPI struct { + request *observability.RecordEventsRequest + err error +} + +func (m *mockTelemetryAPI) RecordEvents(_ context.Context, req *observability.RecordEventsRequest) (*observability.RecordEventsResponse, error) { + m.request = req + return &observability.RecordEventsResponse{}, m.err +} + +func TestNewCmdSendTelemetry(t *testing.T) { + tests := []struct { + name string + stdin string + env map[string]string + wantOpts SendTelemetryOptions + wantErr string + }{ + { + name: "reads payload from stdin", + stdin: `{"events":[{"type":"usage","dimensions":{"command":"gh pr list"}}]}`, + wantOpts: SendTelemetryOptions{ + TelemetryEndpointURL: defaultTelemetryEndpointURL, + PayloadJSON: `{"events":[{"type":"usage","dimensions":{"command":"gh pr list"}}]}`, + }, + }, + { + name: "uses GH_TELEMETRY_ENDPOINT_URL env var", + stdin: `{"events":[]}`, + env: map[string]string{"GH_TELEMETRY_ENDPOINT_URL": "https://custom.endpoint"}, + wantOpts: SendTelemetryOptions{ + TelemetryEndpointURL: "https://custom.endpoint", + PayloadJSON: `{"events":[]}`, + }, + }, + { + name: "defaults endpoint when env var not set", + stdin: `{}`, + wantOpts: SendTelemetryOptions{ + TelemetryEndpointURL: defaultTelemetryEndpointURL, + PayloadJSON: `{}`, + }, + }, + { + name: "errors on empty stdin", + stdin: "", + wantErr: "no payload provided on stdin", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for k, v := range tt.env { + t.Setenv(k, v) + } + + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + Config: func() (gh.Config, error) { + return config.NewBlankConfig(), nil + }, + } + + var gotOpts *SendTelemetryOptions + cmd := newCmdSendTelemetry(f, func(opts *SendTelemetryOptions) error { + gotOpts = opts + return nil + }) + cmd.SetArgs([]string{}) + cmd.SetIn(strings.NewReader(tt.stdin)) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + _, err := cmd.ExecuteC() + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + require.NotNil(t, gotOpts) + assert.Equal(t, tt.wantOpts.TelemetryEndpointURL, gotOpts.TelemetryEndpointURL) + assert.Equal(t, tt.wantOpts.PayloadJSON, gotOpts.PayloadJSON) + }) + } +} + +func TestRunSendTelemetry(t *testing.T) { + tests := []struct { + name string + payload telemetry.SendTelemetryPayload + serverErr error + wantErr bool + assertFunc func(t *testing.T, req *observability.RecordEventsRequest) + }{ + { + name: "posts single event to endpoint", + payload: telemetry.SendTelemetryPayload{ + Events: []telemetry.PayloadEvent{ + { + Type: "command_invocation", + Dimensions: map[string]string{ + "command": "gh pr create", + "device_id": "abc123", + "os": "darwin", + }, + Measures: map[string]int64{"duration_ms": 150}, + }, + }, + }, + assertFunc: func(t *testing.T, req *observability.RecordEventsRequest) { + t.Helper() + require.Len(t, req.Events, 1) + event := req.Events[0] + assert.Equal(t, "github-cli", event.App) + assert.Equal(t, "command_invocation", event.EventType) + assert.Equal(t, "gh pr create", event.Dimensions["command"]) + assert.Equal(t, "abc123", event.Dimensions["device_id"]) + assert.Equal(t, "darwin", event.Dimensions["os"]) + }, + }, + { + name: "posts multiple events in single batch request", + payload: telemetry.SendTelemetryPayload{ + Events: []telemetry.PayloadEvent{ + {Type: "event1", Dimensions: map[string]string{"a": "1"}}, + {Type: "event2", Dimensions: map[string]string{"b": "2"}}, + }, + }, + assertFunc: func(t *testing.T, req *observability.RecordEventsRequest) { + t.Helper() + require.Len(t, req.Events, 2) + assert.Equal(t, "1", req.Events[0].Dimensions["a"]) + assert.Equal(t, "2", req.Events[1].Dimensions["b"]) + assert.Equal(t, "github-cli", req.Events[0].App) + assert.Equal(t, "event1", req.Events[0].EventType) + assert.Equal(t, "github-cli", req.Events[1].App) + assert.Equal(t, "event2", req.Events[1].EventType) + }, + }, + { + name: "empty events list produces no request", + payload: telemetry.SendTelemetryPayload{ + Events: []telemetry.PayloadEvent{}, + }, + assertFunc: func(t *testing.T, req *observability.RecordEventsRequest) { + t.Helper() + assert.Nil(t, req) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockTelemetryAPI{err: tt.serverErr} + handler := observability.NewTelemetryAPIServer(mock) + server := httptest.NewServer(handler) + defer server.Close() + + opts := &SendTelemetryOptions{ + TelemetryEndpointURL: server.URL, + PayloadJSON: mustMarshal(t, tt.payload), + } + + err := runSendTelemetry(context.Background(), opts) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + + if tt.assertFunc != nil { + tt.assertFunc(t, mock.request) + } + }) + } +} + +func TestRunSendTelemetryInvalidPayload(t *testing.T) { + err := runSendTelemetry(context.Background(), &SendTelemetryOptions{ + TelemetryEndpointURL: "http://localhost:0", + PayloadJSON: "not-json", + }) + require.Error(t, err) +} + +func TestRunSendTelemetryServerError(t *testing.T) { + mock := &mockTelemetryAPI{err: assert.AnError} + handler := observability.NewTelemetryAPIServer(mock) + server := httptest.NewServer(handler) + defer server.Close() + + err := runSendTelemetry(context.Background(), &SendTelemetryOptions{ + TelemetryEndpointURL: server.URL, + PayloadJSON: `{"events":[{"type":"test","dimensions":{"a":"1"}}]}`, + }) + require.Error(t, err) +} + +func mustMarshal(t *testing.T, v any) string { + t.Helper() + data, err := json.Marshal(v) + require.NoError(t, err) + return string(data) +} diff --git a/pkg/cmd/version/version.go b/pkg/cmd/version/version.go index 11d4a0271fd..4f68fbe61be 100644 --- a/pkg/cmd/version/version.go +++ b/pkg/cmd/version/version.go @@ -13,8 +13,9 @@ func NewCmdVersion(f *cmdutil.Factory, version, buildDate string) *cobra.Command cmd := &cobra.Command{ Use: "version", Hidden: true, - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { fmt.Fprint(f.IOStreams.Out, cmd.Root().Annotations["versionInfo"]) + return nil }, } diff --git a/pkg/cmdutil/telemetry.go b/pkg/cmdutil/telemetry.go new file mode 100644 index 00000000000..42169beecec --- /dev/null +++ b/pkg/cmdutil/telemetry.go @@ -0,0 +1,66 @@ +package cmdutil + +import ( + "slices" + "strings" + + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/spf13/cobra" + "github.com/spf13/pflag" +) + +func RecordTelemetry(cmd *cobra.Command, telemetry ghtelemetry.EventRecorder) { + if isTelemetryDisabled(cmd) { + return + } + + if cmd.RunE == nil { + return + } + + currentRunE := cmd.RunE + cmd.RunE = func(cmd *cobra.Command, args []string) error { + runErr := currentRunE(cmd, args) + + var flags []string + cmd.Flags().Visit(func(f *pflag.Flag) { + flags = append(flags, f.Name) + }) + slices.Sort(flags) + + telemetry.Record(ghtelemetry.Event{ + Type: "command_invocation", + Dimensions: map[string]string{ + "command": cmd.CommandPath(), + "flags": strings.Join(flags, ","), + }, + }) + + return runErr + } +} + +func RecordTelemetryForSubcommands(cmd *cobra.Command, telemetry ghtelemetry.EventRecorder) { + for _, c := range cmd.Commands() { + RecordTelemetry(c, telemetry) + RecordTelemetryForSubcommands(c, telemetry) + } +} + +func DisableTelemetry(cmd *cobra.Command) { + if cmd.Annotations == nil { + cmd.Annotations = map[string]string{} + } + cmd.Annotations["telemetry"] = "disabled" +} + +func DisableTelemetryForSubcommands(cmd *cobra.Command) { + for _, c := range cmd.Commands() { + DisableTelemetry(c) + DisableTelemetryForSubcommands(c) + } +} + +func isTelemetryDisabled(cmd *cobra.Command) bool { + return cmd.Annotations["telemetry"] == "disabled" +} diff --git a/pkg/cmdutil/telemetry_test.go b/pkg/cmdutil/telemetry_test.go new file mode 100644 index 00000000000..bfe4c420ca0 --- /dev/null +++ b/pkg/cmdutil/telemetry_test.go @@ -0,0 +1,168 @@ +package cmdutil_test + +import ( + "fmt" + "testing" + + "github.com/cli/cli/v2/internal/telemetry" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRecordTelemetry(t *testing.T) { + t.Run("records command path and flags", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + cmd := &cobra.Command{ + Use: "list", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmd.Flags().Bool("web", false, "") + cmd.Flags().String("repo", "", "") + + parent := &cobra.Command{Use: "pr"} + root := &cobra.Command{Use: "gh"} + root.AddCommand(parent) + parent.AddCommand(cmd) + + cmdutil.RecordTelemetry(cmd, recorder) + + require.NoError(t, cmd.Flags().Set("web", "true")) + require.NoError(t, cmd.Flags().Set("repo", "cli/cli")) + require.NoError(t, cmd.RunE(cmd, nil)) + + require.Len(t, recorder.Events, 1) + event := recorder.Events[0] + assert.Equal(t, "command_invocation", event.Type) + assert.Equal(t, "gh pr list", event.Dimensions["command"]) + assert.Equal(t, "repo,web", event.Dimensions["flags"]) + }) + + t.Run("is a no-op when original RunE is nil", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + cmd := &cobra.Command{Use: "test"} + + cmdutil.RecordTelemetry(cmd, recorder) + + assert.Nil(t, cmd.RunE, "RunE should remain nil when it was nil before") + assert.Empty(t, recorder.Events, "no telemetry should be recorded") + }) + + t.Run("propagates error from original RunE", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + expectedErr := fmt.Errorf("something went wrong") + cmd := &cobra.Command{ + Use: "fail", + RunE: func(cmd *cobra.Command, args []string) error { return expectedErr }, + } + + cmdutil.RecordTelemetry(cmd, recorder) + + err := cmd.RunE(cmd, nil) + assert.ErrorIs(t, err, expectedErr) + // Telemetry is still recorded even on error + require.Len(t, recorder.Events, 1) + assert.Equal(t, "command_invocation", recorder.Events[0].Type) + }) + + t.Run("flags are sorted alphabetically", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + cmd := &cobra.Command{ + Use: "test", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmd.Flags().Bool("zebra", false, "") + cmd.Flags().Bool("alpha", false, "") + cmd.Flags().Bool("middle", false, "") + + cmdutil.RecordTelemetry(cmd, recorder) + + require.NoError(t, cmd.Flags().Set("zebra", "true")) + require.NoError(t, cmd.Flags().Set("alpha", "true")) + require.NoError(t, cmd.Flags().Set("middle", "true")) + require.NoError(t, cmd.RunE(cmd, nil)) + + require.Len(t, recorder.Events, 1) + assert.Equal(t, "alpha,middle,zebra", recorder.Events[0].Dimensions["flags"]) + }) + + t.Run("no flags set records empty flags string", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + cmd := &cobra.Command{ + Use: "test", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmd.Flags().Bool("unused", false, "") + + cmdutil.RecordTelemetry(cmd, recorder) + require.NoError(t, cmd.RunE(cmd, nil)) + + require.Len(t, recorder.Events, 1) + assert.Equal(t, "", recorder.Events[0].Dimensions["flags"]) + }) + + t.Run("skips commands with telemetry disabled", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + cmd := &cobra.Command{ + Use: "internal", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmdutil.DisableTelemetry(cmd) + cmdutil.RecordTelemetry(cmd, recorder) + + require.NoError(t, cmd.RunE(cmd, nil)) + assert.Empty(t, recorder.Events, "telemetry should not be recorded for disabled commands") + }) +} + +func TestRecordTelemetryForSubcommands(t *testing.T) { + t.Run("instruments nested subcommands", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + + root := &cobra.Command{Use: "gh"} + parent := &cobra.Command{Use: "pr"} + child := &cobra.Command{ + Use: "list", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(parent) + parent.AddCommand(child) + + cmdutil.RecordTelemetryForSubcommands(root, recorder) + require.NoError(t, child.RunE(child, nil)) + + require.Len(t, recorder.Events, 1) + assert.Equal(t, "command_invocation", recorder.Events[0].Type) + assert.Equal(t, "gh pr list", recorder.Events[0].Dimensions["command"]) + }) + + t.Run("skips subcommands with nil RunE", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + + root := &cobra.Command{Use: "gh"} + child := &cobra.Command{Use: "help"} // no RunE + root.AddCommand(child) + + cmdutil.RecordTelemetryForSubcommands(root, recorder) + + assert.Nil(t, child.RunE, "nil RunE should remain nil") + }) + + t.Run("skips subcommands with telemetry disabled", func(t *testing.T) { + recorder := &telemetry.EventRecorderSpy{} + + root := &cobra.Command{Use: "gh"} + child := &cobra.Command{ + Use: "send-telemetry", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmdutil.DisableTelemetry(child) + root.AddCommand(child) + + cmdutil.RecordTelemetryForSubcommands(root, recorder) + require.NoError(t, child.RunE(child, nil)) + + assert.Empty(t, recorder.Events, "disabled commands should not record telemetry") + }) +} From 3ed389d664d9820f73bd02c3d77575f43299cd62 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 15 Apr 2026 14:55:35 +0200 Subject: [PATCH 2/4] Disable telemetry for GHES --- .../no-telemetry-for-ghes-user.txtar | 8 + api/http_client.go | 21 + api/http_client_test.go | 74 +++ internal/gh/ghtelemetry/telemetry.go | 5 + internal/ghcmd/cmd.go | 174 ++++++- internal/ghcmd/cmd_test.go | 435 ++++++++++++++++++ .../ghcmd/executable_test.go | 11 +- internal/telemetry/fake.go | 2 + internal/telemetry/telemetry.go | 15 + internal/telemetry/telemetry_test.go | 44 ++ pkg/cmd/api/api.go | 2 +- .../verify/verify_integration_test.go | 78 ++-- pkg/cmd/auth/login/login.go | 2 +- pkg/cmd/auth/refresh/refresh.go | 2 +- pkg/cmd/auth/setupgit/setupgit.go | 2 +- pkg/cmd/codespace/root.go | 10 +- pkg/cmd/factory/default.go | 149 ++---- pkg/cmd/factory/default_test.go | 403 +--------------- pkg/cmd/search/shared/shared_test.go | 13 +- pkg/cmd/skills/preview/preview.go | 18 +- pkg/cmd/skills/search/search.go | 24 +- pkg/cmdutil/factory.go | 78 +--- pkg/cmdutil/repo_override.go | 6 +- 23 files changed, 920 insertions(+), 656 deletions(-) create mode 100644 acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar rename pkg/cmdutil/factory_test.go => internal/ghcmd/executable_test.go (94%) diff --git a/acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar b/acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar new file mode 100644 index 00000000000..0fe6f4bb210 --- /dev/null +++ b/acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar @@ -0,0 +1,8 @@ +# GHES users should not get telemetry even when telemetry is enabled +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 +env GH_ENTERPRISE_TOKEN=fake-enterprise-token + +exec gh version +! stderr 'Telemetry payload:' diff --git a/api/http_client.go b/api/http_client.go index 532f79c7f9d..be7a6b8a71c 100644 --- a/api/http_client.go +++ b/api/http_client.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/utils" ghAPI "github.com/cli/go-gh/v2/pkg/api" ghauth "github.com/cli/go-gh/v2/pkg/auth" @@ -26,6 +27,7 @@ type HTTPClientOptions struct { LogColorize bool LogVerboseHTTP bool SkipDefaultHeaders bool + TelemetryDisabler ghtelemetry.Disabler } func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) { @@ -74,6 +76,13 @@ func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) { client.Transport = AddAuthTokenHeader(client.Transport, opts.Config) } + if opts.TelemetryDisabler != nil { + client.Transport = telemetryDisablerTransport{ + wrappedTransport: client.Transport, + telemetryDisabler: opts.TelemetryDisabler, + } + } + return client, nil } @@ -147,3 +156,15 @@ func getHost(r *http.Request) string { } return r.URL.Host } + +type telemetryDisablerTransport struct { + wrappedTransport http.RoundTripper + telemetryDisabler ghtelemetry.Disabler +} + +func (t telemetryDisablerTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if ghauth.IsEnterprise(getHost(req)) { + t.telemetryDisabler.Disable() + } + return t.wrappedTransport.RoundTrip(req) +} diff --git a/api/http_client_test.go b/api/http_client_test.go index 1c81b4aa7a5..198c0849118 100644 --- a/api/http_client_test.go +++ b/api/http_client_test.go @@ -315,6 +315,80 @@ func TestHTTPClientSanitizeControlCharactersC1(t *testing.T) { assert.Equal(t, "monalisa¡", issue.Author.Login) } +func TestNewHTTPClientTelemetryDisabler(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + defer ts.Close() + + tests := []struct { + name string + host string + wantDisabled bool + }{ + { + name: "enterprise host triggers disable", + host: "ghes.example.com", + wantDisabled: true, + }, + { + name: "github.com does not trigger disable", + host: "github.com", + wantDisabled: false, + }, + { + name: "tenancy host does not trigger disable", + host: "my-company.ghe.com", + wantDisabled: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + disabler := &fakeTelemetryDisabler{} + client, err := NewHTTPClient(HTTPClientOptions{ + TelemetryDisabler: disabler, + }) + require.NoError(t, err) + + req, err := http.NewRequest("GET", ts.URL, nil) + require.NoError(t, err) + req.Host = tt.host + + res, err := client.Do(req) + require.NoError(t, err) + assert.Equal(t, 204, res.StatusCode) + assert.Equal(t, tt.wantDisabled, disabler.disabled, "Disable() called") + }) + } +} + +func TestNewHTTPClientWithoutTelemetryDisabler(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{}) + require.NoError(t, err) + + req, err := http.NewRequest("GET", ts.URL, nil) + require.NoError(t, err) + req.Host = "ghes.example.com" + + res, err := client.Do(req) + require.NoError(t, err) + assert.Equal(t, 204, res.StatusCode) +} + +type fakeTelemetryDisabler struct { + disabled bool +} + +func (f *fakeTelemetryDisabler) Disable() { + f.disabled = true +} + type tinyConfig map[string]string func (c tinyConfig) ActiveToken(host string) (string, string) { diff --git a/internal/gh/ghtelemetry/telemetry.go b/internal/gh/ghtelemetry/telemetry.go index c9256361b59..197b955b4c1 100644 --- a/internal/gh/ghtelemetry/telemetry.go +++ b/internal/gh/ghtelemetry/telemetry.go @@ -10,8 +10,13 @@ type Event struct { Measures Measures } +type Disabler interface { + Disable() +} + type EventRecorder interface { Record(event Event) + Disabler } type CommandRecorder interface { diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index 9112d428372..eab842c5a79 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strconv" "strings" "time" @@ -20,6 +21,7 @@ import ( "github.com/cli/cli/v2/internal/build" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/config/migration" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/internal/update" @@ -28,6 +30,8 @@ import ( "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/utils" + ghauth "github.com/cli/go-gh/v2/pkg/auth" + xcolor "github.com/cli/go-gh/v2/pkg/x/color" "github.com/cli/safeexec" "github.com/mgutz/ansi" "github.com/spf13/cobra" @@ -48,33 +52,34 @@ func Main() exitCode { buildVersion := build.Version hasDebug, _ := utils.IsDebugEnabled() - cmdFactory := factory.New(buildVersion, string(agents.Detect())) - stderr := cmdFactory.IOStreams.ErrOut - - cfg, err := cmdFactory.Config() + cfg, err := config.NewConfig() if err != nil { - fmt.Fprintf(stderr, "failed to load config: %s\n", err) + fmt.Fprintf(os.Stderr, "failed to load config: %s\n", err) return exitError } + ioStreams := newIOStreams(cfg) + stderr := ioStreams.ErrOut + + ghExecutablePath := executablePath("gh") + additionalCommonDimensions := ghtelemetry.Dimensions{ "version": strings.TrimPrefix(buildVersion, "v"), - "is_tty": strconv.FormatBool(cmdFactory.IOStreams.IsStdoutTTY()), + "is_tty": strconv.FormatBool(ioStreams.IsStdoutTTY()), "agent": string(agents.Detect()), } var telemetryService ghtelemetry.Service - if os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" { + if os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" || mightBeGHESUser(cfg) { telemetryService = &telemetry.NoOpService{} } else { - telemetryState := telemetry.ParseTelemetryState(cfg.Telemetry().Value) switch telemetryState { case telemetry.Disabled: telemetryService = &telemetry.NoOpService{} case telemetry.Logged: telemetryService = telemetry.NewService( - telemetry.LogFlusher(cmdFactory.IOStreams.ErrOut, cmdFactory.IOStreams.ColorEnabled()), + telemetry.LogFlusher(ioStreams.ErrOut, ioStreams.ColorEnabled()), telemetry.WithAdditionalCommonDimensions(additionalCommonDimensions), ) case telemetry.Enabled: @@ -84,7 +89,7 @@ func Main() exitCode { } additionalCommonDimensions["sample_rate"] = strconv.Itoa(sampleRate) telemetryService = telemetry.NewService( - telemetry.GitHubFlusher(cmdFactory.Executable()), + telemetry.GitHubFlusher(ghExecutablePath), telemetry.WithAdditionalCommonDimensions(additionalCommonDimensions), telemetry.WithSampleRate(sampleRate), ) @@ -95,6 +100,8 @@ func Main() exitCode { } defer telemetryService.Flush() + cmdFactory := factory.New(buildVersion, string(agents.Detect()), cfg, ioStreams, ghExecutablePath, telemetryService) + var m migration.MultiAccount if err := cfg.Migrate(m); err != nil { fmt.Fprintln(stderr, err) @@ -211,7 +218,7 @@ func Main() exitCode { updateCancel() // if the update checker hasn't completed by now, abort it newRelease := <-updateMessageChan if newRelease != nil { - isHomebrew := isUnderHomebrew(cmdFactory.Executable()) + isHomebrew := isUnderHomebrew(cmdFactory.ExecutablePath) if isHomebrew && isRecentRelease(newRelease.PublishedAt) { // do not notify Homebrew users before the version bump had a chance to get merged into homebrew-core return exitOK @@ -289,3 +296,148 @@ func isUnderHomebrew(ghBinary string) bool { brewBinPrefix := filepath.Join(strings.TrimSpace(string(brewPrefixBytes)), "bin") + string(filepath.Separator) return strings.HasPrefix(ghBinary, brewBinPrefix) } + +func newIOStreams(cfg gh.Config) *iostreams.IOStreams { + io := iostreams.System() + + if _, ghPromptDisabled := os.LookupEnv("GH_PROMPT_DISABLED"); ghPromptDisabled { + io.SetNeverPrompt(true) + } else if prompt := cfg.Prompt(""); prompt.Value == "disabled" { + io.SetNeverPrompt(true) + } + + falseyValues := []string{"false", "0", "no", ""} + + accessiblePrompterValue, accessiblePrompterIsSet := os.LookupEnv("GH_ACCESSIBLE_PROMPTER") + if accessiblePrompterIsSet { + if !slices.Contains(falseyValues, accessiblePrompterValue) { + io.SetAccessiblePrompterEnabled(true) + } + } else if prompt := cfg.AccessiblePrompter(""); prompt.Value == "enabled" { + io.SetAccessiblePrompterEnabled(true) + } + + experimentalPrompterValue, experimentalPrompterIsSet := os.LookupEnv("GH_EXPERIMENTAL_PROMPTER") + if experimentalPrompterIsSet { + if !slices.Contains(falseyValues, experimentalPrompterValue) { + io.SetExperimentalPrompterEnabled(true) + } + } + + ghSpinnerDisabledValue, ghSpinnerDisabledIsSet := os.LookupEnv("GH_SPINNER_DISABLED") + if ghSpinnerDisabledIsSet { + if !slices.Contains(falseyValues, ghSpinnerDisabledValue) { + io.SetSpinnerDisabled(true) + } + } else if spinnerDisabled := cfg.Spinner(""); spinnerDisabled.Value == "disabled" { + io.SetSpinnerDisabled(true) + } + + // Pager precedence + // 1. GH_PAGER + // 2. pager from config + // 3. PAGER + if ghPager, ghPagerExists := os.LookupEnv("GH_PAGER"); ghPagerExists { + io.SetPager(ghPager) + } else if pager := cfg.Pager(""); pager.Value != "" { + io.SetPager(pager.Value) + } + + if ghColorLabels, ghColorLabelsExists := os.LookupEnv("GH_COLOR_LABELS"); ghColorLabelsExists { + switch ghColorLabels { + case "", "0", "false", "no": + io.SetColorLabels(false) + default: + io.SetColorLabels(true) + } + } else if prompt := cfg.ColorLabels(""); prompt.Value == "enabled" { + io.SetColorLabels(true) + } + + io.SetAccessibleColorsEnabled(xcolor.IsAccessibleColorsEnabled()) + + return io +} + +// Executable is the path to the currently invoked binary +func executablePath(executableName string) string { + ghPath := os.Getenv("GH_PATH") + if ghPath != "" { + return ghPath + } + + if strings.ContainsRune(executableName, os.PathSeparator) { + return executableName + } + + return executable(executableName) +} + +// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. +// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in +// PATH, return the absolute location to the program. +// +// The idea is that the result of this function is callable in the future and refers to the same +// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software +// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. +// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of +// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew +// location. +// +// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute +// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git +// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh +// auth login`, running `brew update` will print out authentication errors as git is unable to locate +// Homebrew-installed `gh` +func executable(fallback string) string { + exe, err := os.Executable() + if err != nil { + return fallback + } + + base := filepath.Base(exe) + path := os.Getenv("PATH") + for _, dir := range filepath.SplitList(path) { + p, err := filepath.Abs(filepath.Join(dir, base)) + if err != nil { + continue + } + f, err := os.Lstat(p) + if err != nil { + continue + } + + if p == exe { + return p + } else if f.Mode()&os.ModeSymlink != 0 { + realP, err := filepath.EvalSymlinks(p) + if err != nil { + continue + } + realExe, err := filepath.EvalSymlinks(exe) + if err != nil { + continue + } + if realP == realExe { + return p + } + } + } + + return exe +} + +func mightBeGHESUser(cfg gh.Config) bool { + if os.Getenv("GH_ENTERPRISE_TOKEN") != "" || os.Getenv("GITHUB_ENTERPRISE_TOKEN") != "" { + return true + } + + if host := os.Getenv("GH_HOST"); host != "" && ghauth.IsEnterprise(host) { + return true + } + + // If any targeted host is Enterprise, then the user is likely a GHES user. + return slices.ContainsFunc(cfg.Authentication().Hosts(), func(host string) bool { + return ghauth.IsEnterprise(host) + }) +} diff --git a/internal/ghcmd/cmd_test.go b/internal/ghcmd/cmd_test.go index 08bbceb8532..65bcc0f288e 100644 --- a/internal/ghcmd/cmd_test.go +++ b/internal/ghcmd/cmd_test.go @@ -7,8 +7,11 @@ import ( "net" "testing" + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" ) func Test_printError(t *testing.T) { @@ -76,3 +79,435 @@ check your internet connection or https://githubstatus.com }) } } + +func Test_newIOStreams_pager(t *testing.T) { + tests := []struct { + name string + env map[string]string + config gh.Config + wantPager string + }{ + { + name: "GH_PAGER and PAGER set", + env: map[string]string{ + "GH_PAGER": "GH_PAGER", + "PAGER": "PAGER", + }, + wantPager: "GH_PAGER", + }, + { + name: "GH_PAGER and config pager set", + env: map[string]string{ + "GH_PAGER": "GH_PAGER", + }, + config: pagerConfig(), + wantPager: "GH_PAGER", + }, + { + name: "config pager and PAGER set", + env: map[string]string{ + "PAGER": "PAGER", + }, + config: pagerConfig(), + wantPager: "CONFIG_PAGER", + }, + { + name: "only PAGER set", + env: map[string]string{ + "PAGER": "PAGER", + }, + wantPager: "PAGER", + }, + { + name: "GH_PAGER set to blank string", + env: map[string]string{ + "GH_PAGER": "", + "PAGER": "PAGER", + }, + wantPager: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.env != nil { + for k, v := range tt.env { + t.Setenv(k, v) + } + } + var cfg gh.Config + if tt.config != nil { + cfg = tt.config + } else { + cfg = config.NewBlankConfig() + } + io := newIOStreams(cfg) + assert.Equal(t, tt.wantPager, io.GetPager()) + }) + } +} + +func Test_newIOStreams_prompt(t *testing.T) { + tests := []struct { + name string + config gh.Config + promptDisabled bool + env map[string]string + }{ + { + name: "default config", + promptDisabled: false, + }, + { + name: "config with prompt disabled", + config: disablePromptConfig(), + promptDisabled: true, + }, + { + name: "prompt disabled via GH_PROMPT_DISABLED env var", + env: map[string]string{"GH_PROMPT_DISABLED": "1"}, + promptDisabled: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.env != nil { + for k, v := range tt.env { + t.Setenv(k, v) + } + } + var cfg gh.Config + if tt.config != nil { + cfg = tt.config + } else { + cfg = config.NewBlankConfig() + } + io := newIOStreams(cfg) + assert.Equal(t, tt.promptDisabled, io.GetNeverPrompt()) + }) + } +} + +func Test_newIOStreams_spinnerDisabled(t *testing.T) { + tests := []struct { + name string + config gh.Config + spinnerDisabled bool + env map[string]string + }{ + { + name: "default config", + spinnerDisabled: false, + }, + { + name: "config with spinner disabled", + config: disableSpinnersConfig(), + spinnerDisabled: true, + }, + { + name: "config with spinner enabled", + config: enableSpinnersConfig(), + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = 0", + env: map[string]string{"GH_SPINNER_DISABLED": "0"}, + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = false", + env: map[string]string{"GH_SPINNER_DISABLED": "false"}, + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = no", + env: map[string]string{"GH_SPINNER_DISABLED": "no"}, + spinnerDisabled: false, + }, + { + name: "spinner enabled via GH_SPINNER_DISABLED env var = 1", + env: map[string]string{"GH_SPINNER_DISABLED": "1"}, + spinnerDisabled: true, + }, + { + name: "spinner enabled via GH_SPINNER_DISABLED env var = true", + env: map[string]string{"GH_SPINNER_DISABLED": "true"}, + spinnerDisabled: true, + }, + { + name: "config enabled but env disabled, respects env", + config: enableSpinnersConfig(), + env: map[string]string{"GH_SPINNER_DISABLED": "true"}, + spinnerDisabled: true, + }, + { + name: "config disabled but env enabled, respects env", + config: disableSpinnersConfig(), + env: map[string]string{"GH_SPINNER_DISABLED": "false"}, + spinnerDisabled: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for k, v := range tt.env { + t.Setenv(k, v) + } + var cfg gh.Config + if tt.config != nil { + cfg = tt.config + } else { + cfg = config.NewBlankConfig() + } + io := newIOStreams(cfg) + assert.Equal(t, tt.spinnerDisabled, io.GetSpinnerDisabled()) + }) + } +} + +func Test_newIOStreams_accessiblePrompterEnabled(t *testing.T) { + tests := []struct { + name string + config gh.Config + accessiblePrompterEnabled bool + env map[string]string + }{ + { + name: "default config", + accessiblePrompterEnabled: false, + }, + { + name: "config with accessible prompter enabled", + config: enableAccessiblePrompterConfig(), + accessiblePrompterEnabled: true, + }, + { + name: "config with accessible prompter disabled", + config: disableAccessiblePrompterConfig(), + accessiblePrompterEnabled: false, + }, + { + name: "accessible prompter enabled via GH_ACCESSIBLE_PROMPTER env var = 1", + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "1"}, + accessiblePrompterEnabled: true, + }, + { + name: "accessible prompter enabled via GH_ACCESSIBLE_PROMPTER env var = true", + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "true"}, + accessiblePrompterEnabled: true, + }, + { + name: "accessible prompter disabled via GH_ACCESSIBLE_PROMPTER env var = 0", + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "0"}, + accessiblePrompterEnabled: false, + }, + { + name: "config disabled but env enabled, respects env", + config: disableAccessiblePrompterConfig(), + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "true"}, + accessiblePrompterEnabled: true, + }, + { + name: "config enabled but env disabled, respects env", + config: enableAccessiblePrompterConfig(), + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "false"}, + accessiblePrompterEnabled: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for k, v := range tt.env { + t.Setenv(k, v) + } + var cfg gh.Config + if tt.config != nil { + cfg = tt.config + } else { + cfg = config.NewBlankConfig() + } + io := newIOStreams(cfg) + assert.Equal(t, tt.accessiblePrompterEnabled, io.AccessiblePrompterEnabled()) + }) + } +} + +func Test_newIOStreams_colorLabels(t *testing.T) { + tests := []struct { + name string + config gh.Config + colorLabelsEnabled bool + env map[string]string + }{ + { + name: "default config", + colorLabelsEnabled: false, + }, + { + name: "config with colorLabels enabled", + config: enableColorLabelsConfig(), + colorLabelsEnabled: true, + }, + { + name: "config with colorLabels disabled", + config: disableColorLabelsConfig(), + colorLabelsEnabled: false, + }, + { + name: "colorLabels enabled via `1` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "1"}, + colorLabelsEnabled: true, + }, + { + name: "colorLabels enabled via `true` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "true"}, + colorLabelsEnabled: true, + }, + { + name: "colorLabels enabled via `yes` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "yes"}, + colorLabelsEnabled: true, + }, + { + name: "colorLabels disable via empty string in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": ""}, + colorLabelsEnabled: false, + }, + { + name: "colorLabels disabled via `0` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "0"}, + colorLabelsEnabled: false, + }, + { + name: "colorLabels disabled via `false` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "false"}, + colorLabelsEnabled: false, + }, + { + name: "colorLabels disabled via `no` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "no"}, + colorLabelsEnabled: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.env != nil { + for k, v := range tt.env { + t.Setenv(k, v) + } + } + var cfg gh.Config + if tt.config != nil { + cfg = tt.config + } else { + cfg = config.NewBlankConfig() + } + io := newIOStreams(cfg) + assert.Equal(t, tt.colorLabelsEnabled, io.ColorLabels()) + }) + } +} + +func Test_mightBeGHESUser(t *testing.T) { + tests := []struct { + name string + env map[string]string + config gh.Config + want bool + }{ + { + name: "GH_ENTERPRISE_TOKEN set", + env: map[string]string{"GH_ENTERPRISE_TOKEN": "some-token"}, + config: config.NewBlankConfig(), + want: true, + }, + { + name: "GITHUB_ENTERPRISE_TOKEN set", + env: map[string]string{"GITHUB_ENTERPRISE_TOKEN": "some-token"}, + config: config.NewBlankConfig(), + want: true, + }, + { + name: "no env vars, config has enterprise host", + config: config.NewFromString("hosts:\n ghes.example.com:\n oauth_token: abc123\n"), + want: true, + }, + { + name: "no env vars, config has only github.com", + config: config.NewFromString("hosts:\n github.com:\n oauth_token: abc123\n"), + want: false, + }, + { + name: "no env vars, config has no hosts", + config: config.NewBlankConfig(), + want: false, + }, + { + name: "no env vars, config has github.com and enterprise host", + config: config.NewFromString("hosts:\n github.com:\n oauth_token: abc123\n ghes.example.com:\n oauth_token: def456\n"), + want: true, + }, + { + name: "no env vars, config has tenancy host", + config: config.NewFromString("hosts:\n my-company.ghe.com:\n oauth_token: abc123\n"), + want: false, + }, + { + name: "GH_HOST set to enterprise host", + env: map[string]string{"GH_HOST": "ghes.example.com"}, + config: config.NewBlankConfig(), + want: true, + }, + { + name: "GH_HOST set to github.com", + env: map[string]string{"GH_HOST": "github.com"}, + config: config.NewBlankConfig(), + want: false, + }, + { + name: "GH_HOST set to tenancy host", + env: map[string]string{"GH_HOST": "my-company.ghe.com"}, + config: config.NewBlankConfig(), + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for k, v := range tt.env { + t.Setenv(k, v) + } + got := mightBeGHESUser(tt.config) + assert.Equal(t, tt.want, got) + }) + } +} + +func pagerConfig() gh.Config { + return config.NewFromString("pager: CONFIG_PAGER") +} + +func disablePromptConfig() gh.Config { + return config.NewFromString("prompt: disabled") +} + +func enableAccessiblePrompterConfig() gh.Config { + return config.NewFromString("accessible_prompter: enabled") +} + +func disableAccessiblePrompterConfig() gh.Config { + return config.NewFromString("accessible_prompter: disabled") +} + +func disableSpinnersConfig() gh.Config { + return config.NewFromString("spinner: disabled") +} + +func enableSpinnersConfig() gh.Config { + return config.NewFromString("spinner: enabled") +} + +func disableColorLabelsConfig() gh.Config { + return config.NewFromString("color_labels: disabled") +} + +func enableColorLabelsConfig() gh.Config { + return config.NewFromString("color_labels: enabled") +} diff --git a/pkg/cmdutil/factory_test.go b/internal/ghcmd/executable_test.go similarity index 94% rename from pkg/cmdutil/factory_test.go rename to internal/ghcmd/executable_test.go index 0103a04f1b5..f0374429bcd 100644 --- a/pkg/cmdutil/factory_test.go +++ b/internal/ghcmd/executable_test.go @@ -1,10 +1,12 @@ -package cmdutil +package ghcmd import ( "os" "path/filepath" "strings" "testing" + + "github.com/stretchr/testify/require" ) func Test_executable(t *testing.T) { @@ -113,11 +115,8 @@ func Test_executable_relative(t *testing.T) { } } -func Test_Executable_override(t *testing.T) { +func TestExecutablePath(t *testing.T) { override := strings.Join([]string{"C:", "cygwin64", "home", "gh.exe"}, string(os.PathSeparator)) t.Setenv("GH_PATH", override) - f := Factory{} - if got := f.Executable(); got != override { - t.Errorf("executable() = %q, want %q", got, override) - } + require.Equal(t, override, executablePath("gh")) } diff --git a/internal/telemetry/fake.go b/internal/telemetry/fake.go index ee38262d9d5..1dc45ab2692 100644 --- a/internal/telemetry/fake.go +++ b/internal/telemetry/fake.go @@ -10,4 +10,6 @@ func (r *EventRecorderSpy) Record(event ghtelemetry.Event) { r.Events = append(r.Events, event) } +func (r *EventRecorderSpy) Disable() {} + func (r *EventRecorderSpy) Flush() {} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index f8698706ac3..b046ec77d84 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -248,6 +248,15 @@ type service struct { sampleBucket byte events []recordedEvent + + disabled bool +} + +func (s *service) Disable() { + s.mu.Lock() + defer s.mu.Unlock() + + s.disabled = true } func (s *service) Record(event ghtelemetry.Event) { @@ -269,6 +278,10 @@ func (s *service) Flush() { s.mu.Lock() defer s.mu.Unlock() + if s.disabled { + return + } + if s.previouslyCalled { return } @@ -379,6 +392,8 @@ type NoOpService struct{} func (s *NoOpService) Record(event ghtelemetry.Event) {} +func (s *NoOpService) Disable() {} + func (s *NoOpService) SetSampleRate(rate int) {} func (s *NoOpService) Flush() {} diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index 0142d4d1611..207d611eeca 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -598,10 +598,54 @@ func TestWithAdditionalCommonDimensions(t *testing.T) { assert.NotEmpty(t, captured.Events[0].Dimensions["architecture"]) } +func TestServiceDisable(t *testing.T) { + t.Run("prevents flush from sending events", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + called := false + svc := newService(func(SendTelemetryPayload) { called = true }, nil) + + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Disable() + svc.Flush() + + assert.False(t, called, "flusher should not be called after Disable()") + }) + + t.Run("prevents flush even with multiple recorded events", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + called := false + svc := newService(func(SendTelemetryPayload) { called = true }, nil) + + svc.Record(ghtelemetry.Event{Type: "event1"}) + svc.Record(ghtelemetry.Event{Type: "event2"}) + svc.Record(ghtelemetry.Event{Type: "event3"}) + svc.Disable() + svc.Flush() + + assert.False(t, called, "flusher should not be called after Disable()") + }) + + t.Run("can be called before any events are recorded", func(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + called := false + svc := newService(func(SendTelemetryPayload) { called = true }, nil) + + svc.Disable() + svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Flush() + + assert.False(t, called, "flusher should not be called when disabled before recording") + }) +} + func TestNoOpService(t *testing.T) { svc := &NoOpService{} // All methods should be safe to call without panicking svc.Record(ghtelemetry.Event{Type: "test"}) + svc.Disable() svc.SetSampleRate(50) svc.Flush() } diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index fb641457f0f..4a87e0f8cb9 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -223,7 +223,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command }, Args: cobra.ExactArgs(1), PreRun: func(c *cobra.Command, args []string) { - opts.BaseRepo = cmdutil.OverrideBaseRepoFunc(f, "") + opts.BaseRepo = cmdutil.OverrideBaseRepoFunc(f.BaseRepo, "") }, RunE: func(c *cobra.Command, args []string) error { opts.RequestPath = args[0] diff --git a/pkg/cmd/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index ec64cefa7a3..10a1e521657 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -1,17 +1,19 @@ -//go:build integration - package verify import ( "net/http" "testing" + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmd/attestation/api" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" "github.com/cli/cli/v2/pkg/cmd/attestation/io" "github.com/cli/cli/v2/pkg/cmd/attestation/test" "github.com/cli/cli/v2/pkg/cmd/attestation/verification" "github.com/cli/cli/v2/pkg/cmd/factory" + "github.com/cli/cli/v2/pkg/iostreams" o "github.com/cli/cli/v2/pkg/option" "github.com/cli/go-gh/v2/pkg/auth" "github.com/stretchr/testify/require" @@ -26,12 +28,15 @@ func TestVerifyIntegration(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test", "") - - hc, err := cmdFactory.HttpClient() - if err != nil { - t.Fatal(err) - } + ios, _, _, _ := iostreams.Test() + hc, err := factory.HttpClientFunc( + &config.AuthConfig{}, + ios, + "test", + "", + &telemetry.NoOpService{}, + )() + require.NoError(t, err) host, _ := auth.DefaultHost() @@ -143,12 +148,15 @@ func TestVerifyIntegrationCustomIssuer(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test", "") - - hc, err := cmdFactory.HttpClient() - if err != nil { - t.Fatal(err) - } + ios, _, _, _ := iostreams.Test() + hc, err := factory.HttpClientFunc( + &config.AuthConfig{}, + ios, + "test", + "", + &telemetry.NoOpService{}, + )() + require.NoError(t, err) host, _ := auth.DefaultHost() @@ -217,12 +225,16 @@ func TestVerifyIntegrationReusableWorkflow(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test", "") - - hc, err := cmdFactory.HttpClient() - if err != nil { - t.Fatal(err) - } + cfg := config.NewBlankConfig() + ios, _, _, _ := iostreams.Test() + hc, err := factory.HttpClientFunc( + cfg.Authentication(), + ios, + "test", + "", + &telemetry.NoOpService{}, + )() + require.NoError(t, err) host, _ := auth.DefaultHost() @@ -310,22 +322,28 @@ func TestVerifyIntegrationReusableWorkflowSignerWorkflow(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test", "") - - hc, err := cmdFactory.HttpClient() - if err != nil { - t.Fatal(err) - } + cfg := config.NewBlankConfig() + ios, _, _, _ := iostreams.Test() + hc, err := factory.HttpClientFunc( + cfg.Authentication(), + ios, + "test", + "", + &telemetry.NoOpService{}, + )() + require.NoError(t, err) host, _ := auth.DefaultHost() sigstoreVerifier, err := verification.NewLiveSigstoreVerifier(sigstoreConfig) require.NoError(t, err) baseOpts := Options{ - APIClient: api.NewLiveClient(hc, host, logger), - ArtifactPath: artifactPath, - BundlePath: bundlePath, - Config: cmdFactory.Config, + APIClient: api.NewLiveClient(hc, host, logger), + ArtifactPath: artifactPath, + BundlePath: bundlePath, + Config: func() (gh.Config, error) { + return cfg, nil + }, DigestAlgorithm: "sha256", Logger: logger, OCIClient: oci.NewLiveClient(), diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 88bc09f63a0..24d30c56244 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -138,7 +138,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm opts.Hostname, _ = ghauth.DefaultHost() } - opts.MainExecutable = f.Executable() + opts.MainExecutable = f.ExecutablePath if runF != nil { return runF(opts) } diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index c025df465b8..842902502cd 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -101,7 +101,7 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. return cmdutil.FlagErrorf("--hostname required when not running interactively") } - opts.MainExecutable = f.Executable() + opts.MainExecutable = f.ExecutablePath if runF != nil { return runF(opts) } diff --git a/pkg/cmd/auth/setupgit/setupgit.go b/pkg/cmd/auth/setupgit/setupgit.go index 0ff7b690360..a146a579fb9 100644 --- a/pkg/cmd/auth/setupgit/setupgit.go +++ b/pkg/cmd/auth/setupgit/setupgit.go @@ -53,7 +53,7 @@ func NewCmdSetupGit(f *cmdutil.Factory, runF func(*SetupGitOptions) error) *cobr `), RunE: func(cmd *cobra.Command, args []string) error { opts.CredentialsHelperConfig = &gitcredentials.HelperConfig{ - SelfExecutablePath: f.Executable(), + SelfExecutablePath: f.ExecutablePath, GitClient: f.GitClient, } if opts.Hostname == "" && opts.Force { diff --git a/pkg/cmd/codespace/root.go b/pkg/cmd/codespace/root.go index d1675a8f742..5d3bff3d6d8 100644 --- a/pkg/cmd/codespace/root.go +++ b/pkg/cmd/codespace/root.go @@ -7,6 +7,14 @@ import ( "github.com/spf13/cobra" ) +type ghExecutable struct { + executablePath string +} + +func (e *ghExecutable) Executable() string { + return e.executablePath +} + func NewCmdCodespace(f *cmdutil.Factory) *cobra.Command { root := &cobra.Command{ Use: "codespace", @@ -17,7 +25,7 @@ func NewCmdCodespace(f *cmdutil.Factory) *cobra.Command { app := NewApp( f.IOStreams, - f, + &ghExecutable{executablePath: f.ExecutablePath}, codespacesAPI.New(f), f.Browser, f.Remotes, diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 7afc6baa758..bf203bd4314 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -4,46 +4,46 @@ import ( "context" "fmt" "net/http" - "os" "regexp" - "slices" "time" "github.com/cli/cli/v2/api" ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/cmd/extension" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" - xcolor "github.com/cli/go-gh/v2/pkg/x/color" ) var ssoHeader string var ssoURLRE = regexp.MustCompile(`\burl=([^;]+)`) -func New(appVersion string, invokingAgent string) *cmdutil.Factory { +func New(appVersion string, invokingAgent string, cfg gh.Config, ios *iostreams.IOStreams, executablePath string, telemetryDisabler ghtelemetry.Disabler) *cmdutil.Factory { f := &cmdutil.Factory{ - AppVersion: appVersion, - InvokingAgent: invokingAgent, - Config: configFunc(), // No factory dependencies - ExecutableName: "gh", + AppVersion: appVersion, + InvokingAgent: invokingAgent, + Cfg: cfg, + Config: func() (gh.Config, error) { + return cfg, nil + }, // No factory dependencies + ExecutablePath: executablePath, } - f.IOStreams = ioStreams(f) // Depends on Config - f.HttpClient = httpClientFunc(f, appVersion, invokingAgent) // Depends on Config, IOStreams, appVersion, and invokingAgent - f.PlainHttpClient = plainHttpClientFunc(f, appVersion, invokingAgent) // Depends on IOStreams, appVersion, and invokingAgent - f.GitClient = newGitClient(f) // Depends on IOStreams, and Executable - f.Remotes = remotesFunc(f) // Depends on Config, and GitClient - f.BaseRepo = BaseRepoFunc(f) // Depends on Remotes - f.Prompter = newPrompter(f) // Depends on Config and IOStreams - f.Browser = newBrowser(f) // Depends on Config, and IOStreams - f.ExtensionManager = extensionManager(f) // Depends on Config, HttpClient, and IOStreams - f.Branch = branchFunc(f) // Depends on GitClient + f.IOStreams = ios + f.HttpClient = HttpClientFunc(cfg.Authentication(), ios, appVersion, invokingAgent, telemetryDisabler) + f.PlainHttpClient = plainHttpClientFunc(ios, appVersion, invokingAgent, telemetryDisabler) + f.GitClient = newGitClient(f) // Depends on IOStreams, and Executable + f.Remotes = remotesFunc(f) // Depends on Config, and GitClient + f.BaseRepo = BaseRepoFunc(f.Remotes) + f.Prompter = newPrompter(f) // Depends on Config and IOStreams + f.Browser = newBrowser(f) // Depends on Config, and IOStreams + f.ExtensionManager = extensionManager(f) // Depends on Config, HttpClient, and IOStreams + f.Branch = branchFunc(f) // Depends on GitClient return f } @@ -73,9 +73,9 @@ func New(appVersion string, invokingAgent string) *cmdutil.Factory { // origin https://github.com/cli/cli-fork.git (push) // // With this resolution function, the upstream will always be chosen (assuming we have authenticated with github.com). -func BaseRepoFunc(f *cmdutil.Factory) func() (ghrepo.Interface, error) { +func BaseRepoFunc(remotesFunc func() (ghContext.Remotes, error)) func() (ghrepo.Interface, error) { return func() (ghrepo.Interface, error) { - remotes, err := f.Remotes() + remotes, err := remotesFunc() if err != nil { return nil, err } @@ -187,19 +187,15 @@ func remotesFunc(f *cmdutil.Factory) func() (ghContext.Remotes, error) { return rr.Resolver() } -func httpClientFunc(f *cmdutil.Factory, appVersion string, invokingAgent string) func() (*http.Client, error) { +func HttpClientFunc(authCfg gh.AuthConfig, ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) { return func() (*http.Client, error) { - io := f.IOStreams - cfg, err := f.Config() - if err != nil { - return nil, err - } opts := api.HTTPClientOptions{ - Config: cfg.Authentication(), - Log: io.ErrOut, - LogColorize: io.ColorEnabled(), - AppVersion: appVersion, - InvokingAgent: invokingAgent, + Config: authCfg, + Log: ios.ErrOut, + LogColorize: ios.ColorEnabled(), + AppVersion: appVersion, + InvokingAgent: invokingAgent, + TelemetryDisabler: telemetryDisabler, } client, err := api.NewHTTPClient(opts) if err != nil { @@ -210,16 +206,16 @@ func httpClientFunc(f *cmdutil.Factory, appVersion string, invokingAgent string) } } -func plainHttpClientFunc(f *cmdutil.Factory, appVersion string, invokingAgent string) func() (*http.Client, error) { +func plainHttpClientFunc(ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) { return func() (*http.Client, error) { - io := f.IOStreams opts := api.HTTPClientOptions{ - Log: io.ErrOut, - LogColorize: io.ColorEnabled(), + Log: ios.ErrOut, + LogColorize: ios.ColorEnabled(), AppVersion: appVersion, InvokingAgent: invokingAgent, // This is required to prevent automatic setting of auth and other headers. SkipDefaultHeaders: true, + TelemetryDisabler: telemetryDisabler, } client, err := api.NewHTTPClient(opts) if err != nil { @@ -231,9 +227,8 @@ func plainHttpClientFunc(f *cmdutil.Factory, appVersion string, invokingAgent st func newGitClient(f *cmdutil.Factory) *git.Client { io := f.IOStreams - ghPath := f.Executable() client := &git.Client{ - GhPath: ghPath, + GhPath: f.ExecutablePath, Stderr: io.ErrOut, Stdin: io.In, Stdout: io.Out, @@ -252,18 +247,6 @@ func newPrompter(f *cmdutil.Factory) prompter.Prompter { return prompter.New(editor, io) } -func configFunc() func() (gh.Config, error) { - var cachedConfig gh.Config - var configError error - return func() (gh.Config, error) { - if cachedConfig != nil || configError != nil { - return cachedConfig, configError - } - cachedConfig, configError = config.NewConfig() - return cachedConfig, configError - } -} - func branchFunc(f *cmdutil.Factory) func() (string, error) { return func() (string, error) { currentBranch, err := f.GitClient.CurrentBranch(context.Background()) @@ -293,72 +276,6 @@ func extensionManager(f *cmdutil.Factory) *extension.Manager { return em } -func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { - io := iostreams.System() - cfg, err := f.Config() - if err != nil { - return io - } - - if _, ghPromptDisabled := os.LookupEnv("GH_PROMPT_DISABLED"); ghPromptDisabled { - io.SetNeverPrompt(true) - } else if prompt := cfg.Prompt(""); prompt.Value == "disabled" { - io.SetNeverPrompt(true) - } - - falseyValues := []string{"false", "0", "no", ""} - - accessiblePrompterValue, accessiblePrompterIsSet := os.LookupEnv("GH_ACCESSIBLE_PROMPTER") - if accessiblePrompterIsSet { - if !slices.Contains(falseyValues, accessiblePrompterValue) { - io.SetAccessiblePrompterEnabled(true) - } - } else if prompt := cfg.AccessiblePrompter(""); prompt.Value == "enabled" { - io.SetAccessiblePrompterEnabled(true) - } - - experimentalPrompterValue, experimentalPrompterIsSet := os.LookupEnv("GH_EXPERIMENTAL_PROMPTER") - if experimentalPrompterIsSet { - if !slices.Contains(falseyValues, experimentalPrompterValue) { - io.SetExperimentalPrompterEnabled(true) - } - } - - ghSpinnerDisabledValue, ghSpinnerDisabledIsSet := os.LookupEnv("GH_SPINNER_DISABLED") - if ghSpinnerDisabledIsSet { - if !slices.Contains(falseyValues, ghSpinnerDisabledValue) { - io.SetSpinnerDisabled(true) - } - } else if spinnerDisabled := cfg.Spinner(""); spinnerDisabled.Value == "disabled" { - io.SetSpinnerDisabled(true) - } - - // Pager precedence - // 1. GH_PAGER - // 2. pager from config - // 3. PAGER - if ghPager, ghPagerExists := os.LookupEnv("GH_PAGER"); ghPagerExists { - io.SetPager(ghPager) - } else if pager := cfg.Pager(""); pager.Value != "" { - io.SetPager(pager.Value) - } - - if ghColorLabels, ghColorLabelsExists := os.LookupEnv("GH_COLOR_LABELS"); ghColorLabelsExists { - switch ghColorLabels { - case "", "0", "false", "no": - io.SetColorLabels(false) - default: - io.SetColorLabels(true) - } - } else if prompt := cfg.ColorLabels(""); prompt.Value == "enabled" { - io.SetColorLabels(true) - } - - io.SetAccessibleColorsEnabled(xcolor.IsAccessibleColorsEnabled()) - - return io -} - // SSOURL returns the URL of a SAML SSO challenge received by the server for clients that use ExtractHeader // to extract the value of the "X-GitHub-SSO" response header. func SSOURL() string { diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 7d84caa8f26..6f376e48c88 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" ghmock "github.com/cli/cli/v2/internal/gh/mock" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -66,7 +67,6 @@ func Test_BaseRepo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1", "") rr := &remoteResolver{ readRemotes: func() (git.RemoteSet, error) { return tt.remotes, nil @@ -90,8 +90,10 @@ func Test_BaseRepo(t *testing.T) { return cfg, nil }, } - f.Remotes = rr.Resolver() - f.BaseRepo = BaseRepoFunc(f) + remotes := rr.Resolver() + f := &cmdutil.Factory{ + BaseRepo: BaseRepoFunc(remotes), + } repo, err := f.BaseRepo() if tt.wantsErr { assert.Error(t, err) @@ -204,7 +206,7 @@ func Test_SmartBaseRepo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1", "") + f := &cmdutil.Factory{} rr := &remoteResolver{ readRemotes: func() (git.RemoteSet, error) { return tt.remotes, nil @@ -297,7 +299,6 @@ func Test_OverrideBaseRepo(t *testing.T) { if tt.envOverride != "" { t.Setenv("GH_REPO", tt.envOverride) } - f := New("1", "") rr := &remoteResolver{ readRemotes: func() (git.RemoteSet, error) { return tt.remotes, nil @@ -306,8 +307,10 @@ func Test_OverrideBaseRepo(t *testing.T) { return tt.config, nil }, } - f.Remotes = rr.Resolver() - f.BaseRepo = cmdutil.OverrideBaseRepoFunc(f, tt.argOverride) + remotes := rr.Resolver() + f := &cmdutil.Factory{ + BaseRepo: cmdutil.OverrideBaseRepoFunc(BaseRepoFunc(remotes), tt.argOverride), + } repo, err := f.BaseRepo() if tt.wantsErr { assert.Error(t, err) @@ -321,341 +324,6 @@ func Test_OverrideBaseRepo(t *testing.T) { } } -func Test_ioStreams_pager(t *testing.T) { - tests := []struct { - name string - env map[string]string - config gh.Config - wantPager string - }{ - { - name: "GH_PAGER and PAGER set", - env: map[string]string{ - "GH_PAGER": "GH_PAGER", - "PAGER": "PAGER", - }, - wantPager: "GH_PAGER", - }, - { - name: "GH_PAGER and config pager set", - env: map[string]string{ - "GH_PAGER": "GH_PAGER", - }, - config: pagerConfig(), - wantPager: "GH_PAGER", - }, - { - name: "config pager and PAGER set", - env: map[string]string{ - "PAGER": "PAGER", - }, - config: pagerConfig(), - wantPager: "CONFIG_PAGER", - }, - { - name: "only PAGER set", - env: map[string]string{ - "PAGER": "PAGER", - }, - wantPager: "PAGER", - }, - { - name: "GH_PAGER set to blank string", - env: map[string]string{ - "GH_PAGER": "", - "PAGER": "PAGER", - }, - wantPager: "", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.env != nil { - for k, v := range tt.env { - t.Setenv(k, v) - } - } - f := New("1", "") - f.Config = func() (gh.Config, error) { - if tt.config == nil { - return config.NewBlankConfig(), nil - } else { - return tt.config, nil - } - } - io := ioStreams(f) - assert.Equal(t, tt.wantPager, io.GetPager()) - }) - } -} - -func Test_ioStreams_prompt(t *testing.T) { - tests := []struct { - name string - config gh.Config - promptDisabled bool - env map[string]string - }{ - { - name: "default config", - promptDisabled: false, - }, - { - name: "config with prompt disabled", - config: disablePromptConfig(), - promptDisabled: true, - }, - { - name: "prompt disabled via GH_PROMPT_DISABLED env var", - env: map[string]string{"GH_PROMPT_DISABLED": "1"}, - promptDisabled: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.env != nil { - for k, v := range tt.env { - t.Setenv(k, v) - } - } - f := New("1", "") - f.Config = func() (gh.Config, error) { - if tt.config == nil { - return config.NewBlankConfig(), nil - } else { - return tt.config, nil - } - } - io := ioStreams(f) - assert.Equal(t, tt.promptDisabled, io.GetNeverPrompt()) - }) - } -} - -func Test_ioStreams_spinnerDisabled(t *testing.T) { - tests := []struct { - name string - config gh.Config - spinnerDisabled bool - env map[string]string - }{ - { - name: "default config", - spinnerDisabled: false, - }, - { - name: "config with spinner disabled", - config: disableSpinnersConfig(), - spinnerDisabled: true, - }, - { - name: "config with spinner enabled", - config: enableSpinnersConfig(), - spinnerDisabled: false, - }, - { - name: "spinner disabled via GH_SPINNER_DISABLED env var = 0", - env: map[string]string{"GH_SPINNER_DISABLED": "0"}, - spinnerDisabled: false, - }, - { - name: "spinner disabled via GH_SPINNER_DISABLED env var = false", - env: map[string]string{"GH_SPINNER_DISABLED": "false"}, - spinnerDisabled: false, - }, - { - name: "spinner disabled via GH_SPINNER_DISABLED env var = no", - env: map[string]string{"GH_SPINNER_DISABLED": "no"}, - spinnerDisabled: false, - }, - { - name: "spinner enabled via GH_SPINNER_DISABLED env var = 1", - env: map[string]string{"GH_SPINNER_DISABLED": "1"}, - spinnerDisabled: true, - }, - { - name: "spinner enabled via GH_SPINNER_DISABLED env var = true", - env: map[string]string{"GH_SPINNER_DISABLED": "true"}, - spinnerDisabled: true, - }, - { - name: "config enabled but env disabled, respects env", - config: enableSpinnersConfig(), - env: map[string]string{"GH_SPINNER_DISABLED": "true"}, - spinnerDisabled: true, - }, - { - name: "config disabled but env enabled, respects env", - config: disableSpinnersConfig(), - env: map[string]string{"GH_SPINNER_DISABLED": "false"}, - spinnerDisabled: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - for k, v := range tt.env { - t.Setenv(k, v) - } - f := New("1", "") - f.Config = func() (gh.Config, error) { - if tt.config == nil { - return config.NewBlankConfig(), nil - } else { - return tt.config, nil - } - } - io := ioStreams(f) - assert.Equal(t, tt.spinnerDisabled, io.GetSpinnerDisabled()) - }) - } -} - -func Test_ioStreams_accessiblePrompterEnabled(t *testing.T) { - tests := []struct { - name string - config gh.Config - accessiblePrompterEnabled bool - env map[string]string - }{ - { - name: "default config", - accessiblePrompterEnabled: false, - }, - { - name: "config with accessible prompter enabled", - config: enableAccessiblePrompterConfig(), - accessiblePrompterEnabled: true, - }, - { - name: "config with accessible prompter disabled", - config: disableAccessiblePrompterConfig(), - accessiblePrompterEnabled: false, - }, - { - name: "accessible prompter enabled via GH_ACCESSIBLE_PROMPTER env var = 1", - env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "1"}, - accessiblePrompterEnabled: true, - }, - { - name: "accessible prompter enabled via GH_ACCESSIBLE_PROMPTER env var = true", - env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "true"}, - accessiblePrompterEnabled: true, - }, - { - name: "accessible prompter disabled via GH_ACCESSIBLE_PROMPTER env var = 0", - env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "0"}, - accessiblePrompterEnabled: false, - }, - { - name: "config disabled but env enabled, respects env", - config: disableAccessiblePrompterConfig(), - env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "true"}, - accessiblePrompterEnabled: true, - }, - { - name: "config enabled but env disabled, respects env", - config: enableAccessiblePrompterConfig(), - env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "false"}, - accessiblePrompterEnabled: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - for k, v := range tt.env { - t.Setenv(k, v) - } - f := New("1", "") - f.Config = func() (gh.Config, error) { - if tt.config == nil { - return config.NewBlankConfig(), nil - } else { - return tt.config, nil - } - } - io := ioStreams(f) - assert.Equal(t, tt.accessiblePrompterEnabled, io.AccessiblePrompterEnabled()) - }) - } -} - -func Test_ioStreams_colorLabels(t *testing.T) { - tests := []struct { - name string - config gh.Config - colorLabelsEnabled bool - env map[string]string - }{ - { - name: "default config", - colorLabelsEnabled: false, - }, - { - name: "config with colorLabels enabled", - config: enableColorLabelsConfig(), - colorLabelsEnabled: true, - }, - { - name: "config with colorLabels disabled", - config: disableColorLabelsConfig(), - colorLabelsEnabled: false, - }, - { - name: "colorLabels enabled via `1` in GH_COLOR_LABELS env var", - env: map[string]string{"GH_COLOR_LABELS": "1"}, - colorLabelsEnabled: true, - }, - { - name: "colorLabels enabled via `true` in GH_COLOR_LABELS env var", - env: map[string]string{"GH_COLOR_LABELS": "true"}, - colorLabelsEnabled: true, - }, - { - name: "colorLabels enabled via `yes` in GH_COLOR_LABELS env var", - env: map[string]string{"GH_COLOR_LABELS": "yes"}, - colorLabelsEnabled: true, - }, - { - name: "colorLabels disable via empty string in GH_COLOR_LABELS env var", - env: map[string]string{"GH_COLOR_LABELS": ""}, - colorLabelsEnabled: false, - }, - { - name: "colorLabels disabled via `0` in GH_COLOR_LABELS env var", - env: map[string]string{"GH_COLOR_LABELS": "0"}, - colorLabelsEnabled: false, - }, - { - name: "colorLabels disabled via `false` in GH_COLOR_LABELS env var", - env: map[string]string{"GH_COLOR_LABELS": "false"}, - colorLabelsEnabled: false, - }, - { - name: "colorLabels disabled via `no` in GH_COLOR_LABELS env var", - env: map[string]string{"GH_COLOR_LABELS": "no"}, - colorLabelsEnabled: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.env != nil { - for k, v := range tt.env { - t.Setenv(k, v) - } - } - f := New("1", "") - f.Config = func() (gh.Config, error) { - if tt.config == nil { - return config.NewBlankConfig(), nil - } else { - return tt.config, nil - } - } - io := ioStreams(f) - assert.Equal(t, tt.colorLabelsEnabled, io.ColorLabels()) - }) - } -} - func TestSSOURL(t *testing.T) { tests := []struct { name string @@ -683,13 +351,9 @@ func TestSSOURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1", "") - f.Config = func() (gh.Config, error) { - return config.NewBlankConfig(), nil - } + cfg := config.NewBlankConfig() ios, _, _, stderr := iostreams.Test() - f.IOStreams = ios - client, err := httpClientFunc(f, "v1.2.3", "")() + client, err := HttpClientFunc(cfg.Authentication(), ios, "v1.2.3", "", &telemetry.NoOpService{})() require.NoError(t, err) req, err := http.NewRequest("GET", ts.URL, nil) if tt.sso != "" { @@ -718,13 +382,8 @@ func TestPlainHttpClient(t *testing.T) { })) defer ts.Close() - f := New("1", "") - f.Config = func() (gh.Config, error) { - return config.NewBlankConfig(), nil - } ios, _, _, _ := iostreams.Test() - f.IOStreams = ios - client, err := plainHttpClientFunc(f, "v1.2.3", "")() + client, err := plainHttpClientFunc(ios, "v1.2.3", "", &telemetry.NoOpService{})() require.NoError(t, err) req, err := http.NewRequest("GET", ts.URL, nil) @@ -759,7 +418,7 @@ func TestNewGitClient(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1", "") + f := &cmdutil.Factory{} f.Config = func() (gh.Config, error) { if tt.config == nil { return config.NewBlankConfig(), nil @@ -767,7 +426,7 @@ func TestNewGitClient(t *testing.T) { return tt.config, nil } } - f.ExecutableName = tt.executable + f.ExecutablePath = tt.executable ios, _, _, _ := iostreams.Test() f.IOStreams = ios c := newGitClient(f) @@ -784,35 +443,3 @@ func defaultConfig() *ghmock.ConfigMock { cfg.Set("nonsense.com", "oauth_token", "BLAH") return cfg } - -func pagerConfig() gh.Config { - return config.NewFromString("pager: CONFIG_PAGER") -} - -func disablePromptConfig() gh.Config { - return config.NewFromString("prompt: disabled") -} - -func enableAccessiblePrompterConfig() gh.Config { - return config.NewFromString("accessible_prompter: enabled") -} - -func disableAccessiblePrompterConfig() gh.Config { - return config.NewFromString("accessible_prompter: disabled") -} - -func disableSpinnersConfig() gh.Config { - return config.NewFromString("spinner: disabled") -} - -func enableSpinnersConfig() gh.Config { - return config.NewFromString("spinner: enabled") -} - -func disableColorLabelsConfig() gh.Config { - return config.NewFromString("color_labels: disabled") -} - -func enableColorLabelsConfig() gh.Config { - return config.NewFromString("color_labels: enabled") -} diff --git a/pkg/cmd/search/shared/shared_test.go b/pkg/cmd/search/shared/shared_test.go index c66e0908f52..bd8060943c2 100644 --- a/pkg/cmd/search/shared/shared_test.go +++ b/pkg/cmd/search/shared/shared_test.go @@ -2,22 +2,27 @@ package shared import ( "fmt" + "net/http" "testing" "time" "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" - "github.com/cli/cli/v2/pkg/cmd/factory" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/search" "github.com/stretchr/testify/assert" ) func TestSearcher(t *testing.T) { - f := factory.New("1", "") - f.Config = func() (gh.Config, error) { - return config.NewBlankConfig(), nil + f := &cmdutil.Factory{ + Config: func() (gh.Config, error) { + return config.NewBlankConfig(), nil + }, + HttpClient: func() (*http.Client, error) { + return &http.Client{}, nil + }, } _, err := Searcher(f) assert.NoError(t, err) diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go index e39886ecda9..4bdfdd416b4 100644 --- a/pkg/cmd/skills/preview/preview.go +++ b/pkg/cmd/skills/preview/preview.go @@ -22,11 +22,11 @@ import ( ) type PreviewOptions struct { - IO *iostreams.IOStreams - HttpClient func() (*http.Client, error) - Prompter prompter.Prompter - Executable func() string - RenderFile func(string, string) string + IO *iostreams.IOStreams + HttpClient func() (*http.Client, error) + Prompter prompter.Prompter + ExecutablePath string + RenderFile func(string, string) string RepoArg string SkillName string @@ -38,10 +38,10 @@ type PreviewOptions struct { // NewCmdPreview creates the "skills preview" command. func NewCmdPreview(f *cmdutil.Factory, runF func(*PreviewOptions) error) *cobra.Command { opts := &PreviewOptions{ - IO: f.IOStreams, - HttpClient: f.HttpClient, - Prompter: f.Prompter, - Executable: f.Executable, + IO: f.IOStreams, + HttpClient: f.HttpClient, + Prompter: f.Prompter, + ExecutablePath: f.ExecutablePath, } opts.RenderFile = func(filePath, content string) string { return renderMarkdownPreview(opts.IO, filePath, content) diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index 05511484eae..2542b9d904f 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -47,12 +47,12 @@ var SkillSearchFields = []string{ } type SearchOptions struct { - IO *iostreams.IOStreams - HttpClient func() (*http.Client, error) - Config func() (gh.Config, error) - Prompter prompter.Prompter - Executable string // path to the current gh binary for install subprocess - Exporter cmdutil.Exporter + IO *iostreams.IOStreams + HttpClient func() (*http.Client, error) + Config func() (gh.Config, error) + Prompter prompter.Prompter + ExecutablePath string // path to the current gh binary for install subprocess + Exporter cmdutil.Exporter // User inputs Query string @@ -64,11 +64,11 @@ type SearchOptions struct { // NewCmdSearch creates the "skills search" command. func NewCmdSearch(f *cmdutil.Factory, runF func(*SearchOptions) error) *cobra.Command { opts := &SearchOptions{ - IO: f.IOStreams, - HttpClient: f.HttpClient, - Config: f.Config, - Prompter: f.Prompter, - Executable: f.Executable(), + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + Prompter: f.Prompter, + ExecutablePath: f.ExecutablePath, } cmd := &cobra.Command{ @@ -585,7 +585,7 @@ func promptInstall(opts *SearchOptions, skills []skillResult) error { } //nolint:gosec // arguments are from user-selected search results, not arbitrary input - cmd := exec.Command(opts.Executable, "skills", "install", s.Repo, installArg, + cmd := exec.Command(opts.ExecutablePath, "skills", "install", s.Repo, installArg, "--agent", host.ID, "--scope", scope) cmd.Stdin = os.Stdin cmd.Stdout = opts.IO.Out diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index f746ec8978d..9ea8ce4a682 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -2,9 +2,6 @@ package cmdutil import ( "net/http" - "os" - "path/filepath" - "strings" "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" @@ -18,7 +15,7 @@ import ( type Factory struct { AppVersion string - ExecutableName string + ExecutablePath string InvokingAgent string Browser browser.Browser @@ -27,8 +24,11 @@ type Factory struct { IOStreams *iostreams.IOStreams Prompter prompter.Prompter - BaseRepo func() (ghrepo.Interface, error) - Branch func() (string, error) + BaseRepo func() (ghrepo.Interface, error) + Branch func() (string, error) + Cfg gh.Config + // TODO: Config should be removed in favour of cfg being passed to the right place, + // but this is going to be very invasive and shouldn't be done as part of a feature change. Config func() (gh.Config, error) HttpClient func() (*http.Client, error) // PlainHttpClient is a special HTTP client that does not automatically set @@ -37,69 +37,3 @@ type Factory struct { PlainHttpClient func() (*http.Client, error) Remotes func() (context.Remotes, error) } - -// Executable is the path to the currently invoked binary -func (f *Factory) Executable() string { - ghPath := os.Getenv("GH_PATH") - if ghPath != "" { - return ghPath - } - if !strings.ContainsRune(f.ExecutableName, os.PathSeparator) { - f.ExecutableName = executable(f.ExecutableName) - } - return f.ExecutableName -} - -// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. -// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in -// PATH, return the absolute location to the program. -// -// The idea is that the result of this function is callable in the future and refers to the same -// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software -// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. -// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of -// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew -// location. -// -// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute -// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git -// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh -// auth login`, running `brew update` will print out authentication errors as git is unable to locate -// Homebrew-installed `gh`. -func executable(fallbackName string) string { - exe, err := os.Executable() - if err != nil { - return fallbackName - } - - base := filepath.Base(exe) - path := os.Getenv("PATH") - for _, dir := range filepath.SplitList(path) { - p, err := filepath.Abs(filepath.Join(dir, base)) - if err != nil { - continue - } - f, err := os.Lstat(p) - if err != nil { - continue - } - - if p == exe { - return p - } else if f.Mode()&os.ModeSymlink != 0 { - realP, err := filepath.EvalSymlinks(p) - if err != nil { - continue - } - realExe, err := filepath.EvalSymlinks(exe) - if err != nil { - continue - } - if realP == realExe { - return p - } - } - } - - return exe -} diff --git a/pkg/cmdutil/repo_override.go b/pkg/cmdutil/repo_override.go index 791dd919a21..b037859e737 100644 --- a/pkg/cmdutil/repo_override.go +++ b/pkg/cmdutil/repo_override.go @@ -52,12 +52,12 @@ func EnableRepoOverride(cmd *cobra.Command, f *Factory) { return err } repoOverride, _ := cmd.Flags().GetString("repo") - f.BaseRepo = OverrideBaseRepoFunc(f, repoOverride) + f.BaseRepo = OverrideBaseRepoFunc(f.BaseRepo, repoOverride) return nil } } -func OverrideBaseRepoFunc(f *Factory, override string) func() (ghrepo.Interface, error) { +func OverrideBaseRepoFunc(baseRepoFunc func() (ghrepo.Interface, error), override string) func() (ghrepo.Interface, error) { if override == "" { override = os.Getenv("GH_REPO") } @@ -66,5 +66,5 @@ func OverrideBaseRepoFunc(f *Factory, override string) func() (ghrepo.Interface, return ghrepo.FromFullName(override) } } - return f.BaseRepo + return baseRepoFunc } From 17776cafc17df463501c7aeb030584d57da6650a Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 17 Apr 2026 11:45:44 +0200 Subject: [PATCH 3/4] Apply review feedback - Harden SpawnSendTelemetry against relative executable paths - Use io.Copy for telemetry subprocess stdin write - Clean up GH_TELEMETRY/DO_NOT_TRACK help text - Fall back to built-in defaults (NoOp telemetry) on config load failure Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/ghcmd/cmd.go | 35 ++++++++++++------- internal/telemetry/telemetry.go | 15 ++++++-- .../verify/verify_integration_test.go | 8 ++--- pkg/cmd/factory/default.go | 21 +++++------ pkg/cmd/factory/default_test.go | 2 +- pkg/cmd/root/help_topic.go | 8 ++--- pkg/cmdutil/factory.go | 10 ++++-- 7 files changed, 63 insertions(+), 36 deletions(-) diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index eab842c5a79..7350437dfb4 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -52,13 +52,18 @@ func Main() exitCode { buildVersion := build.Version hasDebug, _ := utils.IsDebugEnabled() - cfg, err := config.NewConfig() - if err != nil { - fmt.Fprintf(os.Stderr, "failed to load config: %s\n", err) - return exitError + cfg, cfgErr := config.NewConfig() + if cfgErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to load config: %s\n", cfgErr) } + cfgFunc := func() (gh.Config, error) { return cfg, cfgErr } - ioStreams := newIOStreams(cfg) + var ioStreams *iostreams.IOStreams + if cfgErr == nil { + ioStreams = newIOStreams(cfg) + } else { + ioStreams = iostreams.System() + } stderr := ioStreams.ErrOut ghExecutablePath := executablePath("gh") @@ -70,9 +75,13 @@ func Main() exitCode { } var telemetryService ghtelemetry.Service - if os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" || mightBeGHESUser(cfg) { + switch { + case cfgErr != nil: + // Without a valid on-disk config we can't honour user telemetry preferences, so disable it to be safe. telemetryService = &telemetry.NoOpService{} - } else { + case os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" || mightBeGHESUser(cfg): + telemetryService = &telemetry.NoOpService{} + default: telemetryState := telemetry.ParseTelemetryState(cfg.Telemetry().Value) switch telemetryState { case telemetry.Disabled: @@ -100,12 +109,14 @@ func Main() exitCode { } defer telemetryService.Flush() - cmdFactory := factory.New(buildVersion, string(agents.Detect()), cfg, ioStreams, ghExecutablePath, telemetryService) + cmdFactory := factory.New(buildVersion, string(agents.Detect()), cfgFunc, ioStreams, ghExecutablePath, telemetryService) - var m migration.MultiAccount - if err := cfg.Migrate(m); err != nil { - fmt.Fprintln(stderr, err) - return exitError + if cfgErr == nil { + var m migration.MultiAccount + if err := cfg.Migrate(m); err != nil { + fmt.Fprintln(stderr, err) + return exitError + } } ctx := context.Background() diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index b046ec77d84..67fb8b7626e 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -354,6 +354,13 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { return } + // Resolve the executable to an absolute path before changing the child's + // working directory. Without this, a relative path (e.g. from GH_PATH) would + // be resolved against cmd.Dir at Start time and fail to spawn. + if abs, err := filepath.Abs(executable); err == nil { + executable = abs + } + cmd := exec.Command(executable, "send-telemetry") cmd.Stdout = io.Discard @@ -362,7 +369,10 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { // Set the working directory to a stable directory elsewhere so that the subprocess doesn't // hold a reference to the parent's current working directory, avoiding any weirdness around // deleting the parent process's current working directory while the child is still running. - cmd.Dir = os.TempDir() + // Only do this when we have an absolute executable path so that the child can still be found. + if filepath.IsAbs(executable) { + cmd.Dir = os.TempDir() + } // Configure the child process to be detached from the parent so that it can continue running // after the parent exits, and so that it doesn't receive any signals sent to the parent. @@ -381,7 +391,8 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { // Write the payload synchronously into the kernel pipe buffer, then close // the pipe to signal EOF. The child reads the complete payload from stdin. - _, _ = stdin.Write(payloadBytes) + // io.Copy loops until all bytes are written, avoiding any risk of a short write. + _, _ = io.Copy(stdin, bytes.NewReader(payloadBytes)) _ = stdin.Close() // Release resources associated with the child process since we will never Wait for it. diff --git a/pkg/cmd/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index 10a1e521657..b9994141313 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -30,7 +30,7 @@ func TestVerifyIntegration(t *testing.T) { ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - &config.AuthConfig{}, + func() (gh.Config, error) { return config.NewBlankConfig(), nil }, ios, "test", "", @@ -150,7 +150,7 @@ func TestVerifyIntegrationCustomIssuer(t *testing.T) { ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - &config.AuthConfig{}, + func() (gh.Config, error) { return config.NewBlankConfig(), nil }, ios, "test", "", @@ -228,7 +228,7 @@ func TestVerifyIntegrationReusableWorkflow(t *testing.T) { cfg := config.NewBlankConfig() ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - cfg.Authentication(), + func() (gh.Config, error) { return cfg, nil }, ios, "test", "", @@ -325,7 +325,7 @@ func TestVerifyIntegrationReusableWorkflowSignerWorkflow(t *testing.T) { cfg := config.NewBlankConfig() ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - cfg.Authentication(), + func() (gh.Config, error) { return cfg, nil }, ios, "test", "", diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index bf203bd4314..f61e51b452f 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -23,19 +23,16 @@ import ( var ssoHeader string var ssoURLRE = regexp.MustCompile(`\burl=([^;]+)`) -func New(appVersion string, invokingAgent string, cfg gh.Config, ios *iostreams.IOStreams, executablePath string, telemetryDisabler ghtelemetry.Disabler) *cmdutil.Factory { +func New(appVersion string, invokingAgent string, cfgFunc func() (gh.Config, error), ios *iostreams.IOStreams, executablePath string, telemetryDisabler ghtelemetry.Disabler) *cmdutil.Factory { f := &cmdutil.Factory{ - AppVersion: appVersion, - InvokingAgent: invokingAgent, - Cfg: cfg, - Config: func() (gh.Config, error) { - return cfg, nil - }, // No factory dependencies + AppVersion: appVersion, + InvokingAgent: invokingAgent, + Config: cfgFunc, ExecutablePath: executablePath, } f.IOStreams = ios - f.HttpClient = HttpClientFunc(cfg.Authentication(), ios, appVersion, invokingAgent, telemetryDisabler) + f.HttpClient = HttpClientFunc(cfgFunc, ios, appVersion, invokingAgent, telemetryDisabler) f.PlainHttpClient = plainHttpClientFunc(ios, appVersion, invokingAgent, telemetryDisabler) f.GitClient = newGitClient(f) // Depends on IOStreams, and Executable f.Remotes = remotesFunc(f) // Depends on Config, and GitClient @@ -187,10 +184,14 @@ func remotesFunc(f *cmdutil.Factory) func() (ghContext.Remotes, error) { return rr.Resolver() } -func HttpClientFunc(authCfg gh.AuthConfig, ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) { +func HttpClientFunc(cfgFunc func() (gh.Config, error), ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) { return func() (*http.Client, error) { + cfg, err := cfgFunc() + if err != nil { + return nil, err + } opts := api.HTTPClientOptions{ - Config: authCfg, + Config: cfg.Authentication(), Log: ios.ErrOut, LogColorize: ios.ColorEnabled(), AppVersion: appVersion, diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 6f376e48c88..9cf34f3b0e5 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -353,7 +353,7 @@ func TestSSOURL(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cfg := config.NewBlankConfig() ios, _, _, stderr := iostreams.Test() - client, err := HttpClientFunc(cfg.Authentication(), ios, "v1.2.3", "", &telemetry.NoOpService{})() + client, err := HttpClientFunc(func() (gh.Config, error) { return cfg, nil }, ios, "v1.2.3", "", &telemetry.NoOpService{})() require.NoError(t, err) req, err := http.NewRequest("GET", ts.URL, nil) if tt.sso != "" { diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index fbacef356cc..3002a351294 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -118,10 +118,10 @@ var HelpTopics = []helpTopic{ more compatible with speech synthesis and braille screen readers. %[1]sGH_TELEMETRY%[1]s: set to %[1]slog%[1]s to print telemetry data to standard error instead of sending it. - Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry that would have been printed when set to %[1]slog%[1]s. - - %[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry that would have been printed - when %[1]sGH_TELEMETRY%[1]s is set to %[1]slog%[1]s. %[1]sGH_TELEMETRY%[1]s takes precedence if both are set. + Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry. Takes precedence over %[1]sDO_NOT_TRACK%[1]s. + + %[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry. Ignored when + %[1]sGH_TELEMETRY%[1]s is set, which takes precedence. %[1]sGH_SPINNER_DISABLED%[1]s: set to a truthy value to replace the spinner animation with a textual progress indicator. diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 9ea8ce4a682..1faf859f00c 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -26,9 +26,13 @@ type Factory struct { BaseRepo func() (ghrepo.Interface, error) Branch func() (string, error) - Cfg gh.Config - // TODO: Config should be removed in favour of cfg being passed to the right place, - // but this is going to be very invasive and shouldn't be done as part of a feature change. + // It would be nice if Config were just loaded once at startup and an error + // were returned, but this would prevent commands like "gh version" from running. + // So for now, we eagerly load the config and don't fail if there is an error, + // and defer the error handling to commands that need it. + // HOWEVER, as an additional point, the root command setup currently DOES call + // this and errors, so we never get to "gh version" anyway. + // We need to revisit that, but I don't want to make it worse. Config func() (gh.Config, error) HttpClient func() (*http.Client, error) // PlainHttpClient is a special HTTP client that does not automatically set From 6709e315df8fac55140e7e281cd6e20262cff906 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Apr 2026 22:12:01 +0200 Subject: [PATCH 4/4] Do not send telemetry for aliases --- .../telemetry/no-telemetry-for-alias.txtar | 18 +++++++++++++ pkg/cmd/root/alias.go | 27 ++++++++++++------- 2 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 acceptance/testdata/telemetry/no-telemetry-for-alias.txtar diff --git a/acceptance/testdata/telemetry/no-telemetry-for-alias.txtar b/acceptance/testdata/telemetry/no-telemetry-for-alias.txtar new file mode 100644 index 00000000000..733bea11f5c --- /dev/null +++ b/acceptance/testdata/telemetry/no-telemetry-for-alias.txtar @@ -0,0 +1,18 @@ +# Aliases should not leak their user-defined names via telemetry, but the +# resolved inner command should still record normally — its path is a core +# gh command and conveys no user-authored identifier. + +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 + +# Create a regular (non-shell) alias that resolves to an existing command. +exec gh alias set secret-project-alias version + +# Invoking the alias must not produce any event carrying the alias name. +exec gh secret-project-alias +! stderr 'secret-project-alias' + +# The resolved inner command still records telemetry as normal. +stderr 'Telemetry payload:' +stderr '"command": "gh version"' diff --git a/pkg/cmd/root/alias.go b/pkg/cmd/root/alias.go index 4f504f2b8c4..ea4c21d8a97 100644 --- a/pkg/cmd/root/alias.go +++ b/pkg/cmd/root/alias.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/v2/internal/run" "github.com/cli/cli/v2/internal/text" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/findsh" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" @@ -17,7 +18,7 @@ import ( ) func NewCmdShellAlias(io *iostreams.IOStreams, aliasName, aliasValue string) *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: aliasName, Short: fmt.Sprintf("Shell alias for %q", text.Truncate(80, aliasValue)), RunE: func(c *cobra.Command, args []string) error { @@ -39,16 +40,19 @@ func NewCmdShellAlias(io *iostreams.IOStreams, aliasName, aliasValue string) *co } return nil }, - GroupID: "alias", - Annotations: map[string]string{ - "skipAuthCheck": "true", - }, + GroupID: "alias", DisableFlagParsing: true, } + cmdutil.DisableAuthCheck(cmd) + // Aliases are user-defined names and must not be reported as telemetry + // dimensions, since the name itself may be sensitive (e.g. project or + // organization names). + cmdutil.DisableTelemetry(cmd) + return cmd } func NewCmdAlias(io *iostreams.IOStreams, aliasName, aliasValue string) *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: aliasName, Short: fmt.Sprintf("Alias for %q", text.Truncate(80, aliasValue)), RunE: func(c *cobra.Command, args []string) error { @@ -60,12 +64,15 @@ func NewCmdAlias(io *iostreams.IOStreams, aliasName, aliasValue string) *cobra.C root.SetArgs(expandedArgs) return root.Execute() }, - GroupID: "alias", - Annotations: map[string]string{ - "skipAuthCheck": "true", - }, + GroupID: "alias", DisableFlagParsing: true, } + cmdutil.DisableAuthCheck(cmd) + // Aliases are user-defined names and must not be reported as telemetry + // dimensions, since the name itself may be sensitive (e.g. project or + // organization names). + cmdutil.DisableTelemetry(cmd) + return cmd } // ExpandAlias processes argv to see if it should be rewritten according to a user's aliases.