[PR #7043] [client] disambiguate the connection_type metric tag #29478

Open
opened 2026-08-05 08:08:09 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/7043
Author: @riccardomanfrin
Created: 8/3/2026
Status: 🔄 Open

Base: mainHead: disambiguate_p2p_metrics


📝 Commits (1)

  • a4bc8b1 [client] disambiguate the connection_type metric tag

📊 Changes

5 files changed (+100 additions, -16 deletions)

View changed files

📝 client/internal/metrics/connection_type.go (+19 -4)
📝 client/internal/metrics/influxdb_test.go (+2 -2)
📝 client/internal/metrics/infra/README.md (+18 -1)
📝 client/internal/peer/conn.go (+23 -9)
📝 client/internal/peer/conn_test.go (+38 -0)

📄 Description

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 itself counts as relayed — and None, the transient state set when the relay drops
(conn.go:632) or the peer state is reset (conn.go:757).
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.

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.

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • 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.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • 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

Summary by CodeRabbit

  • New Features

    • Connection metrics now distinguish direct peer-to-peer, TURN-assisted, relay, and unknown connection types.
    • Metrics include additional peer and connection-pair identifiers.
  • Documentation

    • Updated connection timing metric values, traffic semantics, priority behavior, and historical data guidance.
  • Bug Fixes

    • Unset or unrecognized connection priorities are now reported as unknown instead of being incorrectly classified as peer-to-peer.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/7043 **Author:** [@riccardomanfrin](https://github.com/riccardomanfrin) **Created:** 8/3/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `disambiguate_p2p_metrics` --- ### 📝 Commits (1) - [`a4bc8b1`](https://github.com/netbirdio/netbird/commit/a4bc8b10be590c91e02bd90801f82576016b62c6) [client] disambiguate the connection_type metric tag ### 📊 Changes **5 files changed** (+100 additions, -16 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/metrics/connection_type.go` (+19 -4) 📝 `client/internal/metrics/influxdb_test.go` (+2 -2) 📝 `client/internal/metrics/infra/README.md` (+18 -1) 📝 `client/internal/peer/conn.go` (+23 -9) 📝 `client/internal/peer/conn_test.go` (+38 -0) </details> ### 📄 Description ## 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: - [x] I added/updated documentation for this change - [ ] 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 * **New Features** * Connection metrics now distinguish direct peer-to-peer, TURN-assisted, relay, and unknown connection types. * Metrics include additional peer and connection-pair identifiers. * **Documentation** * Updated connection timing metric values, traffic semantics, priority behavior, and historical data guidance. * **Bug Fixes** * Unset or unrecognized connection priorities are now reported as unknown instead of being incorrectly classified as peer-to-peer. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:08:09 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#29478