wgengine: use underlying expvar for usermetrics

avoid the mutex in MultiLabelMap

Updates tailscale/corp#22075

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

wgengine/magicsock/derp.go

@@ -691,8 +691,8 @@ func (c *connBind) receiveDERP(buffs [][]byte, sizes []int, eps []conn.Endpoint)
 			continue
 		}
 		metricRecvDataPacketsDERP.Add(1)
-		c.metrics.inboundPacketsTotal.Add(pathLabel{Path: PathDERP}, 1)
+		c.metrics.inboundPacketsDERPTotal.Add(1)
-		c.metrics.inboundBytesTotal.Add(pathLabel{Path: PathDERP}, int64(n))
+		c.metrics.inboundBytesDERPTotal.Add(int64(n))
 		sizes[0] = n
 		eps[0] = ep
 		return 1, nil

wgengine/magicsock/endpoint.go

@@ -965,17 +965,15 @@ func (de *endpoint) send(buffs [][]byte) error {
 			txBytes += len(b)
 		}
 
-		var label pathLabel
 		switch {
 		case udpAddr.Addr().Is4():
-			label = pathLabel{Path: PathDirectIPv4}
+			de.c.metrics.outboundPacketsIPv4Total.Add(int64(len(buffs)))
+			de.c.metrics.outboundBytesIPv4Total.Add(int64(txBytes))
 		case udpAddr.Addr().Is6():
-			label = pathLabel{Path: PathDirectIPv6}
+			de.c.metrics.outboundPacketsIPv6Total.Add(int64(len(buffs)))
+			de.c.metrics.outboundBytesIPv6Total.Add(int64(txBytes))
 		}
 
-		de.c.metrics.outboundPacketsTotal.Add(label, int64(len(buffs)))
-		de.c.metrics.outboundBytesTotal.Add(label, int64(txBytes))
-
 		// TODO(raggi): needs updating for accuracy, as in error conditions we may have partial sends.
 		if stats := de.c.stats.Load(); err == nil && stats != nil {
 			stats.UpdateTxPhysical(de.nodeAddr, udpAddr, txBytes)
@@ -995,8 +993,8 @@ func (de *endpoint) send(buffs [][]byte) error {
 		if stats := de.c.stats.Load(); stats != nil {
 			stats.UpdateTxPhysical(de.nodeAddr, derpAddr, txBytes)
 		}
-		de.c.metrics.outboundPacketsTotal.Add(pathLabel{Path: PathDERP}, int64(len(buffs)))
+		de.c.metrics.outboundPacketsDERPTotal.Add(int64(len(buffs)))
-		de.c.metrics.outboundBytesTotal.Add(pathLabel{Path: PathDERP}, int64(txBytes))
+		de.c.metrics.outboundBytesDERPTotal.Add(int64(txBytes))
 		if allOk {
 			return nil
 		}

wgengine/magicsock/magicsock.go

@@ -10,6 +10,7 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"expvar"
 	"fmt"
 	"io"
 	"net"
@@ -33,7 +34,6 @@ import (
 	"tailscale.com/health"
 	"tailscale.com/hostinfo"
 	"tailscale.com/ipn/ipnstate"
-	tsmetrics "tailscale.com/metrics"
 	"tailscale.com/net/connstats"
 	"tailscale.com/net/netcheck"
 	"tailscale.com/net/neterror"
@@ -98,22 +98,35 @@ type pathLabel struct {
 	Path Path
 }
 
+// metrics in wgengine contains the usermetrics counters for magicsock, it
+// is however a bit special. All them metrics are labeled, but looking up
+// the metric everytime we need to record it has an overhead, and includes
+// a lock in MultiLabelMap. The metrics are therefore instead created with
+// wgengine and the underlying expvar.Int is stored to be used directly.
 type metrics struct {
 	// inboundPacketsTotal is the total number of inbound packets received,
 	// labeled by the path the packet took.
-	inboundPacketsTotal *tsmetrics.MultiLabelMap[pathLabel]
+	inboundPacketsIPv4Total *expvar.Int
-
+	inboundPacketsIPv6Total *expvar.Int
-	// outboundPacketsTotal is the total number of outbound packets sent,
+	inboundPacketsDERPTotal *expvar.Int
-	// labeled by the path the packet took.
-	outboundPacketsTotal *tsmetrics.MultiLabelMap[pathLabel]
 
 	// inboundBytesTotal is the total number of inbound bytes received,
 	// labeled by the path the packet took.
-	inboundBytesTotal *tsmetrics.MultiLabelMap[pathLabel]
+	inboundBytesIPv4Total *expvar.Int
+	inboundBytesIPv6Total *expvar.Int
+	inboundBytesDERPTotal *expvar.Int
+
+	// outboundPacketsTotal is the total number of outbound packets sent,
+	// labeled by the path the packet took.
+	outboundPacketsIPv4Total *expvar.Int
+	outboundPacketsIPv6Total *expvar.Int
+	outboundPacketsDERPTotal *expvar.Int
 
 	// outboundBytesTotal is the total number of outbound bytes sent,
 	// labeled by the path the packet took.
-	outboundBytesTotal *tsmetrics.MultiLabelMap[pathLabel]
+	outboundBytesIPv4Total *expvar.Int
+	outboundBytesIPv6Total *expvar.Int
+	outboundBytesDERPTotal *expvar.Int
 }
 
 // A Conn routes UDP packets and actively manages a list of its endpoints.
@@ -542,32 +555,7 @@ func NewConn(opts Options) (*Conn, error) {
 		UseDNSCache:         true,
 	}
 
-	c.metrics = &metrics{
+	c.metrics = registerMetrics(opts.Metrics)
-		inboundBytesTotal: usermetric.NewMultiLabelMapWithRegistry[pathLabel](
-			opts.Metrics,
-			"tailscaled_inbound_bytes_total",
-			"counter",
-			"Counts the number of bytes received from other peers",
-		),
-		inboundPacketsTotal: usermetric.NewMultiLabelMapWithRegistry[pathLabel](
-			opts.Metrics,
-			"tailscaled_inbound_packets_total",
-			"counter",
-			"Counts the number of packets received from other peers",
-		),
-		outboundBytesTotal: usermetric.NewMultiLabelMapWithRegistry[pathLabel](
-			opts.Metrics,
-			"tailscaled_outbound_bytes_total",
-			"counter",
-			"Counts the number of bytes sent to other peers",
-		),
-		outboundPacketsTotal: usermetric.NewMultiLabelMapWithRegistry[pathLabel](
-			opts.Metrics,
-			"tailscaled_outbound_packets_total",
-			"counter",
-			"Counts the number of packets sent to other peers",
-		),
-	}
 
 	if d4, err := c.listenRawDisco("ip4"); err == nil {
 		c.logf("[v1] using BPF disco receiver for IPv4")
@@ -586,6 +574,74 @@ func NewConn(opts Options) (*Conn, error) {
 	return c, nil
 }
 
+// registerMetrics wires up the metrics for wgengine, instead of
+// registering the label metric directly, the underlying expvar is exposed.
+// See metrics for more info.
+func registerMetrics(reg *usermetric.Registry) *metrics {
+	pathDirectV4 := pathLabel{Path: PathDirectIPv4}
+	pathDirectV6 := pathLabel{Path: PathDirectIPv6}
+	pathDERP := pathLabel{Path: PathDERP}
+	inboundPacketsTotal := usermetric.NewMultiLabelMapWithRegistry[pathLabel](
+		reg,
+		"tailscaled_inbound_packets_total",
+		"counter",
+		"Counts the number of packets received from other peers",
+	)
+	inboundBytesTotal := usermetric.NewMultiLabelMapWithRegistry[pathLabel](
+		reg,
+		"tailscaled_inbound_bytes_total",
+		"counter",
+		"Counts the number of bytes received from other peers",
+	)
+	outboundPacketsTotal := usermetric.NewMultiLabelMapWithRegistry[pathLabel](
+		reg,
+		"tailscaled_outbound_packets_total",
+		"counter",
+		"Counts the number of packets sent to other peers",
+	)
+	outboundBytesTotal := usermetric.NewMultiLabelMapWithRegistry[pathLabel](
+		reg,
+		"tailscaled_outbound_bytes_total",
+		"counter",
+		"Counts the number of bytes sent to other peers",
+	)
+	m := metrics{
+		inboundPacketsIPv4Total: &expvar.Int{},
+		inboundPacketsIPv6Total: &expvar.Int{},
+		inboundPacketsDERPTotal: &expvar.Int{},
+
+		inboundBytesIPv4Total: &expvar.Int{},
+		inboundBytesIPv6Total: &expvar.Int{},
+		inboundBytesDERPTotal: &expvar.Int{},
+
+		outboundPacketsIPv4Total: &expvar.Int{},
+		outboundPacketsIPv6Total: &expvar.Int{},
+		outboundPacketsDERPTotal: &expvar.Int{},
+
+		outboundBytesIPv4Total: &expvar.Int{},
+		outboundBytesIPv6Total: &expvar.Int{},
+		outboundBytesDERPTotal: &expvar.Int{},
+	}
+
+	inboundPacketsTotal.Set(pathDirectV4, m.inboundPacketsIPv4Total)
+	inboundPacketsTotal.Set(pathDirectV6, m.inboundPacketsIPv6Total)
+	inboundPacketsTotal.Set(pathDERP, m.inboundPacketsDERPTotal)
+
+	inboundBytesTotal.Set(pathDirectV4, m.inboundBytesIPv4Total)
+	inboundBytesTotal.Set(pathDirectV6, m.inboundBytesIPv6Total)
+	inboundBytesTotal.Set(pathDERP, m.inboundBytesDERPTotal)
+
+	outboundPacketsTotal.Set(pathDirectV4, m.outboundPacketsIPv4Total)
+	outboundPacketsTotal.Set(pathDirectV6, m.outboundPacketsIPv6Total)
+	outboundPacketsTotal.Set(pathDERP, m.outboundPacketsDERPTotal)
+
+	outboundBytesTotal.Set(pathDirectV4, m.outboundBytesIPv4Total)
+	outboundBytesTotal.Set(pathDirectV6, m.outboundBytesIPv6Total)
+	outboundBytesTotal.Set(pathDERP, m.outboundBytesDERPTotal)
+
+	return &m
+}
+
 // InstallCaptureHook installs a callback which is called to
 // log debug information into the pcap stream. This function
 // can be called with a nil argument to uninstall the capture
@@ -1210,11 +1266,11 @@ func (c *Conn) sendUDP(ipp netip.AddrPort, b []byte) (sent bool, err error) {
 
 			switch {
 			case ipp.Addr().Is4():
-				c.metrics.outboundPacketsTotal.Add(pathLabel{Path: PathDirectIPv4}, 1)
+				c.metrics.outboundPacketsIPv4Total.Add(1)
-				c.metrics.outboundBytesTotal.Add(pathLabel{Path: PathDirectIPv4}, int64(len(b)))
+				c.metrics.outboundBytesIPv4Total.Add(int64(len(b)))
 			case ipp.Addr().Is6():
-				c.metrics.outboundPacketsTotal.Add(pathLabel{Path: PathDirectIPv6}, 1)
+				c.metrics.outboundPacketsIPv6Total.Add(1)
-				c.metrics.outboundBytesTotal.Add(pathLabel{Path: PathDirectIPv6}, int64(len(b)))
+				c.metrics.outboundBytesIPv6Total.Add(int64(len(b)))
 			}
 		}
 	}
@@ -1357,9 +1413,9 @@ func (c *Conn) receiveIPv4() conn.ReceiveFunc {
 	return c.mkReceiveFunc(&c.pconn4, c.health.ReceiveFuncStats(health.ReceiveIPv4),
 		func(i int64) {
 			metricRecvDataPacketsIPv4.Add(i)
-			c.metrics.inboundPacketsTotal.Add(pathLabel{Path: PathDirectIPv4}, i)
+			c.metrics.inboundPacketsIPv4Total.Add(i)
 		}, func(i int64) {
-			c.metrics.inboundBytesTotal.Add(pathLabel{Path: PathDirectIPv4}, i)
+			c.metrics.inboundBytesIPv4Total.Add(i)
 		})
 }
 
@@ -1368,9 +1424,9 @@ func (c *Conn) receiveIPv6() conn.ReceiveFunc {
 	return c.mkReceiveFunc(&c.pconn6, c.health.ReceiveFuncStats(health.ReceiveIPv6),
 		func(i int64) {
 			metricRecvDataPacketsIPv6.Add(i)
-			c.metrics.inboundPacketsTotal.Add(pathLabel{Path: PathDirectIPv6}, i)
+			c.metrics.inboundPacketsIPv6Total.Add(i)
 		}, func(i int64) {
-			c.metrics.inboundBytesTotal.Add(pathLabel{Path: PathDirectIPv6}, i)
+			c.metrics.inboundBytesIPv6Total.Add(i)
 		})
 }
 

wgengine/magicsock/magicsock_test.go

@@ -10,7 +10,6 @@ import (
 	"crypto/tls"
 	"encoding/binary"
 	"errors"
-	"expvar"
 	"fmt"
 	"io"
 	"math/rand"
@@ -1207,10 +1206,18 @@ func testTwoDevicePing(t *testing.T, d *devices) {
 }
 
 func (c *Conn) resetMetricsForTest() {
-	c.metrics.inboundBytesTotal.ResetAllForTest()
+	c.metrics.inboundBytesIPv4Total.Set(0)
-	c.metrics.inboundPacketsTotal.ResetAllForTest()
+	c.metrics.inboundPacketsIPv4Total.Set(0)
-	c.metrics.outboundBytesTotal.ResetAllForTest()
+	c.metrics.outboundBytesIPv4Total.Set(0)
-	c.metrics.outboundPacketsTotal.ResetAllForTest()
+	c.metrics.outboundPacketsIPv4Total.Set(0)
+	c.metrics.inboundBytesIPv6Total.Set(0)
+	c.metrics.inboundPacketsIPv6Total.Set(0)
+	c.metrics.outboundBytesIPv6Total.Set(0)
+	c.metrics.outboundPacketsIPv6Total.Set(0)
+	c.metrics.inboundBytesDERPTotal.Set(0)
+	c.metrics.inboundPacketsDERPTotal.Set(0)
+	c.metrics.outboundBytesDERPTotal.Set(0)
+	c.metrics.outboundPacketsDERPTotal.Set(0)
 }
 
 func assertConnStatsAndUserMetricsEqual(t *testing.T, ms *magicStack) {
@@ -1239,33 +1246,15 @@ func assertConnStatsAndUserMetricsEqual(t *testing.T, ms *magicStack) {
 		}
 	}
 
-	var metricIPv4RxBytes, metricIPv4TxBytes, metricDERPRxBytes, metricDERPTxBytes int64
+	metricIPv4RxBytes := ms.conn.metrics.inboundBytesIPv4Total.Value()
-	var metricIPv4RxPackets, metricIPv4TxPackets, metricDERPRxPackets, metricDERPTxPackets int64
+	metricIPv4RxPackets := ms.conn.metrics.inboundPacketsIPv4Total.Value()
+	metricIPv4TxBytes := ms.conn.metrics.outboundBytesIPv4Total.Value()
+	metricIPv4TxPackets := ms.conn.metrics.outboundPacketsIPv4Total.Value()
 
-	if m, ok := ms.conn.metrics.inboundBytesTotal.Get(pathLabel{Path: PathDirectIPv4}).(*expvar.Int); ok {
+	metricDERPRxBytes := ms.conn.metrics.inboundBytesDERPTotal.Value()
-		metricIPv4RxBytes = m.Value()
+	metricDERPRxPackets := ms.conn.metrics.inboundPacketsDERPTotal.Value()
-	}
+	metricDERPTxBytes := ms.conn.metrics.outboundBytesDERPTotal.Value()
-	if m, ok := ms.conn.metrics.outboundBytesTotal.Get(pathLabel{Path: PathDirectIPv4}).(*expvar.Int); ok {
+	metricDERPTxPackets := ms.conn.metrics.outboundPacketsDERPTotal.Value()
-		metricIPv4TxBytes = m.Value()
-	}
-	if m, ok := ms.conn.metrics.inboundBytesTotal.Get(pathLabel{Path: PathDERP}).(*expvar.Int); ok {
-		metricDERPRxBytes = m.Value()
-	}
-	if m, ok := ms.conn.metrics.outboundBytesTotal.Get(pathLabel{Path: PathDERP}).(*expvar.Int); ok {
-		metricDERPTxBytes = m.Value()
-	}
-	if m, ok := ms.conn.metrics.inboundPacketsTotal.Get(pathLabel{Path: PathDirectIPv4}).(*expvar.Int); ok {
-		metricIPv4RxPackets = m.Value()
-	}
-	if m, ok := ms.conn.metrics.outboundPacketsTotal.Get(pathLabel{Path: PathDirectIPv4}).(*expvar.Int); ok {
-		metricIPv4TxPackets = m.Value()
-	}
-	if m, ok := ms.conn.metrics.inboundPacketsTotal.Get(pathLabel{Path: PathDERP}).(*expvar.Int); ok {
-		metricDERPRxPackets = m.Value()
-	}
-	if m, ok := ms.conn.metrics.outboundPacketsTotal.Get(pathLabel{Path: PathDERP}).(*expvar.Int); ok {
-		metricDERPTxPackets = m.Value()
-	}
 
 	assertEqual(t, "derp bytes inbound", physDERPRxBytes, metricDERPRxBytes)
 	assertEqual(t, "derp bytes outbound", physDERPTxBytes, metricDERPTxBytes)