From 76912df6a8bd85dec4ea3947f4e5cc95862dc900 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Mon, 15 May 2023 22:15:19 +0000 Subject: [PATCH 1/7] Send command invocation info in CLI --- cli/clibase/cmd.go | 10 +++++++++ cli/root.go | 39 +++++++++++++++++++++++++++++++---- cli/vscodessh.go | 2 +- coderd/telemetry/telemetry.go | 11 ++++++++++ codersdk/client.go | 21 +++++++++++-------- 5 files changed, 69 insertions(+), 14 deletions(-) diff --git a/cli/clibase/cmd.go b/cli/clibase/cmd.go index 7d3547dd8839d..dff1e10a63b9b 100644 --- a/cli/clibase/cmd.go +++ b/cli/clibase/cmd.go @@ -145,6 +145,16 @@ func (c *Cmd) FullUsage() string { return strings.Join(uses, " ") } +// FullOptions returns the options of the command and its parents. +func (c *Cmd) FullOptions() OptionSet { + var opts OptionSet + if c.Parent != nil { + opts = append(opts, c.Parent.FullOptions()...) + } + opts = append(opts, c.Options...) + return opts +} + // Invoke creates a new invocation of the command, with // stdio discarded. // diff --git a/cli/root.go b/cli/root.go index 6702754abb0bf..c70a3ad88a358 100644 --- a/cli/root.go +++ b/cli/root.go @@ -2,6 +2,8 @@ package cli import ( "context" + "encoding/base64" + "encoding/json" "errors" "flag" "fmt" @@ -33,6 +35,7 @@ import ( "github.com/coder/coder/cli/config" "github.com/coder/coder/coderd" "github.com/coder/coder/coderd/gitauth" + "github.com/coder/coder/coderd/telemetry" "github.com/coder/coder/codersdk" "github.com/coder/coder/codersdk/agentsdk" ) @@ -425,6 +428,23 @@ type RootCmd struct { noFeatureWarning bool } +func telemetryInvocation(i *clibase.Invocation) telemetry.CLIInvocation { + var topts []telemetry.CLIOption + for _, opt := range i.Command.FullOptions() { + if opt.Value.String() == opt.Default { + continue + } + topts = append(topts, telemetry.CLIOption{ + Name: opt.Name, + ValueSource: string(opt.ValueSource), + }) + } + return telemetry.CLIInvocation{ + Command: i.Command.FullName(), + Options: topts, + } +} + // InitClient sets client to a new client. // It reads from global configuration files if flags are not set. func (r *RootCmd) InitClient(client *codersdk.Client) clibase.MiddlewareFunc { @@ -465,7 +485,18 @@ func (r *RootCmd) InitClient(client *codersdk.Client) clibase.MiddlewareFunc { } } - err = r.setClient(client, r.clientURL) + telemInv := telemetryInvocation(i) + byt, err := json.Marshal(telemInv) + if err != nil { + // Should be impossible + panic(err) + } + err = r.setClient( + client, r.clientURL, + append(r.header, "Coder-CLI-Invokation="+ + base64.StdEncoding.EncodeToString(byt), + ), + ) if err != nil { return err } @@ -512,12 +543,12 @@ func (r *RootCmd) InitClient(client *codersdk.Client) clibase.MiddlewareFunc { } } -func (r *RootCmd) setClient(client *codersdk.Client, serverURL *url.URL) error { +func (*RootCmd) setClient(client *codersdk.Client, serverURL *url.URL, headers []string) error { transport := &headerTransport{ transport: http.DefaultTransport, header: http.Header{}, } - for _, header := range r.header { + for _, header := range headers { parts := strings.SplitN(header, "=", 2) if len(parts) < 2 { return xerrors.Errorf("split header %q had less than two parts", header) @@ -533,7 +564,7 @@ func (r *RootCmd) setClient(client *codersdk.Client, serverURL *url.URL) error { func (r *RootCmd) createUnauthenticatedClient(serverURL *url.URL) (*codersdk.Client, error) { var client codersdk.Client - err := r.setClient(&client, serverURL) + err := r.setClient(&client, serverURL, r.header) return &client, err } diff --git a/cli/vscodessh.go b/cli/vscodessh.go index 363f0215a7696..424aa362e3590 100644 --- a/cli/vscodessh.go +++ b/cli/vscodessh.go @@ -83,7 +83,7 @@ func (r *RootCmd) vscodeSSH() *clibase.Cmd { client.SetSessionToken(string(sessionToken)) // This adds custom headers to the request! - err = r.setClient(client, serverURL) + err = r.setClient(client, serverURL, r.header) if err != nil { return xerrors.Errorf("set client: %w", err) } diff --git a/coderd/telemetry/telemetry.go b/coderd/telemetry/telemetry.go index 9e53e5b63506d..06013aaba1b2e 100644 --- a/coderd/telemetry/telemetry.go +++ b/coderd/telemetry/telemetry.go @@ -700,6 +700,7 @@ type Snapshot struct { WorkspaceBuilds []WorkspaceBuild `json:"workspace_build"` WorkspaceResources []WorkspaceResource `json:"workspace_resources"` WorkspaceResourceMetadata []WorkspaceResourceMetadata `json:"workspace_resource_metadata"` + CLIInvokations []CLIInvocation `json:"cli_invocations"` } // Deployment contains information about the host running Coder. @@ -874,6 +875,16 @@ type License struct { UUID uuid.UUID `json:"uuid"` } +type CLIOption struct { + Name string `json:"name"` + ValueSource string `json:"set_via"` +} + +type CLIInvocation struct { + Command string `json:"command"` + Options []CLIOption +} + type noopReporter struct{} func (*noopReporter) Report(_ *Snapshot) {} diff --git a/codersdk/client.go b/codersdk/client.go index a558c3d4f5a76..84cb6ecfb5371 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -179,15 +179,6 @@ func (c *Client) Request(ctx context.Context, method, path string, body interfac return nil, xerrors.Errorf("create request: %w", err) } - if c.PlainLogger != nil { - out, err := httputil.DumpRequest(req, c.LogBodies) - if err != nil { - return nil, xerrors.Errorf("dump request: %w", err) - } - out = prefixLines([]byte("http --> "), out) - _, _ = c.PlainLogger.Write(out) - } - tokenHeader := c.SessionTokenHeader if tokenHeader == "" { tokenHeader = SessionTokenHeader @@ -221,6 +212,18 @@ func (c *Client) Request(ctx context.Context, method, path string, body interfac }) resp, err := c.HTTPClient.Do(req) + + // We log after sending the request because the HTTP Transport may modify + // the request within Do, e.g. by adding headers. + if resp != nil && c.PlainLogger != nil { + out, err := httputil.DumpRequest(resp.Request, c.LogBodies) + if err != nil { + return nil, xerrors.Errorf("dump request: %w", err) + } + out = prefixLines([]byte("http --> "), out) + _, _ = c.PlainLogger.Write(out) + } + if err != nil { return nil, err } From 0b2d56b0fa658f696b0b30f169519842fc0dd737 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Tue, 16 May 2023 00:25:03 +0000 Subject: [PATCH 2/7] Get to middleware --- cli/root.go | 4 +- coderd/httpmw/telemetry.go | 78 +++++++++++++++++++++++++++++++++++ coderd/telemetry/telemetry.go | 6 +-- codersdk/client.go | 10 +++++ 4 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 coderd/httpmw/telemetry.go diff --git a/cli/root.go b/cli/root.go index c70a3ad88a358..80fb57e21fe94 100644 --- a/cli/root.go +++ b/cli/root.go @@ -431,7 +431,7 @@ type RootCmd struct { func telemetryInvocation(i *clibase.Invocation) telemetry.CLIInvocation { var topts []telemetry.CLIOption for _, opt := range i.Command.FullOptions() { - if opt.Value.String() == opt.Default { + if opt.ValueSource == clibase.ValueSourceNone || opt.ValueSource == clibase.ValueSourceDefault { continue } topts = append(topts, telemetry.CLIOption{ @@ -493,7 +493,7 @@ func (r *RootCmd) InitClient(client *codersdk.Client) clibase.MiddlewareFunc { } err = r.setClient( client, r.clientURL, - append(r.header, "Coder-CLI-Invokation="+ + append(r.header, codersdk.CLITelemetryHeader+"="+ base64.StdEncoding.EncodeToString(byt), ), ) diff --git a/coderd/httpmw/telemetry.go b/coderd/httpmw/telemetry.go new file mode 100644 index 0000000000000..db250ad2ba31b --- /dev/null +++ b/coderd/httpmw/telemetry.go @@ -0,0 +1,78 @@ +package httpmw + +import ( + "encoding/base64" + "encoding/json" + "net/http" + "sync" + "time" + + "tailscale.com/tstime/rate" + + "cdr.dev/slog" + "github.com/coder/coder/coderd/telemetry" + "github.com/coder/coder/codersdk" +) + +func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handler) http.Handler { + var mu sync.Mutex + + var ( + // We send telemetry at most once per minute. + limiter = rate.NewLimiter(rate.Every(time.Minute), 1) + queue []telemetry.CLIInvocation + ) + + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + defer next.ServeHTTP(rw, r) + payload := r.Header.Get(codersdk.CLITelemetryHeader) + + // We do simple checks and processing outside of the goroutine + // to avoid the overhead of an additional goroutine on every + // request. + if payload == "" { + return + } + + byt, err := base64.StdEncoding.DecodeString(payload) + if err != nil { + log.Error( + r.Context(), + "base64 decode CLI telemetry header", + slog.F("error", err), + ) + return + } + + var inv telemetry.CLIInvocation + err = json.Unmarshal(byt, &inv) + if err != nil { + log.Error( + r.Context(), + "unmarshal CLI telemetry header", + slog.Error(err), + ) + return + } + + go func() { + mu.Lock() + defer mu.Unlock() + + queue = append(queue, inv) + if !limiter.Allow() && len(queue) < 1024 { + return + } + rep.Report(&telemetry.Snapshot{ + CLIInvocations: queue, + }) + log.Debug( + r.Context(), + "reported CLI telemetry", slog.F("count", len(queue)), + ) + queue = queue[:0] + }() + }) + } +} diff --git a/coderd/telemetry/telemetry.go b/coderd/telemetry/telemetry.go index 06013aaba1b2e..4e757552970db 100644 --- a/coderd/telemetry/telemetry.go +++ b/coderd/telemetry/telemetry.go @@ -700,7 +700,7 @@ type Snapshot struct { WorkspaceBuilds []WorkspaceBuild `json:"workspace_build"` WorkspaceResources []WorkspaceResource `json:"workspace_resources"` WorkspaceResourceMetadata []WorkspaceResourceMetadata `json:"workspace_resource_metadata"` - CLIInvokations []CLIInvocation `json:"cli_invocations"` + CLIInvocations []CLIInvocation `json:"cli_invocations"` } // Deployment contains information about the host running Coder. @@ -881,8 +881,8 @@ type CLIOption struct { } type CLIInvocation struct { - Command string `json:"command"` - Options []CLIOption + Command string `json:"command"` + Options []CLIOption `json:"options"` } type noopReporter struct{} diff --git a/codersdk/client.go b/codersdk/client.go index 84cb6ecfb5371..b7206cf18238e 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -61,6 +61,16 @@ const ( // Only owners can bypass rate limits. This is typically used for scale testing. // nolint: gosec BypassRatelimitHeader = "X-Coder-Bypass-Ratelimit" + + // Note: the use of X- prefix is deprecated, and we should eventually remove + // it from BypassRatelimitHeader. + // + // See: https://datatracker.ietf.org/doc/html/rfc6648. + + // CLIInvokableHeader contains a base64-encoded representation of the CLI + // command that was invoked to produce the request. It is for internal use + // only and should not be relied upon. + CLITelemetryHeader = "Coder-CLI-Telemetry" ) // loggableMimeTypes is a list of MIME types that are safe to log From c6f75043914cf0278b65500926020934b002b01b Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Tue, 16 May 2023 16:09:35 +0000 Subject: [PATCH 3/7] Add mw --- coderd/coderd.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/coderd.go b/coderd/coderd.go index a22f632485692..e0c7ae2f9c482 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -463,6 +463,7 @@ func New(options *Options) *API { // Specific routes can specify different limits, but every rate // limit must be configurable by the admin. apiRateLimiter, + httpmw.ReportCLITelemetry(api.Logger, options.Telemetry), ) r.Get("/", apiRoot) // All CSP errors will be logged From 672117307cb3a34b0cde1e8418dd08dee5d0b5e1 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Tue, 16 May 2023 19:22:11 +0000 Subject: [PATCH 4/7] Record InvokedAt --- cli/root.go | 5 +++-- coderd/telemetry/telemetry.go | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cli/root.go b/cli/root.go index 80fb57e21fe94..3cad8a9d4a4db 100644 --- a/cli/root.go +++ b/cli/root.go @@ -440,8 +440,9 @@ func telemetryInvocation(i *clibase.Invocation) telemetry.CLIInvocation { }) } return telemetry.CLIInvocation{ - Command: i.Command.FullName(), - Options: topts, + Command: i.Command.FullName(), + Options: topts, + InvokedAt: time.Now(), } } diff --git a/coderd/telemetry/telemetry.go b/coderd/telemetry/telemetry.go index 4e757552970db..f278f6c5c1c0a 100644 --- a/coderd/telemetry/telemetry.go +++ b/coderd/telemetry/telemetry.go @@ -883,6 +883,8 @@ type CLIOption struct { type CLIInvocation struct { Command string `json:"command"` Options []CLIOption `json:"options"` + // InvokedAt is provided for deduplication purposes. + InvokedAt time.Time `json:"invoked_at"` } type noopReporter struct{} From 0649152da471e98162946a1698d69c5897531066 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Tue, 16 May 2023 19:26:48 +0000 Subject: [PATCH 5/7] Improve logging --- coderd/httpmw/telemetry.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/coderd/httpmw/telemetry.go b/coderd/httpmw/telemetry.go index db250ad2ba31b..c1af7fd0c924e 100644 --- a/coderd/httpmw/telemetry.go +++ b/coderd/httpmw/telemetry.go @@ -23,6 +23,8 @@ func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handl queue []telemetry.CLIInvocation ) + log = log.Named("cli-telemetry") + return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { defer next.ServeHTTP(rw, r) @@ -39,7 +41,7 @@ func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handl if err != nil { log.Error( r.Context(), - "base64 decode CLI telemetry header", + "base64 decode", slog.F("error", err), ) return @@ -50,7 +52,7 @@ func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handl if err != nil { log.Error( r.Context(), - "unmarshal CLI telemetry header", + "unmarshal header", slog.Error(err), ) return @@ -69,7 +71,7 @@ func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handl }) log.Debug( r.Context(), - "reported CLI telemetry", slog.F("count", len(queue)), + "report sent", slog.F("count", len(queue)), ) queue = queue[:0] }() From 586130d73ed47d67f6d66a161586850208765fcf Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Wed, 17 May 2023 21:40:28 +0000 Subject: [PATCH 6/7] Better comments --- coderd/httpmw/{telemetry.go => clitelemetry.go} | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) rename coderd/httpmw/{telemetry.go => clitelemetry.go} (90%) diff --git a/coderd/httpmw/telemetry.go b/coderd/httpmw/clitelemetry.go similarity index 90% rename from coderd/httpmw/telemetry.go rename to coderd/httpmw/clitelemetry.go index c1af7fd0c924e..8d7666e284f11 100644 --- a/coderd/httpmw/telemetry.go +++ b/coderd/httpmw/clitelemetry.go @@ -15,9 +15,9 @@ import ( ) func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handler) http.Handler { - var mu sync.Mutex - var ( + mu sync.Mutex + // We send telemetry at most once per minute. limiter = rate.NewLimiter(rate.Every(time.Minute), 1) queue []telemetry.CLIInvocation @@ -27,12 +27,10 @@ func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handl return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + // No matter what, we proceed with the request. defer next.ServeHTTP(rw, r) - payload := r.Header.Get(codersdk.CLITelemetryHeader) - // We do simple checks and processing outside of the goroutine - // to avoid the overhead of an additional goroutine on every - // request. + payload := r.Header.Get(codersdk.CLITelemetryHeader) if payload == "" { return } @@ -58,6 +56,8 @@ func ReportCLITelemetry(log slog.Logger, rep telemetry.Reporter) func(http.Handl return } + // We do expensive work in a goroutine so we don't block the + // request. go func() { mu.Lock() defer mu.Unlock() From b07281b116ce53d054aadc4f75ff73cfa4f5edf1 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Wed, 17 May 2023 21:44:40 +0000 Subject: [PATCH 7/7] comments --- coderd/telemetry/telemetry.go | 2 +- codersdk/client.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/telemetry/telemetry.go b/coderd/telemetry/telemetry.go index f278f6c5c1c0a..bd062e456ec87 100644 --- a/coderd/telemetry/telemetry.go +++ b/coderd/telemetry/telemetry.go @@ -877,7 +877,7 @@ type License struct { type CLIOption struct { Name string `json:"name"` - ValueSource string `json:"set_via"` + ValueSource string `json:"value_source"` } type CLIInvocation struct { diff --git a/codersdk/client.go b/codersdk/client.go index b7206cf18238e..19f765c097c5d 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -67,9 +67,9 @@ const ( // // See: https://datatracker.ietf.org/doc/html/rfc6648. - // CLIInvokableHeader contains a base64-encoded representation of the CLI + // CLITelemetryHeader contains a base64-encoded representation of the CLI // command that was invoked to produce the request. It is for internal use - // only and should not be relied upon. + // only. CLITelemetryHeader = "Coder-CLI-Telemetry" )