From 636da82533ea2a4823f0b9eb23c10a8ea6dce9e0 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Mon, 23 Sep 2024 08:46:07 +0000 Subject: [PATCH 1/4] feat!: add summary to `coder ping` --- cli/cliui/agent.go | 9 +- cli/cliui/agent_test.go | 26 ---- cli/cliui/table.go | 4 + cli/ping.go | 181 +++++++++++++++++++------- cli/ping_internal_test.go | 76 +++++++++++ cli/ping_test.go | 2 +- cli/testdata/coder_ping_--help.golden | 5 +- docs/reference/cli/ping.md | 9 +- 8 files changed, 224 insertions(+), 88 deletions(-) create mode 100644 cli/ping_internal_test.go diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 211074ff74231..96774ccde8632 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -370,6 +370,9 @@ func (d ConnDiags) Write(w io.Writer) { for _, msg := range general { _, _ = fmt.Fprintln(w, msg) } + if len(general) > 0 { + _, _ = fmt.Fprintln(w, "") + } if len(client) > 0 { _, _ = fmt.Fprint(w, "Possible client-side issues with direct connection:\n\n") for _, msg := range client { @@ -385,12 +388,6 @@ func (d ConnDiags) Write(w io.Writer) { } func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { - if d.PingP2P { - general = append(general, "✔ You are connected directly (p2p)") - } else { - general = append(general, "❗ You are connected via a DERP relay, not directly (p2p)") - } - if d.AgentNetcheck != nil { for _, msg := range d.AgentNetcheck.Interfaces.Warnings { agent = append(agent, msg.Message) diff --git a/cli/cliui/agent_test.go b/cli/cliui/agent_test.go index 31442ae0224da..966d53578780a 100644 --- a/cli/cliui/agent_test.go +++ b/cli/cliui/agent_test.go @@ -683,19 +683,6 @@ func TestConnDiagnostics(t *testing.T) { diags cliui.ConnDiags want []string }{ - { - name: "Direct", - diags: cliui.ConnDiags{ - ConnInfo: workspacesdk.AgentConnectionInfo{ - DERPMap: &tailcfg.DERPMap{}, - }, - PingP2P: true, - LocalNetInfo: &tailcfg.NetInfo{}, - }, - want: []string{ - `✔ You are connected directly (p2p)`, - }, - }, { name: "DirectBlocked", diags: cliui.ConnDiags{ @@ -705,7 +692,6 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `❗ Your Coder administrator has blocked direct connections`, }, }, @@ -718,7 +704,6 @@ func TestConnDiagnostics(t *testing.T) { LocalNetInfo: &tailcfg.NetInfo{}, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `The DERP map is not configured to use STUN`, }, }, @@ -743,7 +728,6 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `Client could not connect to STUN over UDP`, }, }, @@ -770,7 +754,6 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `Agent could not connect to STUN over UDP`, }, }, @@ -785,7 +768,6 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers`, }, }, @@ -795,14 +777,12 @@ func TestConnDiagnostics(t *testing.T) { ConnInfo: workspacesdk.AgentConnectionInfo{ DERPMap: &tailcfg.DERPMap{}, }, - PingP2P: false, LocalNetInfo: &tailcfg.NetInfo{}, AgentNetcheck: &healthsdk.AgentNetcheckReport{ NetInfo: &tailcfg.NetInfo{MappingVariesByDestIP: "true"}, }, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers`, }, }, @@ -812,7 +792,6 @@ func TestConnDiagnostics(t *testing.T) { ConnInfo: workspacesdk.AgentConnectionInfo{ DERPMap: &tailcfg.DERPMap{}, }, - PingP2P: true, AgentNetcheck: &healthsdk.AgentNetcheckReport{ Interfaces: healthsdk.InterfacesReport{ BaseReport: healthsdk.BaseReport{ @@ -824,7 +803,6 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `✔ You are connected directly (p2p)`, `Network interface eth0 has MTU 1280, (less than 1378), which may degrade the quality of direct connections`, }, }, @@ -834,7 +812,6 @@ func TestConnDiagnostics(t *testing.T) { ConnInfo: workspacesdk.AgentConnectionInfo{ DERPMap: &tailcfg.DERPMap{}, }, - PingP2P: true, LocalInterfaces: &healthsdk.InterfacesReport{ BaseReport: healthsdk.BaseReport{ Warnings: []health.Message{ @@ -844,7 +821,6 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `✔ You are connected directly (p2p)`, `Network interface eth1 has MTU 1310, (less than 1378), which may degrade the quality of direct connections`, }, }, @@ -858,7 +834,6 @@ func TestConnDiagnostics(t *testing.T) { AgentIPIsAWS: false, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `Client IP address is within an AWS range (AWS uses hard NAT)`, }, }, @@ -872,7 +847,6 @@ func TestConnDiagnostics(t *testing.T) { AgentIPIsAWS: true, }, want: []string{ - `❗ You are connected via a DERP relay, not directly (p2p)`, `Agent IP address is within an AWS range (AWS uses hard NAT)`, }, }, diff --git a/cli/cliui/table.go b/cli/cliui/table.go index c9f3ee69936b4..d113b97c2dc72 100644 --- a/cli/cliui/table.go +++ b/cli/cliui/table.go @@ -199,6 +199,10 @@ func renderTable(out any, sort string, headers table.Row, filterColumns []string if val != nil { v = *val } + case *time.Duration: + if val != nil { + v = val.String() + } case fmt.Stringer: if val != nil { v = val.String() diff --git a/cli/ping.go b/cli/ping.go index 4ce9cd5373bd2..dde3b983cbec3 100644 --- a/cli/ping.go +++ b/cli/ping.go @@ -4,11 +4,15 @@ import ( "context" "errors" "fmt" + "io" + "math" "net/http" "net/netip" + "strings" "time" "golang.org/x/xerrors" + "tailscale.com/ipn/ipnstate" "tailscale.com/tailcfg" "cdr.dev/slog" @@ -18,12 +22,71 @@ import ( "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/cli/cliutil" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/healthsdk" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/serpent" ) +type pingSummary struct { + Workspace string `table:"workspace,nosort"` + Total int `table:"total"` + Successful int `table:"successful"` + Min *time.Duration `table:"min"` + Avg *time.Duration `table:"avg"` + Max *time.Duration `table:"max"` + StdDev *time.Duration `table:"stddev"` +} + +func buildSummary(wsname string, r []*ipnstate.PingResult) *pingSummary { + out := pingSummary{ + Workspace: wsname, + } + totalLatency := 0.0 + latencies := make([]float64, 0, len(r)) + for _, pong := range r { + out.Total++ + if pong.Err == "" { + out.Successful++ + latencies = append(latencies, pong.LatencySeconds) + totalLatency += pong.LatencySeconds + } + } + if out.Successful > 0 { + minLatency := latencies[0] + maxLatency := latencies[0] + varianceSum := 0.0 + avgLatency := totalLatency / float64(out.Successful) + for _, l := range latencies { + if l < minLatency { + minLatency = l + } + if l > maxLatency { + maxLatency = l + } + varianceSum += (l - avgLatency) * (l - avgLatency) + } + stddev := math.Sqrt(varianceSum / float64(out.Successful)) + out.StdDev = ptr.Ref(time.Duration(stddev * float64(time.Second))) + out.Avg = ptr.Ref(time.Duration(avgLatency * float64(time.Second))) + out.Max = ptr.Ref(time.Duration(maxLatency * float64(time.Second))) + out.Min = ptr.Ref(time.Duration(minLatency * float64(time.Second))) + } + return &out +} + +func (p *pingSummary) Write(w io.Writer) { + out, err := cliui.DisplayTable([]*pingSummary{p}, "", nil) + if err != nil { + _, _ = fmt.Fprintf(w, "Failed to display ping summary: %v\n", err) + return + } + width := len(strings.Split(out, "\n")[0]) + _, _ = fmt.Println(strings.Repeat("-", width)) + _, _ = fmt.Fprint(w, out) +} + func (r *RootCmd) ping() *serpent.Command { var ( pingNum int64 @@ -46,6 +109,9 @@ func (r *RootCmd) ping() *serpent.Command { ctx, cancel := context.WithCancel(inv.Context()) defer cancel() + notifyCtx, notifyCancel := inv.SignalNotifyContext(ctx, StopSignals...) + defer notifyCancel() + workspaceName := inv.Args[0] _, workspaceAgent, err := getWorkspaceAndAgent( ctx, inv, client, @@ -77,11 +143,62 @@ func (r *RootCmd) ping() *serpent.Command { defer conn.Close() derpMap := conn.DERPMap() - _ = derpMap + + diagCtx, diagCancel := context.WithTimeout(inv.Context(), 30*time.Second) + defer diagCancel() + diags := conn.GetPeerDiagnostics() + cliui.PeerDiagnostics(inv.Stdout, diags) + + // Silent ping to determine whether we should show diags + _, didP2p, _, _ := conn.Ping(ctx) + + ni := conn.GetNetInfo() + connDiags := cliui.ConnDiags{ + DisableDirect: r.disableDirect, + LocalNetInfo: ni, + Verbose: r.verbose, + PingP2P: didP2p, + TroubleshootingURL: appearanceConfig.DocsURL + "/networking/troubleshooting", + } + + awsRanges, err := cliutil.FetchAWSIPRanges(diagCtx, cliutil.AWSIPRangesURL) + if err != nil { + opts.Logger.Debug(inv.Context(), "failed to retrieve AWS IP ranges", slog.Error(err)) + } + + connDiags.ClientIPIsAWS = isAWSIP(awsRanges, ni) + + connInfo, err := wsClient.AgentConnectionInfoGeneric(diagCtx) + if err != nil || connInfo.DERPMap == nil { + return xerrors.Errorf("Failed to retrieve connection info from server: %w\n", err) + } + connDiags.ConnInfo = connInfo + ifReport, err := healthsdk.RunInterfacesReport() + if err == nil { + connDiags.LocalInterfaces = &ifReport + } else { + _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve local interfaces report: %v\n", err) + } + + agentNetcheck, err := conn.Netcheck(diagCtx) + if err == nil { + connDiags.AgentNetcheck = &agentNetcheck + connDiags.AgentIPIsAWS = isAWSIP(awsRanges, agentNetcheck.NetInfo) + } else { + var sdkErr *codersdk.Error + if errors.As(err, &sdkErr) && sdkErr.StatusCode() == http.StatusNotFound { + _, _ = fmt.Fprint(inv.Stdout, "Could not generate full connection report as the workspace agent is outdated\n") + } else { + _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve connection report from agent: %v\n", err) + } + } + + connDiags.Write(inv.Stdout) n := 0 - didP2p := false start := time.Now() + results := make([]*ipnstate.PingResult, 0) + pingLoop: for { if n > 0 { time.Sleep(pingWait) @@ -91,6 +208,7 @@ func (r *RootCmd) ping() *serpent.Command { ctx, cancel := context.WithTimeout(ctx, pingTimeout) dur, p2p, pong, err := conn.Ping(ctx) cancel() + results = append(results, pong) if err != nil { if xerrors.Is(err, context.DeadlineExceeded) { _, _ = fmt.Fprintf(inv.Stdout, "ping to %q timed out \n", workspaceName) @@ -146,57 +264,25 @@ func (r *RootCmd) ping() *serpent.Command { pretty.Sprint(cliui.DefaultStyles.DateTimeStamp, dur.String()), ) - if n == int(pingNum) { - break + select { + case <-notifyCtx.Done(): + break pingLoop + default: + if n == int(pingNum) { + break pingLoop + } } } - diagCtx, diagCancel := context.WithTimeout(inv.Context(), 30*time.Second) - defer diagCancel() - diags := conn.GetPeerDiagnostics() - cliui.PeerDiagnostics(inv.Stdout, diags) - - ni := conn.GetNetInfo() - connDiags := cliui.ConnDiags{ - PingP2P: didP2p, - DisableDirect: r.disableDirect, - LocalNetInfo: ni, - Verbose: r.verbose, - TroubleshootingURL: appearanceConfig.DocsURL + "/networking/troubleshooting", - } - - awsRanges, err := cliutil.FetchAWSIPRanges(diagCtx, cliutil.AWSIPRangesURL) - if err != nil { - opts.Logger.Debug(inv.Context(), "failed to retrieve AWS IP ranges", slog.Error(err)) - } - connDiags.ClientIPIsAWS = isAWSIP(awsRanges, ni) - - connInfo, err := wsClient.AgentConnectionInfoGeneric(diagCtx) - if err != nil || connInfo.DERPMap == nil { - return xerrors.Errorf("Failed to retrieve connection info from server: %w\n", err) - } - connDiags.ConnInfo = connInfo - ifReport, err := healthsdk.RunInterfacesReport() - if err == nil { - connDiags.LocalInterfaces = &ifReport + if didP2p { + _, _ = fmt.Fprintf(inv.Stdout, "✔ You are connected directly (p2p)\n") } else { - _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve local interfaces report: %v\n", err) + _, _ = fmt.Fprintf(inv.Stdout, "❗ You are connected via a DERP relay, not directly (p2p)\n%s#common-problems-with-direct-connections\n", connDiags.TroubleshootingURL) } - agentNetcheck, err := conn.Netcheck(diagCtx) - if err == nil { - connDiags.AgentNetcheck = &agentNetcheck - connDiags.AgentIPIsAWS = isAWSIP(awsRanges, agentNetcheck.NetInfo) - } else { - var sdkErr *codersdk.Error - if errors.As(err, &sdkErr) && sdkErr.StatusCode() == http.StatusNotFound { - _, _ = fmt.Fprint(inv.Stdout, "Could not generate full connection report as the workspace agent is outdated\n") - } else { - _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve connection report from agent: %v\n", err) - } - } + summary := buildSummary(workspaceName, results) + summary.Write(inv.Stdout) - connDiags.Write(inv.Stdout) return nil }, } @@ -218,8 +304,7 @@ func (r *RootCmd) ping() *serpent.Command { { Flag: "num", FlagShorthand: "n", - Default: "10", - Description: "Specifies the number of pings to perform.", + Description: "Specifies the number of pings to perform. By default, pings will continue until interrupted.", Value: serpent.Int64Of(&pingNum), }, } diff --git a/cli/ping_internal_test.go b/cli/ping_internal_test.go new file mode 100644 index 0000000000000..9eb6c77d5e22f --- /dev/null +++ b/cli/ping_internal_test.go @@ -0,0 +1,76 @@ +package cli + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + "tailscale.com/ipn/ipnstate" + + "github.com/coder/coder/v2/coderd/util/ptr" +) + +func TestBuildSummary(t *testing.T) { + t.Parallel() + + t.Run("Ok", func(t *testing.T) { + t.Parallel() + input := []*ipnstate.PingResult{ + { + Err: "", + LatencySeconds: 0.1, + }, + { + Err: "", + LatencySeconds: 0.2, + }, + { + Err: "", + LatencySeconds: 0.3, + }, + { + Err: "ping error", + LatencySeconds: 0.4, + }, + } + + expected := &pingSummary{ + Workspace: "test", + Total: 4, + Successful: 3, + Min: ptr.Ref(time.Duration(0.1 * float64(time.Second))), + Avg: ptr.Ref(time.Duration(0.2 * float64(time.Second))), + Max: ptr.Ref(time.Duration(0.3 * float64(time.Second))), + StdDev: ptr.Ref(time.Duration(0.081649658 * float64(time.Second))), + } + + actual := buildSummary("test", input) + require.Equal(t, expected, actual) + }) + + t.Run("NoLatency", func(t *testing.T) { + t.Parallel() + input := []*ipnstate.PingResult{ + { + Err: "ping error", + }, + { + Err: "ping error", + LatencySeconds: 0.2, + }, + } + + expected := &pingSummary{ + Workspace: "test", + Total: 2, + Successful: 0, + Min: nil, + Avg: nil, + Max: nil, + StdDev: nil, + } + + actual := buildSummary("test", input) + require.Equal(t, expected, actual) + }) +} diff --git a/cli/ping_test.go b/cli/ping_test.go index 9e3225df28e29..5043806b0c3a1 100644 --- a/cli/ping_test.go +++ b/cli/ping_test.go @@ -65,8 +65,8 @@ func TestPing(t *testing.T) { assert.NoError(t, err) }) - pty.ExpectMatch("pong from " + workspace.Name) pty.ExpectMatch("✔ received remote agent data from Coder networking coordinator") + pty.ExpectMatch("pong from " + workspace.Name) pty.ExpectMatch("You are connected") cancel() <-cmdDone diff --git a/cli/testdata/coder_ping_--help.golden b/cli/testdata/coder_ping_--help.golden index 9410f272bdb91..4955e889c3651 100644 --- a/cli/testdata/coder_ping_--help.golden +++ b/cli/testdata/coder_ping_--help.golden @@ -6,8 +6,9 @@ USAGE: Ping a workspace OPTIONS: - -n, --num int (default: 10) - Specifies the number of pings to perform. + -n, --num int + Specifies the number of pings to perform. By default, pings will + continue until interrupted. -t, --timeout duration (default: 5s) Specifies how long to wait for a ping to complete. diff --git a/docs/reference/cli/ping.md b/docs/reference/cli/ping.md index e279bd264037b..c8d63addcf8d7 100644 --- a/docs/reference/cli/ping.md +++ b/docs/reference/cli/ping.md @@ -32,9 +32,8 @@ Specifies how long to wait for a ping to complete. ### -n, --num -| | | -| ------- | ---------------- | -| Type | int | -| Default | 10 | +| | | +| ---- | ---------------- | +| Type | int | -Specifies the number of pings to perform. +Specifies the number of pings to perform. By default, pings will continue until interrupted. From 13517170924a9f4ddf783d835008e73e33682fd7 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 24 Sep 2024 12:49:16 +0000 Subject: [PATCH 2/4] single-pass variance --- cli/cliui/agent.go | 7 ++++ cli/ping.go | 79 ++++++++++++++++----------------------- cli/ping_internal_test.go | 52 +++++++++++++++++++------- cli/ping_test.go | 2 +- 4 files changed, 78 insertions(+), 62 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 96774ccde8632..9a0290a1728c0 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -406,6 +406,12 @@ func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { } } + if d.PingP2P { + general = append(general, "✔ You are connected directly (p2p)") + } else { + general = append(general, fmt.Sprintf("❗ You are connected via a DERP relay, not directly (p2p)\n%s#common-problems-with-direct-connections", d.TroubleshootingURL)) + } + if d.PingP2P && !d.Verbose { return general, client, agent } @@ -458,5 +464,6 @@ func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { agent = append(agent, fmt.Sprintf("Agent IP address is within an AWS range (AWS uses hard NAT)\n %s#endpoint-dependent-nat-hard-nat", d.TroubleshootingURL)) } + return general, client, agent } diff --git a/cli/ping.go b/cli/ping.go index dde3b983cbec3..b14497e1ec867 100644 --- a/cli/ping.go +++ b/cli/ping.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "math" "net/http" "net/netip" "strings" @@ -36,48 +35,42 @@ type pingSummary struct { Min *time.Duration `table:"min"` Avg *time.Duration `table:"avg"` Max *time.Duration `table:"max"` - StdDev *time.Duration `table:"stddev"` + Variance *time.Duration `table:"variance"` + latencySum float64 + runningAvg float64 + m2 float64 } -func buildSummary(wsname string, r []*ipnstate.PingResult) *pingSummary { - out := pingSummary{ - Workspace: wsname, +func (s *pingSummary) addResult(r *ipnstate.PingResult) { + s.Total++ + if r == nil || r.Err != "" { + return } - totalLatency := 0.0 - latencies := make([]float64, 0, len(r)) - for _, pong := range r { - out.Total++ - if pong.Err == "" { - out.Successful++ - latencies = append(latencies, pong.LatencySeconds) - totalLatency += pong.LatencySeconds - } + s.Successful++ + if s.Min == nil || r.LatencySeconds < s.Min.Seconds() { + s.Min = ptr.Ref(time.Duration(r.LatencySeconds * float64(time.Second))) } - if out.Successful > 0 { - minLatency := latencies[0] - maxLatency := latencies[0] - varianceSum := 0.0 - avgLatency := totalLatency / float64(out.Successful) - for _, l := range latencies { - if l < minLatency { - minLatency = l - } - if l > maxLatency { - maxLatency = l - } - varianceSum += (l - avgLatency) * (l - avgLatency) - } - stddev := math.Sqrt(varianceSum / float64(out.Successful)) - out.StdDev = ptr.Ref(time.Duration(stddev * float64(time.Second))) - out.Avg = ptr.Ref(time.Duration(avgLatency * float64(time.Second))) - out.Max = ptr.Ref(time.Duration(maxLatency * float64(time.Second))) - out.Min = ptr.Ref(time.Duration(minLatency * float64(time.Second))) + if s.Max == nil || r.LatencySeconds > s.Min.Seconds() { + s.Max = ptr.Ref(time.Duration(r.LatencySeconds * float64(time.Second))) } - return &out + s.latencySum += r.LatencySeconds + + d := r.LatencySeconds - s.runningAvg + s.runningAvg += d / float64(s.Successful) + d2 := r.LatencySeconds - s.runningAvg + s.m2 += d * d2 } -func (p *pingSummary) Write(w io.Writer) { - out, err := cliui.DisplayTable([]*pingSummary{p}, "", nil) +// Write finalizes the summary and writes it +func (s *pingSummary) Write(wsname string, w io.Writer) { + s.Workspace = wsname + if s.Successful > 0 { + s.Avg = ptr.Ref(time.Duration(s.latencySum / float64(s.Successful) * float64(time.Second))) + } + if s.Successful > 1 { + s.Variance = ptr.Ref(time.Duration((s.m2 / float64(s.Successful-1)) * float64(time.Second))) + } + out, err := cliui.DisplayTable([]*pingSummary{s}, "", nil) if err != nil { _, _ = fmt.Fprintf(w, "Failed to display ping summary: %v\n", err) return @@ -194,10 +187,9 @@ func (r *RootCmd) ping() *serpent.Command { } connDiags.Write(inv.Stdout) - + results := &pingSummary{} n := 0 start := time.Now() - results := make([]*ipnstate.PingResult, 0) pingLoop: for { if n > 0 { @@ -208,7 +200,7 @@ func (r *RootCmd) ping() *serpent.Command { ctx, cancel := context.WithTimeout(ctx, pingTimeout) dur, p2p, pong, err := conn.Ping(ctx) cancel() - results = append(results, pong) + results.addResult(pong) if err != nil { if xerrors.Is(err, context.DeadlineExceeded) { _, _ = fmt.Fprintf(inv.Stdout, "ping to %q timed out \n", workspaceName) @@ -274,14 +266,7 @@ func (r *RootCmd) ping() *serpent.Command { } } - if didP2p { - _, _ = fmt.Fprintf(inv.Stdout, "✔ You are connected directly (p2p)\n") - } else { - _, _ = fmt.Fprintf(inv.Stdout, "❗ You are connected via a DERP relay, not directly (p2p)\n%s#common-problems-with-direct-connections\n", connDiags.TroubleshootingURL) - } - - summary := buildSummary(workspaceName, results) - summary.Write(inv.Stdout) + results.Write(workspaceName, inv.Stdout) return nil }, diff --git a/cli/ping_internal_test.go b/cli/ping_internal_test.go index 9eb6c77d5e22f..d8372dc311847 100644 --- a/cli/ping_internal_test.go +++ b/cli/ping_internal_test.go @@ -1,13 +1,12 @@ package cli import ( + "io" "testing" "time" "github.com/stretchr/testify/require" "tailscale.com/ipn/ipnstate" - - "github.com/coder/coder/v2/coderd/util/ptr" ) func TestBuildSummary(t *testing.T) { @@ -34,18 +33,36 @@ func TestBuildSummary(t *testing.T) { }, } - expected := &pingSummary{ - Workspace: "test", - Total: 4, - Successful: 3, - Min: ptr.Ref(time.Duration(0.1 * float64(time.Second))), - Avg: ptr.Ref(time.Duration(0.2 * float64(time.Second))), - Max: ptr.Ref(time.Duration(0.3 * float64(time.Second))), - StdDev: ptr.Ref(time.Duration(0.081649658 * float64(time.Second))), + actual := pingSummary{} + for _, r := range input { + actual.addResult(r) } + actual.Write("test", io.Discard) + require.Equal(t, time.Duration(0.1*float64(time.Second)), *actual.Min) + require.Equal(t, time.Duration(0.2*float64(time.Second)), *actual.Avg) + require.Equal(t, time.Duration(0.3*float64(time.Second)), *actual.Max) + require.Equal(t, time.Duration(0.009999999*float64(time.Second)), *actual.Variance) + require.Equal(t, actual.Successful, 3) + }) - actual := buildSummary("test", input) - require.Equal(t, expected, actual) + t.Run("One", func(t *testing.T) { + t.Parallel() + input := []*ipnstate.PingResult{ + { + LatencySeconds: 0.2, + }, + } + + actual := &pingSummary{} + for _, r := range input { + actual.addResult(r) + } + actual.Write("test", io.Discard) + require.Equal(t, actual.Successful, 1) + require.Equal(t, time.Duration(0.2*float64(time.Second)), *actual.Min) + require.Equal(t, time.Duration(0.2*float64(time.Second)), *actual.Avg) + require.Equal(t, time.Duration(0.2*float64(time.Second)), *actual.Max) + require.Nil(t, actual.Variance) }) t.Run("NoLatency", func(t *testing.T) { @@ -67,10 +84,17 @@ func TestBuildSummary(t *testing.T) { Min: nil, Avg: nil, Max: nil, - StdDev: nil, + Variance: nil, + latencySum: 0, + runningAvg: 0, + m2: 0, } - actual := buildSummary("test", input) + actual := &pingSummary{} + for _, r := range input { + actual.addResult(r) + } + actual.Write("test", io.Discard) require.Equal(t, expected, actual) }) } diff --git a/cli/ping_test.go b/cli/ping_test.go index 5043806b0c3a1..3cbac79f0e4f6 100644 --- a/cli/ping_test.go +++ b/cli/ping_test.go @@ -66,8 +66,8 @@ func TestPing(t *testing.T) { }) pty.ExpectMatch("✔ received remote agent data from Coder networking coordinator") - pty.ExpectMatch("pong from " + workspace.Name) pty.ExpectMatch("You are connected") + pty.ExpectMatch("pong from " + workspace.Name) cancel() <-cmdDone }) From 0efd6ed5e4820e5047235d6ac918057274945f0b Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 24 Sep 2024 14:47:08 +0000 Subject: [PATCH 3/4] write diags to stderr --- cli/cliui/agent.go | 6 ------ cli/ping.go | 19 +++++++++++++------ cli/ping_internal_test.go | 18 ++++++++++++------ cli/ping_test.go | 2 -- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 9a0290a1728c0..dbc73fb13e663 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -406,12 +406,6 @@ func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { } } - if d.PingP2P { - general = append(general, "✔ You are connected directly (p2p)") - } else { - general = append(general, fmt.Sprintf("❗ You are connected via a DERP relay, not directly (p2p)\n%s#common-problems-with-direct-connections", d.TroubleshootingURL)) - } - if d.PingP2P && !d.Verbose { return general, client, agent } diff --git a/cli/ping.go b/cli/ping.go index b14497e1ec867..6517bf262f308 100644 --- a/cli/ping.go +++ b/cli/ping.go @@ -62,8 +62,7 @@ func (s *pingSummary) addResult(r *ipnstate.PingResult) { } // Write finalizes the summary and writes it -func (s *pingSummary) Write(wsname string, w io.Writer) { - s.Workspace = wsname +func (s *pingSummary) Write(w io.Writer) { if s.Successful > 0 { s.Avg = ptr.Ref(time.Duration(s.latencySum / float64(s.Successful) * float64(time.Second))) } @@ -140,7 +139,7 @@ func (r *RootCmd) ping() *serpent.Command { diagCtx, diagCancel := context.WithTimeout(inv.Context(), 30*time.Second) defer diagCancel() diags := conn.GetPeerDiagnostics() - cliui.PeerDiagnostics(inv.Stdout, diags) + cliui.PeerDiagnostics(inv.Stderr, diags) // Silent ping to determine whether we should show diags _, didP2p, _, _ := conn.Ping(ctx) @@ -186,8 +185,10 @@ func (r *RootCmd) ping() *serpent.Command { } } - connDiags.Write(inv.Stdout) - results := &pingSummary{} + connDiags.Write(inv.Stderr) + results := &pingSummary{ + Workspace: workspaceName, + } n := 0 start := time.Now() pingLoop: @@ -266,7 +267,13 @@ func (r *RootCmd) ping() *serpent.Command { } } - results.Write(workspaceName, inv.Stdout) + if didP2p { + _, _ = fmt.Fprintf(inv.Stderr, "✔ You are connected directly (p2p)\n") + } else { + _, _ = fmt.Fprintf(inv.Stderr, "❗ You are connected via a DERP relay, not directly (p2p)\n%s#common-problems-with-direct-connections\n", connDiags.TroubleshootingURL) + } + + results.Write(inv.Stdout) return nil }, diff --git a/cli/ping_internal_test.go b/cli/ping_internal_test.go index d8372dc311847..0c131fadfa52a 100644 --- a/cli/ping_internal_test.go +++ b/cli/ping_internal_test.go @@ -33,11 +33,13 @@ func TestBuildSummary(t *testing.T) { }, } - actual := pingSummary{} + actual := pingSummary{ + Workspace: "test", + } for _, r := range input { actual.addResult(r) } - actual.Write("test", io.Discard) + actual.Write(io.Discard) require.Equal(t, time.Duration(0.1*float64(time.Second)), *actual.Min) require.Equal(t, time.Duration(0.2*float64(time.Second)), *actual.Avg) require.Equal(t, time.Duration(0.3*float64(time.Second)), *actual.Max) @@ -53,11 +55,13 @@ func TestBuildSummary(t *testing.T) { }, } - actual := &pingSummary{} + actual := &pingSummary{ + Workspace: "test", + } for _, r := range input { actual.addResult(r) } - actual.Write("test", io.Discard) + actual.Write(io.Discard) require.Equal(t, actual.Successful, 1) require.Equal(t, time.Duration(0.2*float64(time.Second)), *actual.Min) require.Equal(t, time.Duration(0.2*float64(time.Second)), *actual.Avg) @@ -90,11 +94,13 @@ func TestBuildSummary(t *testing.T) { m2: 0, } - actual := &pingSummary{} + actual := &pingSummary{ + Workspace: "test", + } for _, r := range input { actual.addResult(r) } - actual.Write("test", io.Discard) + actual.Write(io.Discard) require.Equal(t, expected, actual) }) } diff --git a/cli/ping_test.go b/cli/ping_test.go index 3cbac79f0e4f6..bc0bb7c0e423a 100644 --- a/cli/ping_test.go +++ b/cli/ping_test.go @@ -65,8 +65,6 @@ func TestPing(t *testing.T) { assert.NoError(t, err) }) - pty.ExpectMatch("✔ received remote agent data from Coder networking coordinator") - pty.ExpectMatch("You are connected") pty.ExpectMatch("pong from " + workspace.Name) cancel() <-cmdDone From d5bdff0c6fc23b1f6c814e12132eb99dc40fa710 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 24 Sep 2024 15:25:24 +0000 Subject: [PATCH 4/4] add spinner --- cli/ping.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cli/ping.go b/cli/ping.go index 6517bf262f308..0423416f040cb 100644 --- a/cli/ping.go +++ b/cli/ping.go @@ -17,6 +17,8 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" + "github.com/briandowns/spinner" + "github.com/coder/pretty" "github.com/coder/coder/v2/cli/cliui" @@ -101,6 +103,11 @@ func (r *RootCmd) ping() *serpent.Command { ctx, cancel := context.WithCancel(inv.Context()) defer cancel() + spin := spinner.New(spinner.CharSets[5], 100*time.Millisecond) + spin.Writer = inv.Stderr + spin.Suffix = pretty.Sprint(cliui.DefaultStyles.Keyword, " Collecting diagnostics...") + spin.Start() + notifyCtx, notifyCancel := inv.SignalNotifyContext(ctx, StopSignals...) defer notifyCancel() @@ -139,7 +146,6 @@ func (r *RootCmd) ping() *serpent.Command { diagCtx, diagCancel := context.WithTimeout(inv.Context(), 30*time.Second) defer diagCancel() diags := conn.GetPeerDiagnostics() - cliui.PeerDiagnostics(inv.Stderr, diags) // Silent ping to determine whether we should show diags _, didP2p, _, _ := conn.Ping(ctx) @@ -185,6 +191,8 @@ func (r *RootCmd) ping() *serpent.Command { } } + spin.Stop() + cliui.PeerDiagnostics(inv.Stderr, diags) connDiags.Write(inv.Stderr) results := &pingSummary{ Workspace: workspaceName,