Skip to content

Commit 67898fd

Browse files
ethanndicksonethanndickson
committed
review
1 parent c0684bb commit 67898fd

File tree

12 files changed

+69
-31
lines changed

12 files changed

+69
-31
lines changed

‎cli/ping.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (r *RootCmd) ping() *serpent.Command {
5858
_, _ = fmt.Fprintln(inv.Stderr, "Direct connections disabled.")
5959
opts.BlockEndpoints = true
6060
}
61-
if !r.noNetworkTelemetry {
61+
if !r.disableNetworkTelemetry {
6262
opts.EnableTelemetry = true
6363
}
6464
conn, err := workspacesdk.New(client).DialAgent(ctx, workspaceAgent.ID, opts)

‎cli/portforward.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func (r *RootCmd) portForward() *serpent.Command {
106106
_, _ = fmt.Fprintln(inv.Stderr, "Direct connections disabled.")
107107
opts.BlockEndpoints = true
108108
}
109-
if !r.noNetworkTelemetry {
109+
if !r.disableNetworkTelemetry {
110110
opts.EnableTelemetry = true
111111
}
112112
conn, err := workspacesdk.New(client).DialAgent(ctx, workspaceAgent.ID, opts)

‎cli/root.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -439,8 +439,8 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
439439
{
440440
Flag: varDisableNetworkTelemetry,
441441
Env: "CODER_DISABLE_NETWORK_TELEMETRY",
442-
Description: "Disable network telemetry.",
443-
Value: serpent.BoolOf(&r.noNetworkTelemetry),
442+
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.",
443+
Value: serpent.BoolOf(&r.disableNetworkTelemetry),
444444
Group: globalGroup,
445445
},
446446
{
@@ -489,9 +489,9 @@ type RootCmd struct {
489489
disableDirect bool
490490
debugHTTP bool
491491

492-
noNetworkTelemetry bool
493-
noVersionCheck bool
494-
noFeatureWarning bool
492+
disableNetworkTelemetry bool
493+
noVersionCheck bool
494+
noFeatureWarning bool
495495
}
496496

497497
// InitClient authenticates the client with files from disk

‎cli/speedtest.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ func (r *RootCmd) speedtest() *serpent.Command {
102102
_, _ = fmt.Fprintln(inv.Stderr, "Direct connections disabled.")
103103
opts.BlockEndpoints = true
104104
}
105-
if !r.noNetworkTelemetry {
105+
if !r.disableNetworkTelemetry {
106106
opts.EnableTelemetry = true
107107
}
108108
if pcapFile != "" {

‎cli/ssh.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ func (r *RootCmd) ssh() *serpent.Command {
245245
DialAgent(ctx, workspaceAgent.ID, &workspacesdk.DialAgentOptions{
246246
Logger: logger,
247247
BlockEndpoints: r.disableDirect,
248-
EnableTelemetry: !r.noNetworkTelemetry,
248+
EnableTelemetry: !r.disableNetworkTelemetry,
249249
})
250250
if err != nil {
251251
return xerrors.Errorf("dial agent: %w", err)

‎cli/testdata/coder_--help.golden

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,11 @@ variables or flags.
6767
Disable direct (P2P) connections to workspaces.
6868

6969
--disable-network-telemetry bool, $CODER_DISABLE_NETWORK_TELEMETRY
70-
Disable network telemetry.
70+
Disable network telemetry. Network telemetry is collected when
71+
connecting to workspaces using the CLI, and is forwarded to the
72+
server. If telemetry is also enabled on the server, it may be sent to
73+
Coder. Network telemetry is used to measure network quality and detect
74+
regressions.
7175

7276
--global-config string, $CODER_CONFIG_DIR (default: ~/.config/coderv2)
7377
Path to the global `coder` config directory.

‎codersdk/workspacesdk/connector_internal_test.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,10 +231,13 @@ func TestTailnetAPIConnector_TelemetryUnimplemented(t *testing.T) {
231231
fakeDRPCClient.telemeteryErorr = drpcerr.WithCode(xerrors.New("Unimplemented"), 0)
232232
uut.SendTelemetryEvent(&proto.TelemetryEvent{})
233233
require.False(t, uut.telemetryUnavailable.Load())
234+
require.Equal(t, int64(1), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
234235

235236
fakeDRPCClient.telemeteryErorr = drpcerr.WithCode(xerrors.New("Unimplemented"), drpcerr.Unimplemented)
236237
uut.SendTelemetryEvent(&proto.TelemetryEvent{})
237238
require.True(t, uut.telemetryUnavailable.Load())
239+
uut.SendTelemetryEvent(&proto.TelemetryEvent{})
240+
require.Equal(t, int64(2), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
238241
}
239242

240243
func TestTailnetAPIConnector_TelemetryNotRecognised(t *testing.T) {
@@ -268,10 +271,13 @@ func TestTailnetAPIConnector_TelemetryNotRecognised(t *testing.T) {
268271
fakeDRPCClient.telemeteryErorr = drpc.ProtocolError.New("Protocol Error")
269272
uut.SendTelemetryEvent(&proto.TelemetryEvent{})
270273
require.False(t, uut.telemetryUnavailable.Load())
274+
require.Equal(t, int64(1), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
271275

272276
fakeDRPCClient.telemeteryErorr = drpc.ProtocolError.New("unknown rpc: /coder.tailnet.v2.Tailnet/PostTelemetry")
273277
uut.SendTelemetryEvent(&proto.TelemetryEvent{})
274278
require.True(t, uut.telemetryUnavailable.Load())
279+
uut.SendTelemetryEvent(&proto.TelemetryEvent{})
280+
require.Equal(t, int64(2), atomic.LoadInt64(&fakeDRPCClient.postTelemetryCalls))
275281
}
276282

277283
type fakeTailnetConn struct{}
@@ -294,14 +300,16 @@ func newFakeTailnetConn() *fakeTailnetConn {
294300
}
295301

296302
type fakeDRPCClient struct {
297-
telemeteryErorr error
303+
postTelemetryCalls int64
304+
telemeteryErorr error
298305
fakeDRPPCMapStream
299306
}
300307

301308
var _ proto.DRPCTailnetClient = &fakeDRPCClient{}
302309

303310
func newFakeDRPCClient() *fakeDRPCClient {
304311
return &fakeDRPCClient{
312+
postTelemetryCalls: 0,
305313
fakeDRPPCMapStream: fakeDRPPCMapStream{
306314
fakeDRPCStream: fakeDRPCStream{
307315
ch: make(chan struct{}),
@@ -322,6 +330,7 @@ func (*fakeDRPCClient) DRPCConn() drpc.Conn {
322330

323331
// PostTelemetry implements proto.DRPCTailnetClient.
324332
func (f *fakeDRPCClient) PostTelemetry(_ context.Context, _ *proto.TelemetryRequest) (*proto.TelemetryResponse, error) {
333+
atomic.AddInt64(&f.postTelemetryCalls, 1)
325334
return nil, f.telemeteryErorr
326335
}
327336

‎docs/cli.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,10 @@ Disable direct (P2P) connections to workspaces.
156156
| Type | <code>bool</code> |
157157
| Environment | <code>$CODER_DISABLE_NETWORK_TELEMETRY</code> |
158158

159-
Disable network telemetry.
159+
Disable network telemetry. Network telemetry is collected when connecting to
160+
workspaces using the CLI, and is forwarded to the server. If telemetry is also
161+
enabled on the server, it may be sent to Coder. Network telemetry is used to
162+
measure network quality and detect regressions.
160163

161164
### --global-config
162165

‎enterprise/cli/testdata/coder_--help.golden

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,11 @@ variables or flags.
3131
Disable direct (P2P) connections to workspaces.
3232

3333
--disable-network-telemetry bool, $CODER_DISABLE_NETWORK_TELEMETRY
34-
Disable network telemetry.
34+
Disable network telemetry. Network telemetry is collected when
35+
connecting to workspaces using the CLI, and is forwarded to the
36+
server. If telemetry is also enabled on the server, it may be sent to
37+
Coder. Network telemetry is used to measure network quality and detect
38+
regressions.
3539

3640
--global-config string, $CODER_CONFIG_DIR (default: ~/.config/coderv2)
3741
Path to the global `coder` config directory.

‎tailnet/conn.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ func IPFromUUID(uid uuid.UUID) netip.Addr {
334334

335335
// Conn is an actively listening Wireguard connection.
336336
type Conn struct {
337-
// ID must be unique to this connection
337+
// Unique ID used for telemetry.
338338
id uuid.UUID
339339
mutex sync.Mutex
340340
closed chan struct{}
@@ -443,7 +443,6 @@ func (c *Conn) Status() *ipnstate.Status {
443443
func (c *Conn) Ping(ctx context.Context, ip netip.Addr) (time.Duration, bool, *ipnstate.PingResult, error) {
444444
dur, p2p, pr, err := c.pingWithType(ctx, ip, tailcfg.PingDisco)
445445
if err == nil {
446-
// TODO(ethanndickson): Is this too often?
447446
c.sendPingTelemetry(pr)
448447
}
449448
return dur, p2p, pr, err
@@ -742,7 +741,7 @@ func (c *Conn) SendSpeedtestTelemetry(throughputMbits float64) {
742741
}()
743742
}
744743

745-
// nolint: revive
744+
// nolint:revive
746745
func (c *Conn) sendPingTelemetry(pr *ipnstate.PingResult) {
747746
if c.telemetrySink == nil {
748747
return
@@ -752,9 +751,6 @@ func (c *Conn) sendPingTelemetry(pr *ipnstate.PingResult) {
752751
latency := durationpb.New(time.Duration(pr.LatencySeconds * float64(time.Second)))
753752
if pr.Endpoint != "" {
754753
e.P2PLatency = latency
755-
// TODO(ethanndickson): This could be nil if we can't parse the endpoint
756-
// But tailscale only ever sets the endpoint using `netip.AddrPort.String()`
757-
// So it's infallible.
758754
e.P2PEndpoint = c.telemeteryStore.toEndpoint(pr.Endpoint)
759755
} else {
760756
e.DerpLatency = latency

‎tailnet/telemetry.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"github.com/coder/coder/v2/tailnet/proto"
1818
)
1919

20-
// TODO(ethanndickson): How useful is the 'application' field?
2120
const (
2221
TelemetryApplicationSSH string = "ssh"
2322
TelemetryApplicationSpeedtest string = "speedtest"
@@ -77,9 +76,9 @@ func (b *TelemetryStore) updateDerpMap(cur *tailcfg.DERPMap) {
7776
n.IPv6 = ipv6
7877
stunIP, _, _ := b.processIPLocked(n.STUNTestIP)
7978
n.STUNTestIP = stunIP
80-
hn := b.hashAddr(n.HostName)
79+
hn := b.hashAddrorHostname(n.HostName)
8180
n.HostName = hn
82-
cn := b.hashAddr(n.CertName)
81+
cn := b.hashAddrorHostname(n.CertName)
8382
n.CertName = cn
8483
}
8584
}
@@ -140,7 +139,7 @@ func (b *TelemetryStore) toEndpoint(ipport string) *proto.TelemetryEvent_P2PEndp
140139
}
141140
addr := addrport.Addr()
142141
fields := addrToFields(addr)
143-
hashStr := b.hashAddr(addr.String())
142+
hashStr := b.hashAddrorHostname(addr.String())
144143
return &proto.TelemetryEvent_P2PEndpoint{
145144
Hash: hashStr,
146145
Port: int32(addrport.Port()),
@@ -159,11 +158,11 @@ func (b *TelemetryStore) processIPLocked(ip string) (string, *proto.IPFields, er
159158
}
160159

161160
fields := addrToFields(addr)
162-
hashStr := b.hashAddr(ip)
161+
hashStr := b.hashAddrorHostname(ip)
163162
return hashStr, fields, nil
164163
}
165164

166-
func (b *TelemetryStore) hashAddr(addr string) string {
165+
func (b *TelemetryStore) hashAddrorHostname(addr string) string {
167166
if hashStr, ok := b.hashCache[addr]; ok {
168167
return hashStr
169168
}

‎tailnet/telemetry_internal_test.go

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,16 +68,18 @@ func TestTelemetryStore(t *testing.T) {
6868

6969
event := telemetry.newEvent()
7070

71+
v4hash := telemetry.hashAddrorHostname(c.ipv4)
7172
require.Equal(t, &proto.Netcheck_NetcheckIP{
72-
Hash: telemetry.hashCache[c.ipv4],
73+
Hash: v4hash,
7374
Fields: &proto.IPFields{
7475
Version: 4,
7576
Class: c.expectedClass,
7677
},
7778
}, event.LatestNetcheck.GlobalV4)
7879

80+
v6hash := telemetry.hashAddrorHostname(c.ipv6)
7981
require.Equal(t, &proto.Netcheck_NetcheckIP{
80-
Hash: telemetry.hashCache[c.ipv6],
82+
Hash: v6hash,
8183
Fields: &proto.IPFields{
8284
Version: 6,
8385
Class: c.expectedClass,
@@ -95,9 +97,8 @@ func TestTelemetryStore(t *testing.T) {
9597
derpMap := &tailcfg.DERPMap{
9698
Regions: make(map[int]*tailcfg.DERPRegion),
9799
}
98-
// Add a region and node that uses every single field.
99-
derpMap.Regions[999] = &tailcfg.DERPRegion{
100-
RegionID: 999,
100+
derpMap.Regions[998] = &tailcfg.DERPRegion{
101+
RegionID: 998,
101102
EmbeddedRelay: true,
102103
RegionCode: "zzz",
103104
RegionName: "Cool Region",
@@ -106,7 +107,24 @@ func TestTelemetryStore(t *testing.T) {
106107
Nodes: []*tailcfg.DERPNode{
107108
{
108109
Name: "zzz1",
109-
RegionID: 999,
110+
RegionID: 998,
111+
HostName: "coolderp.com",
112+
CertName: "coolderpcert",
113+
IPv4: "1.2.3.4",
114+
IPv6: "2001:db8::1",
115+
STUNTestIP: "5.6.7.8",
116+
},
117+
},
118+
}
119+
derpMap.Regions[999] = &tailcfg.DERPRegion{
120+
RegionID: 999,
121+
EmbeddedRelay: true,
122+
RegionCode: "zzo",
123+
RegionName: "Other Cool Region",
124+
Avoid: true,
125+
Nodes: []*tailcfg.DERPNode{
126+
{
127+
Name: "zzo1",
110128
HostName: "coolderp.com",
111129
CertName: "coolderpcert",
112130
IPv4: "1.2.3.4",
@@ -124,5 +142,10 @@ func TestTelemetryStore(t *testing.T) {
124142
require.NotContains(t, node.Ipv4, "1.2.3.4")
125143
require.NotContains(t, node.Ipv6, "2001:db8::1")
126144
require.NotContains(t, node.StunTestIp, "5.6.7.8")
145+
otherNode := event.DerpMap.Regions[998].Nodes[0]
146+
require.Equal(t, otherNode.HostName, node.HostName)
147+
require.Equal(t, otherNode.Ipv4, node.Ipv4)
148+
require.Equal(t, otherNode.Ipv6, node.Ipv6)
149+
require.Equal(t, otherNode.StunTestIp, node.StunTestIp)
127150
})
128151
}

0 commit comments

Comments
 (0)