From d8138c08b8b59a39918c572e06d2d38317beadf0 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Wed, 8 Sep 2021 14:56:16 -0400 Subject: [PATCH 1/2] revert errgroup usage for ssh and logs --- cmd/ghcs/logs.go | 26 +++++++++++++++++++------- cmd/ghcs/ssh.go | 24 +++++++++++++++--------- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index f069a58ab..e35ecb728 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -11,7 +11,6 @@ import ( "github.com/github/ghcs/internal/codespaces" "github.com/github/go-liveshare" "github.com/spf13/cobra" - "golang.org/x/sync/errgroup" ) func newLogsCmd() *cobra.Command { @@ -85,12 +84,25 @@ func logs(ctx context.Context, tail bool, codespaceName string) error { ctx, localPort, dst, fmt.Sprintf("%s /workspaces/.codespaces/.persistedshare/creation.log", cmdType), ) - group, ctx := errgroup.WithContext(ctx) - group.Go(func() error { + tunnelClosed := make(chan error, 1) + go func() { fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort) - err := fwd.ForwardToListener(ctx, listen) // error is non-nil + tunnelClosed <- fwd.ForwardToListener(ctx, listen) // error is non-nil + }() + + cmdDone := make(chan error, 1) + go func() { + cmdDone <- cmd.Run() + }() + + select { + case err := <-tunnelClosed: return fmt.Errorf("connection closed: %v", err) - }) - group.Go(cmd.Run) - return group.Wait() + case err := <-cmdDone: + if err != nil { + return fmt.Errorf("error retrieving logs: %v", err) + } + + return nil // success + } } diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 6e2724e73..e2003b347 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -13,7 +13,6 @@ import ( "github.com/github/ghcs/internal/codespaces" "github.com/github/go-liveshare" "github.com/spf13/cobra" - "golang.org/x/sync/errgroup" ) func newSSHCmd() *cobra.Command { @@ -99,19 +98,26 @@ func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPo } log.Println("Ready...") - group, ctx := errgroup.WithContext(ctx) - group.Go(func() error { + tunnelClosed := make(chan error, 1) + go func() { fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort) - err := fwd.ForwardToListener(ctx, listen) // always non-nil + tunnelClosed <- fwd.ForwardToListener(ctx, listen) // always non-nil + }() + + shellClosed := make(chan error, 1) + go func() { + shellClosed <- codespaces.Shell(ctx, log, localSSHServerPort, connectDestination, usingCustomPort) + }() + + select { + case err := <-tunnelClosed: return fmt.Errorf("tunnel closed: %v", err) - }) - group.Go(func() error { - if err := codespaces.Shell(ctx, log, localSSHServerPort, connectDestination, usingCustomPort); err != nil { + case err := <-shellClosed: + if err != nil { return fmt.Errorf("shell closed: %v", err) } return nil // success - }) - return group.Wait() + } } func getContainerID(ctx context.Context, logger *output.Logger, terminal *liveshare.Terminal) (string, error) { From 3a46f2ac56e9aa23037f76e56a0d2858ea41f152 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 8 Sep 2021 17:15:42 -0400 Subject: [PATCH 2/2] add --lightstep flag for tracing --- api/api.go | 31 +++++++++++------ cmd/ghcs/main.go | 90 +++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 98 insertions(+), 23 deletions(-) diff --git a/api/api.go b/api/api.go index 7bb43811a..28c7072ea 100644 --- a/api/api.go +++ b/api/api.go @@ -18,6 +18,8 @@ import ( "net/http" "strconv" "strings" + + "github.com/opentracing/opentracing-go" ) const githubAPI = "https://api.github.com" @@ -42,7 +44,7 @@ func (a *API) GetUser(ctx context.Context) (*User, error) { } a.setHeaders(req) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/user") if err != nil { return nil, fmt.Errorf("error making request: %v", err) } @@ -87,7 +89,7 @@ func (a *API) GetRepository(ctx context.Context, nwo string) (*Repository, error } a.setHeaders(req) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/repos/*") if err != nil { return nil, fmt.Errorf("error making request: %v", err) } @@ -146,7 +148,7 @@ func (a *API) ListCodespaces(ctx context.Context, user *User) ([]*Codespace, err } a.setHeaders(req) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/vscs_internal/user/*/codespaces") if err != nil { return nil, fmt.Errorf("error making request: %v", err) } @@ -194,7 +196,7 @@ func (a *API) GetCodespaceToken(ctx context.Context, ownerLogin, codespaceName s } a.setHeaders(req) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/vscs_internal/user/*/codespaces/*/token") if err != nil { return "", fmt.Errorf("error making request: %v", err) } @@ -228,7 +230,7 @@ func (a *API) GetCodespace(ctx context.Context, token, owner, codespace string) } req.Header.Set("Authorization", "Bearer "+token) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/vscs_internal/user/*/codespaces/*") if err != nil { return nil, fmt.Errorf("error making request: %v", err) } @@ -262,7 +264,7 @@ func (a *API) StartCodespace(ctx context.Context, token string, codespace *Codes } req.Header.Set("Authorization", "Bearer "+token) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/vscs_internal/proxy/environments/*/start") if err != nil { return fmt.Errorf("error making request: %v", err) } @@ -299,7 +301,7 @@ func (a *API) GetCodespaceRegionLocation(ctx context.Context) (string, error) { return "", fmt.Errorf("error creating request: %v", err) } - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, req.URL.String()) if err != nil { return "", fmt.Errorf("error making request: %v", err) } @@ -340,7 +342,7 @@ func (a *API) GetCodespacesSKUs(ctx context.Context, user *User, repository *Rep req.URL.RawQuery = q.Encode() a.setHeaders(req) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/vscs_internal/user/*/skus") if err != nil { return nil, fmt.Errorf("error making request: %v", err) } @@ -384,7 +386,7 @@ func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repos } a.setHeaders(req) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/vscs_internal/user/*/codespaces") if err != nil { return nil, fmt.Errorf("error making request: %v", err) } @@ -414,7 +416,7 @@ func (a *API) DeleteCodespace(ctx context.Context, user *User, token, codespaceN } req.Header.Set("Authorization", "Bearer "+token) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/vscs_internal/user/*/codespaces/*") if err != nil { return fmt.Errorf("error making request: %v", err) } @@ -446,7 +448,7 @@ func (a *API) GetCodespaceRepositoryContents(ctx context.Context, codespace *Cod req.URL.RawQuery = q.Encode() a.setHeaders(req) - resp, err := a.client.Do(req) + resp, err := a.do(ctx, req, "/repos/*/contents/*") if err != nil { return nil, fmt.Errorf("error making request: %v", err) } @@ -478,6 +480,13 @@ func (a *API) GetCodespaceRepositoryContents(ctx context.Context, codespace *Cod return decoded, nil } +func (a *API) do(ctx context.Context, req *http.Request, spanName string) (*http.Response, error) { + // TODO(adonovan): use NewRequestWithContext(ctx) and drop ctx parameter. + span, ctx := opentracing.StartSpanFromContext(ctx, spanName) + defer span.Finish() + return a.client.Do(req) +} + func (a *API) setHeaders(req *http.Request) { req.Header.Set("Authorization", "Bearer "+a.token) req.Header.Set("Accept", "application/vnd.github.v3+json") diff --git a/cmd/ghcs/main.go b/cmd/ghcs/main.go index 3e861dacb..5d7d7cf23 100644 --- a/cmd/ghcs/main.go +++ b/cmd/ghcs/main.go @@ -4,8 +4,13 @@ import ( "errors" "fmt" "io" + "log" "os" + "strconv" + "strings" + "github.com/lightstep/lightstep-tracer-go" + "github.com/opentracing/opentracing-go" "github.com/spf13/cobra" ) @@ -18,22 +23,33 @@ func main() { var version = "DEV" -var rootCmd = &cobra.Command{ - Use: "ghcs", - SilenceUsage: true, // don't print usage message after each error (see #80) - SilenceErrors: false, // print errors automatically so that main need not - Long: `Unofficial CLI tool to manage GitHub Codespaces. +var rootCmd = newRootCmd() + +func newRootCmd() *cobra.Command { + var lightstep string + + root := &cobra.Command{ + Use: "ghcs", + SilenceUsage: true, // don't print usage message after each error (see #80) + SilenceErrors: false, // print errors automatically so that main need not + Long: `Unofficial CLI tool to manage GitHub Codespaces. Running commands requires the GITHUB_TOKEN environment variable to be set to a token to access the GitHub API with.`, - Version: version, + Version: version, - PersistentPreRunE: func(cmd *cobra.Command, args []string) error { - if os.Getenv("GITHUB_TOKEN") == "" { - return tokenError - } - return nil - }, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + if os.Getenv("GITHUB_TOKEN") == "" { + return tokenError + } + initLightstep(lightstep) + return nil + }, + } + + root.PersistentFlags().StringVar(&lightstep, "lightstep", "", "Lightstep tracing endpoint (service:token@host:port)") + + return root } var tokenError = errors.New("GITHUB_TOKEN is missing") @@ -45,3 +61,53 @@ func explainError(w io.Writer, err error) { return } } + +// initLightstep parses the --lightstep=service:token@host:port flag and +// enables tracing if non-empty. +func initLightstep(config string) { + if config == "" { + return + } + + cut := func(s, sep string) (pre, post string) { + if i := strings.Index(s, sep); i >= 0 { + return s[:i], s[i+len(sep):] + } + return s, "" + } + + // Parse service:password@host:port. + serviceToken, hostPort := cut(config, "@") + service, token := cut(serviceToken, ":") + host, port := cut(hostPort, ":") + portI, err := strconv.Atoi(port) + if err != nil { + log.Fatalf("invalid lightstep configuration: %s", config) + } + + // View at https://app.lightstep.com/github-prod/service-directory/ghcs/deployments + // --lightstep=ghcs:dhhPgaoavzIHz3tJMnj3Oz88Md2VC4HpwcZ8mpoWwwuOwcfU3x+K70lLhJJAXsk63T3bWfPXGgrAwTMQxLY=@lightstep-collector.service.iad.github.net:443 + // From https://app.lightstep.com/github-prod/project ghcs + + opentracing.SetGlobalTracer(lightstep.NewTracer(lightstep.Options{ + AccessToken: token, + Collector: lightstep.Endpoint{ + Host: host, + Port: portI, + Plaintext: false, + }, + Tags: opentracing.Tags{ + lightstep.ComponentNameKey: service, + }, + })) + + // Report failure to record traces. + lightstep.SetGlobalEventHandler(func(ev lightstep.Event) { + switch ev := ev.(type) { + case lightstep.EventStatusReport, lightstep.MetricEventStatusReport: + // ignore + default: + log.Printf("[trace] %s", ev) + } + }) +}