review

coder · ethanndickson · Jul 3, 2024 · Jun 26, 2024 · Jun 26, 2024 · Jun 26, 2024
commit 67898fdb2de4ddb464d62bab5572f5ff28fc29c9
diff --git a/cli/ping.go b/cli/ping.go
@@ -58,7 +58,7 @@ func (r *RootCmd) ping() *serpent.Command {
 				_, _ = fmt.Fprintln(inv.Stderr, "Direct connections disabled.")
 				opts.BlockEndpoints = true
 			}
-			if !r.noNetworkTelemetry {
+			if !r.disableNetworkTelemetry {
 				opts.EnableTelemetry = true
 			}
 			conn, err := workspacesdk.New(client).DialAgent(ctx, workspaceAgent.ID, opts)

diff --git a/cli/portforward.go b/cli/portforward.go
@@ -106,7 +106,7 @@ func (r *RootCmd) portForward() *serpent.Command {
 				_, _ = fmt.Fprintln(inv.Stderr, "Direct connections disabled.")
 				opts.BlockEndpoints = true
 			}
-			if !r.noNetworkTelemetry {
+			if !r.disableNetworkTelemetry {
 				opts.EnableTelemetry = true
 			}
 			conn, err := workspacesdk.New(client).DialAgent(ctx, workspaceAgent.ID, opts)

diff --git a/cli/root.go b/cli/root.go
@@ -439,8 +439,8 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
 		{
 			Flag:        varDisableNetworkTelemetry,
 			Env:         "CODER_DISABLE_NETWORK_TELEMETRY",
-			Description: "Disable network telemetry.",
-			Value:       serpent.BoolOf(&r.noNetworkTelemetry),
+			Description: "Disable network telemetry. Network telemetry is collected when connecting to workspaces using the CLI, and is forwarded to the server. If telemetry is also enabled on the server, it may be sent to Coder. Network telemetry is used to measure network quality and detect regressions.",
+			Value:       serpent.BoolOf(&r.disableNetworkTelemetry),
 			Group:       globalGroup,
 		},
 		{
@@ -489,9 +489,9 @@ type RootCmd struct {
 	disableDirect  bool
 	debugHTTP      bool
 
-	noNetworkTelemetry bool
-	noVersionCheck     bool
-	noFeatureWarning   bool
+	disableNetworkTelemetry bool
+	noVersionCheck          bool
+	noFeatureWarning        bool
 }
 
 // InitClient authenticates the client with files from disk

diff --git a/cli/speedtest.go b/cli/speedtest.go
@@ -102,7 +102,7 @@ func (r *RootCmd) speedtest() *serpent.Command {
 				_, _ = fmt.Fprintln(inv.Stderr, "Direct connections disabled.")
 				opts.BlockEndpoints = true
 			}
-			if !r.noNetworkTelemetry {
+			if !r.disableNetworkTelemetry {
 				opts.EnableTelemetry = true
 			}
 			if pcapFile != "" {

diff --git a/cli/ssh.go b/cli/ssh.go
@@ -245,7 +245,7 @@ func (r *RootCmd) ssh() *serpent.Command {
 				DialAgent(ctx, workspaceAgent.ID, &workspacesdk.DialAgentOptions{
 					Logger:          logger,
 					BlockEndpoints:  r.disableDirect,
-					EnableTelemetry: !r.noNetworkTelemetry,
+					EnableTelemetry: !r.disableNetworkTelemetry,
 				})
 			if err != nil {
 				return xerrors.Errorf("dial agent: %w", err)

diff --git a/cli/testdata/coder_--help.golden b/cli/testdata/coder_--help.golden
@@ -67,7 +67,11 @@ variables or flags.
           Disable direct (P2P) connections to workspaces.
 
       --disable-network-telemetry bool, $CODER_DISABLE_NETWORK_TELEMETRY
-          Disable network telemetry.
+          Disable network telemetry. Network telemetry is collected when
+          connecting to workspaces using the CLI, and is forwarded to the
+          server. If telemetry is also enabled on the server, it may be sent to
+          Coder. Network telemetry is used to measure network quality and detect
+          regressions.
 
       --global-config string, $CODER_CONFIG_DIR (default: ~/.config/coderv2)
           Path to the global `coder` config directory.

diff --git a/codersdk/workspacesdk/connector_internal_test.go b/codersdk/workspacesdk/connector_internal_test.go
@@ -231,10 +231,13 @@ func TestTailnetAPIConnector_TelemetryUnimplemented(t *testing.T) {
 	fakeDRPCClient.telemeteryErorr = drpcerr.WithCode(xerrors.New("Unimplemented"), 0)
 	uut.SendTelemetryEvent(&proto.TelemetryEvent{})
 	require.False(t, uut.telemetryUnavailable.Load())
+	require.Equal(t, int64(1), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
 
 	fakeDRPCClient.telemeteryErorr = drpcerr.WithCode(xerrors.New("Unimplemented"), drpcerr.Unimplemented)
 	uut.SendTelemetryEvent(&proto.TelemetryEvent{})
 	require.True(t, uut.telemetryUnavailable.Load())
+	uut.SendTelemetryEvent(&proto.TelemetryEvent{})
+	require.Equal(t, int64(2), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
 }
 
 func TestTailnetAPIConnector_TelemetryNotRecognised(t *testing.T) {
@@ -268,10 +271,13 @@ func TestTailnetAPIConnector_TelemetryNotRecognised(t *testing.T) {
 	fakeDRPCClient.telemeteryErorr = drpc.ProtocolError.New("Protocol Error")
 	uut.SendTelemetryEvent(&proto.TelemetryEvent{})
 	require.False(t, uut.telemetryUnavailable.Load())
+	require.Equal(t, int64(1), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
 
 	fakeDRPCClient.telemeteryErorr = drpc.ProtocolError.New("unknown rpc: /coder.tailnet.v2.Tailnet/PostTelemetry")
 	uut.SendTelemetryEvent(&proto.TelemetryEvent{})
 	require.True(t, uut.telemetryUnavailable.Load())
+	uut.SendTelemetryEvent(&proto.TelemetryEvent{})
+	require.Equal(t, int64(2), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
 }
 
 type fakeTailnetConn struct{}
@@ -294,14 +300,16 @@ func newFakeTailnetConn() *fakeTailnetConn {
 }
 
 type fakeDRPCClient struct {
-	telemeteryErorr error
+	postTelemetryCalls int64
+	telemeteryErorr    error
 	fakeDRPPCMapStream
 }
 
 var _ proto.DRPCTailnetClient = &fakeDRPCClient{}
 
 func newFakeDRPCClient() *fakeDRPCClient {
 	return &fakeDRPCClient{
+		postTelemetryCalls: 0,
 		fakeDRPPCMapStream: fakeDRPPCMapStream{
 			fakeDRPCStream: fakeDRPCStream{
 				ch: make(chan struct{}),
@@ -322,6 +330,7 @@ func (*fakeDRPCClient) DRPCConn() drpc.Conn {
 
 // PostTelemetry implements proto.DRPCTailnetClient.
 func (f *fakeDRPCClient) PostTelemetry(_ context.Context, _ *proto.TelemetryRequest) (*proto.TelemetryResponse, error) {
+	atomic.AddInt64(&f.postTelemetryCalls, 1)
 	return nil, f.telemeteryErorr
 }
 

diff --git a/docs/cli.md b/docs/cli.md
@@ -156,7 +156,10 @@ Disable direct (P2P) connections to workspaces.
 | Type        | <code>bool</code>                             |
 | Environment | <code>$CODER_DISABLE_NETWORK_TELEMETRY</code> |
 
-Disable network telemetry.
+Disable network telemetry. Network telemetry is collected when connecting to
+workspaces using the CLI, and is forwarded to the server. If telemetry is also
+enabled on the server, it may be sent to Coder. Network telemetry is used to
+measure network quality and detect regressions.
 
 ### --global-config
 

diff --git a/enterprise/cli/testdata/coder_--help.golden b/enterprise/cli/testdata/coder_--help.golden
@@ -31,7 +31,11 @@ variables or flags.
           Disable direct (P2P) connections to workspaces.
 
       --disable-network-telemetry bool, $CODER_DISABLE_NETWORK_TELEMETRY
-          Disable network telemetry.
+          Disable network telemetry. Network telemetry is collected when
+          connecting to workspaces using the CLI, and is forwarded to the
+          server. If telemetry is also enabled on the server, it may be sent to
+          Coder. Network telemetry is used to measure network quality and detect
+          regressions.
 
       --global-config string, $CODER_CONFIG_DIR (default: ~/.config/coderv2)
           Path to the global `coder` config directory.

diff --git a/tailnet/conn.go b/tailnet/conn.go
@@ -334,7 +334,7 @@ func IPFromUUID(uid uuid.UUID) netip.Addr {
 
 // Conn is an actively listening Wireguard connection.
 type Conn struct {
-	// ID must be unique to this connection
+	// Unique ID used for telemetry.
 	id     uuid.UUID
 	mutex  sync.Mutex
 	closed chan struct{}
@@ -443,7 +443,6 @@ func (c *Conn) Status() *ipnstate.Status {
 func (c *Conn) Ping(ctx context.Context, ip netip.Addr) (time.Duration, bool, *ipnstate.PingResult, error) {
 	dur, p2p, pr, err := c.pingWithType(ctx, ip, tailcfg.PingDisco)
 	if err == nil {
-		// TODO(ethanndickson): Is this too often?
 		c.sendPingTelemetry(pr)
 	}
 	return dur, p2p, pr, err
@@ -742,7 +741,7 @@ func (c *Conn) SendSpeedtestTelemetry(throughputMbits float64) {
 	}()
 }
 
-// nolint: revive
+// nolint:revive
 func (c *Conn) sendPingTelemetry(pr *ipnstate.PingResult) {
 	if c.telemetrySink == nil {
 		return
@@ -752,9 +751,6 @@ func (c *Conn) sendPingTelemetry(pr *ipnstate.PingResult) {
 	latency := durationpb.New(time.Duration(pr.LatencySeconds * float64(time.Second)))
 	if pr.Endpoint != "" {
 		e.P2PLatency = latency
-		// TODO(ethanndickson): This could be nil if we can't parse the endpoint
-		// But tailscale only ever sets the endpoint using `netip.AddrPort.String()`
-		// So it's infallible.
 		e.P2PEndpoint = c.telemeteryStore.toEndpoint(pr.Endpoint)
 	} else {
 		e.DerpLatency = latency

diff --git a/tailnet/telemetry.go b/tailnet/telemetry.go
@@ -17,7 +17,6 @@ import (
 	"github.com/coder/coder/v2/tailnet/proto"
 )
 
-// TODO(ethanndickson): How useful is the 'application' field?
 const (
 	TelemetryApplicationSSH       string = "ssh"
 	TelemetryApplicationSpeedtest string = "speedtest"
@@ -77,9 +76,9 @@ func (b *TelemetryStore) updateDerpMap(cur *tailcfg.DERPMap) {
 			n.IPv6 = ipv6
 			stunIP, _, _ := b.processIPLocked(n.STUNTestIP)
 			n.STUNTestIP = stunIP
-			hn := b.hashAddr(n.HostName)
+			hn := b.hashAddrorHostname(n.HostName)
 			n.HostName = hn
-			cn := b.hashAddr(n.CertName)
+			cn := b.hashAddrorHostname(n.CertName)
 			n.CertName = cn
 		}
 	}
@@ -140,7 +139,7 @@ func (b *TelemetryStore) toEndpoint(ipport string) *proto.TelemetryEvent_P2PEndp
 	}
 	addr := addrport.Addr()
 	fields := addrToFields(addr)
-	hashStr := b.hashAddr(addr.String())
+	hashStr := b.hashAddrorHostname(addr.String())
 	return &proto.TelemetryEvent_P2PEndpoint{
 		Hash:   hashStr,
 		Port:   int32(addrport.Port()),
@@ -159,11 +158,11 @@ func (b *TelemetryStore) processIPLocked(ip string) (string, *proto.IPFields, er
 	}
 
 	fields := addrToFields(addr)
-	hashStr := b.hashAddr(ip)
+	hashStr := b.hashAddrorHostname(ip)
 	return hashStr, fields, nil
 }
 
-func (b *TelemetryStore) hashAddr(addr string) string {
+func (b *TelemetryStore) hashAddrorHostname(addr string) string {
 	if hashStr, ok := b.hashCache[addr]; ok {
 		return hashStr
 	}

diff --git a/tailnet/telemetry_internal_test.go b/tailnet/telemetry_internal_test.go
@@ -68,16 +68,18 @@ func TestTelemetryStore(t *testing.T) {
 
 				event := telemetry.newEvent()
 
+				v4hash := telemetry.hashAddrorHostname(c.ipv4)
 				require.Equal(t, &proto.Netcheck_NetcheckIP{
-					Hash: telemetry.hashCache[c.ipv4],
+					Hash: v4hash,
 					Fields: &proto.IPFields{
 						Version: 4,
 						Class:   c.expectedClass,
 					},
 				}, event.LatestNetcheck.GlobalV4)
 
+				v6hash := telemetry.hashAddrorHostname(c.ipv6)
 				require.Equal(t, &proto.Netcheck_NetcheckIP{
-					Hash: telemetry.hashCache[c.ipv6],
+					Hash: v6hash,
 					Fields: &proto.IPFields{
 						Version: 6,
 						Class:   c.expectedClass,
@@ -95,9 +97,8 @@ func TestTelemetryStore(t *testing.T) {
 		derpMap := &tailcfg.DERPMap{
 			Regions: make(map[int]*tailcfg.DERPRegion),
 		}
-		// Add a region and node that uses every single field.
-		derpMap.Regions[999] = &tailcfg.DERPRegion{
-			RegionID:      999,
+		derpMap.Regions[998] = &tailcfg.DERPRegion{
+			RegionID:      998,
 			EmbeddedRelay: true,
 			RegionCode:    "zzz",
 			RegionName:    "Cool Region",
@@ -106,7 +107,24 @@ func TestTelemetryStore(t *testing.T) {
 			Nodes: []*tailcfg.DERPNode{
 				{
 					Name:       "zzz1",
-					RegionID:   999,
+					RegionID:   998,
+					HostName:   "coolderp.com",
+					CertName:   "coolderpcert",
+					IPv4:       "1.2.3.4",
+					IPv6:       "2001:db8::1",
+					STUNTestIP: "5.6.7.8",
+				},
+			},
+		}
+		derpMap.Regions[999] = &tailcfg.DERPRegion{
+			RegionID:      999,
+			EmbeddedRelay: true,
+			RegionCode:    "zzo",
+			RegionName:    "Other Cool Region",
+			Avoid:         true,
+			Nodes: []*tailcfg.DERPNode{
+				{
+					Name:       "zzo1",
 					HostName:   "coolderp.com",
 					CertName:   "coolderpcert",
 					IPv4:       "1.2.3.4",
@@ -124,5 +142,10 @@ func TestTelemetryStore(t *testing.T) {
 		require.NotContains(t, node.Ipv4, "1.2.3.4")
 		require.NotContains(t, node.Ipv6, "2001:db8::1")
 		require.NotContains(t, node.StunTestIp, "5.6.7.8")
+		otherNode := event.DerpMap.Regions[998].Nodes[0]
+		require.Equal(t, otherNode.HostName, node.HostName)
+		require.Equal(t, otherNode.Ipv4, node.Ipv4)
+		require.Equal(t, otherNode.Ipv6, node.Ipv6)
+		require.Equal(t, otherNode.StunTestIp, node.StunTestIp)
 	})
 }