mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-09 06:59:08 +02:00
[client] disambiguate the connection_type metric tag (#7043)
## Describe your changes `recordConnectionMetrics` mapped only `conntype.Relay` to `relay` and let a `default` branch record everything else as `ice`. That silently included `ICETurn` — an ICE connection through a TURN server, which [`conn.isRelayed`](https://github.com/netbirdio/netbird/blob/main/client/internal/peer/conn.go#L788-L795) itself counts as relayed — and `None`, the transient state set when the relay drops ([conn.go:632](https://github.com/netbirdio/netbird/blob/main/client/internal/peer/conn.go#L632)) or the peer state is reset ([conn.go:757](https://github.com/netbirdio/netbird/blob/main/client/internal/peer/conn.go#L757)). Both were reported as direct peer-to-peer, so the `ice` share overstated direct connections on every platform. The mapping now lists every priority explicitly and emits `ice_p2p`, `ice_turn`, `relay` or `unknown`. The new values deliberately do not reuse `ice` to avoid ambuguity. ## Issue ticket number and link No public issue. Found while reviewing the first production sample of client metrics: 38% of iOS connection events were tagged `ice` on a platform that forces relay by default, which traced back to the `default` branch at [client/internal/peer/conn.go#L963-L968](https://github.com/netbirdio/netbird/blob/main/client/internal/peer/conn.go#L963-L968). ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal metrics documentation only, in `client/internal/metrics/infra/README.md`: the four `connection_type` values with their derivation, and a note that pre-fix `ice` samples are not comparable with `ice_p2p`. No public API, CLI or configuration change, so no netbirdio/docs PR. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: N/A <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit * **New Features** * Connection metrics now distinguish direct peer-to-peer, TURN-assisted, relay, and unknown connection types. * Metrics include clearer connection and peer identification details. * **Documentation** * Updated connection timing metric values, traffic semantics, priority behavior, and historical data guidance. * **Bug Fixes** * Unset or unrecognized connection priorities are no longer incorrectly classified as peer-to-peer. * Unknown-transport metrics are skipped to prevent misleading connection data. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -961,12 +961,9 @@ func (conn *Conn) recordConnectionMetrics() {
|
||||
priority := conn.currentConnPriority
|
||||
conn.mu.Unlock()
|
||||
|
||||
var connType metrics.ConnectionType
|
||||
switch priority {
|
||||
case conntype.Relay:
|
||||
connType = metrics.ConnectionTypeRelay
|
||||
default:
|
||||
connType = metrics.ConnectionTypeICE
|
||||
connType := metricsConnType(priority)
|
||||
if connType == metrics.ConnectionTypeUnknown {
|
||||
return
|
||||
}
|
||||
|
||||
// Record metrics with timestamps - duration calculation happens in metrics package
|
||||
@@ -1067,3 +1064,16 @@ func boolToConnStatus(connected bool) guard.ConnStatus {
|
||||
}
|
||||
return guard.ConnStatusDisconnected
|
||||
}
|
||||
|
||||
func metricsConnType(priority conntype.ConnPriority) metrics.ConnectionType {
|
||||
switch priority {
|
||||
case conntype.Relay:
|
||||
return metrics.ConnectionTypeRelay
|
||||
case conntype.ICETurn:
|
||||
return metrics.ConnectionTypeICETurn
|
||||
case conntype.ICEP2P:
|
||||
return metrics.ConnectionTypeICEP2P
|
||||
default:
|
||||
return metrics.ConnectionTypeUnknown
|
||||
}
|
||||
}
|
||||
|
||||
@@ -11,6 +11,8 @@ import (
|
||||
"github.com/stretchr/testify/assert"
|
||||
|
||||
"github.com/netbirdio/netbird/client/iface"
|
||||
"github.com/netbirdio/netbird/client/internal/metrics"
|
||||
"github.com/netbirdio/netbird/client/internal/peer/conntype"
|
||||
"github.com/netbirdio/netbird/client/internal/peer/dispatcher"
|
||||
"github.com/netbirdio/netbird/client/internal/peer/guard"
|
||||
"github.com/netbirdio/netbird/client/internal/peer/ice"
|
||||
@@ -386,3 +388,33 @@ func TestConn_onWGDisconnected_NoEscalationWithoutRosenpass(t *testing.T) {
|
||||
}
|
||||
assert.Empty(t, disconnected, "escalation must be limited to rosenpass connections")
|
||||
}
|
||||
|
||||
func TestMetricsConnType(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
priority conntype.ConnPriority
|
||||
expected metrics.ConnectionType
|
||||
}{
|
||||
{"relay", conntype.Relay, metrics.ConnectionTypeRelay},
|
||||
{"ice over turn is relayed, not p2p", conntype.ICETurn, metrics.ConnectionTypeICETurn},
|
||||
{"direct p2p", conntype.ICEP2P, metrics.ConnectionTypeICEP2P},
|
||||
{"unset priority is unknown, not p2p", conntype.None, metrics.ConnectionTypeUnknown},
|
||||
{"unrecognised priority is unknown", conntype.ConnPriority(99), metrics.ConnectionTypeUnknown},
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
assert.Equal(t, tc.expected, metricsConnType(tc.priority))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestMetricsConnType_RelayedMatchesIsRelayed(t *testing.T) {
|
||||
for _, priority := range []conntype.ConnPriority{conntype.None, conntype.Relay, conntype.ICETurn, conntype.ICEP2P} {
|
||||
conn := &Conn{currentConnPriority: priority}
|
||||
tag := metricsConnType(priority)
|
||||
relayedTag := tag == metrics.ConnectionTypeRelay || tag == metrics.ConnectionTypeICETurn
|
||||
assert.Equal(t, conn.isRelayed(), relayedTag,
|
||||
"priority %s: isRelayed and the %q metric tag must agree", priority, tag)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user