Re: [PATCH v2 0/7] Add script for real-time telemetry monitoring

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  5 Jan 2026 17:55:58 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> TL;DR
> ------
> 
> For a  quick demo, apply patches, run e.g. testpmd and then in a separate
> terminal run:
> 
>   ./usertools/dpdk-telemetry-watcher.py -d1T eth.tx
> 
> Output, updated once per second, will be traffic rate per port e.g.:
> 
> Connected to application: "dpdk-testpmd"
> Time       /ethdev/stats,0.opackets /ethdev/stats,1.opackets        Total
> 16:29:12                  5,213,119                5,214,304   10,427,423
> 
> 
> Fuller details
> --------------
> 
> While we have the dpdk-telemetry.py CLI app for interactive querying of
> telemetry on the commandline, and a telemetry exporter script for
> sending telemetry to external tools for real-time monitoring, we don't
> have an app that can print real-time stats for DPDK apps on the
> terminal. This patchset adds such a script, developed with the help of
> Github copilot to fill a need that I found in my testing. Submitting it
> here in the hopes that others find it of use.
> 
> The script acts as a wrapper around the existing dpdk-telemetry.py
> script, and pipes the commands to that script and reads the responses,
> querying it once per second. It takes a number of flag parameters, such
> as the ones above:
>  - "-d" for delta values, i.e. PPS rather than total packets
>  - "-1" for single-line output, i.e. no scrolling up the screen
>  - "-T" to display a total column
> 
> Other flag parameters can be seen by looking at the help output.
> 
> Beyond the flags, the script also takes a number of positional
> parameters, which refer to specific stats to display. These stats must
> be numeric values, and should take the form of the telemetry command to
> send, followed by a "." and the stat within the result which is to be
> tracked. As above, a stat would be e.g. "/ethdev/stats,0.opackets",
> where we send "/ethdev/stats,0" to telemetry and extract the "opackets"
> part of the result.
> 
> However, specifying individual stats can be awkward, so some shortcuts
> are provided too for the common case of monitoring ethernet ports. Any
> positional arg starting with "eth" will be replaced by the set of
> equivalent values for each port, e.g. "eth.imissed" will track the
> imissed value on all ports in use in the app. The ipackets and opackets
> values, as common metrics, are also available as shortened values as
> just "rx" and "tx", so in the example above, "eth.tx" means to track the
> opackets stat for every ethdev port.
> 
> Finally, the script also has reconnection support so you can leave it
> running while you start and stop your application in another terminal.
> The watcher will try and reconnect to a running instance every second.
> 
> v2:
> - improve reconnection handling, eliminating some crashes seen in testing.
> 
> Bruce Richardson (7):
>   usertools: add new script to monitor telemetry on terminal
>   usertools/telemetry-watcher: add displaying stats
>   usertools/telemetry-watcher: add delta and timeout opts
>   usertools/telemetry-watcher: add total and one-line opts
>   usertools/telemetry-watcher: add thousands separator
>   usertools/telemetry-watcher: add eth name shortcuts
>   usertools/telemetry-watcher: support reconnection
> 
>  usertools/dpdk-telemetry-watcher.py | 435 ++++++++++++++++++++++++++++
>  usertools/meson.build               |   1 +
>  2 files changed, 436 insertions(+)
>  create mode 100755 usertools/dpdk-telemetry-watcher.py
> 
> --
> 2.51.0

Series-Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Automated patch review reports some things that could be added.



## DPDK Patch Review: Telemetry Watcher (v2, 7 patches)

**Submitter:** Bruce Richardson <bruce.richardson@intel.com>  
**Series:** [PATCH v2 1-7/7] usertools: add telemetry watcher script

---

### Overall Summary

This is a well-structured patch series adding a new Python script (`dpdk-telemetry-watcher.py`) to monitor DPDK telemetry statistics on the command line. The series progressively builds up functionality across 7 commits, making the series bisectable.

---

### Patch-by-Patch Review

#### **Patch 1/7: usertools: add new script to monitor telemetry on terminal**

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 53 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | "add new script" |
| No trailing period | ✅ PASS | |
| Signed-off-by | ✅ PASS | |
| Body doesn't start with "It" | ✅ PASS | Starts with "The" |
| SPDX license | ✅ PASS | BSD-3-Clause on line 2 (correct for shebang script) |
| Copyright line | ✅ PASS | |
| Blank line after copyright | ✅ PASS | |

**Warning (should fix):**
- **Line 178**: Unnecessary f-string prefix:
  ```python
  print(f"Error: Python interpreter or script not found", file=sys.stderr)
  ```
  Should be:
  ```python
  print("Error: Python interpreter or script not found", file=sys.stderr)
  ```

#### **Patch 2/7: usertools/telemetry-watcher: add displaying stats**

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 47 characters |
| Signed-off-by | ✅ PASS | |
| Body wrapping | ✅ PASS | Wrapped at 75 chars |

✅ No issues found.

#### **Patch 3/7: usertools/telemetry-watcher: add delta and timeout opts**

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 55 characters |
| Signed-off-by | ✅ PASS | |

✅ No issues found.

#### **Patch 4/7: usertools/telemetry-watcher: add total and one-line opts**

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 56 characters |
| Signed-off-by | ✅ PASS | |

✅ No issues found.

#### **Patch 5/7: usertools/telemetry-watcher: add thousands separator**

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 51 characters |
| Signed-off-by | ✅ PASS | |

✅ No issues found.

#### **Patch 6/7: usertools/telemetry-watcher: add eth name shortcuts**

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 50 characters |
| Signed-off-by | ✅ PASS | |

**Info (consider):**
- Line 1124: f-string without interpolation:
  ```python
  print(f"Error: Failed to get ethernet device list", file=sys.stderr)
  ```

#### **Patch 7/7: usertools/telemetry-watcher: support reconnection**

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 48 characters |
| Signed-off-by | ✅ PASS | |

**Info (consider):**
- Lines 1243-1244: Dynamically adding attributes to `subprocess.Popen` object:
  ```python
  process.script = telemetry_script
  process.args = args_list
  ```
  This works but could be cleaner using a wrapper class or dataclass to hold the process and its metadata together.

---

### Series-Level Issues

#### **Warning: Missing Documentation**

Per AGENTS.md: *"Code and documentation must be updated atomically in same patch"* and *"Documentation must match the code"*

The series adds a new usertool but does not include documentation updates. Consider adding:
- Usage documentation in `doc/guides/tools/` describing the new script
- Entry in the tools index

#### **Warning: Missing Release Notes**

Per AGENTS.md: *"New drivers or subsystems must have release notes"* and *"Current release notes updated for significant changes"*

A new usertools script should be mentioned in `doc/guides/rel_notes/release_XX_XX.rst` under a section like "New Features" or similar.

#### **Info: meson.build List Ordering**

In patch 1/7, the addition to `usertools/meson.build`:
```python
            'dpdk-hugepages.py',
            'dpdk-rss-flows.py',
            'dpdk-telemetry-exporter.py',
+           'dpdk-telemetry-watcher.py',
```

Per AGENTS.md: *"Lists alphabetically ordered"*. The insertion is correctly placed alphabetically between `dpdk-telemetry-exporter.py` and the end. ✅

---

### Code Quality Observations

**Positive:**
- Clean incremental commits that are individually bisectable
- Good docstrings on functions
- Proper error handling with messages to stderr
- Sensible defaults for optional arguments
- Good use of argparse for CLI handling

**Info (minor suggestions):**
1. The reconnection logic in patch 7/7 could benefit from exponential backoff instead of fixed 1-second retries
2. Consider adding `--help` examples in the epilog of argparse

---

### Summary Table

| Severity | Count | Items |
|----------|-------|-------|
| **Error** | 0 | None |
| **Warning** | 3 | Missing documentation, missing release notes, unnecessary f-string (1/7 line 178) |
| **Info** | 3 | Unnecessary f-string (6/7), dynamic attribute assignment style (7/7), reconnection backoff |

---

### Recommendation

**Acked-by candidate** with minor fixes needed:

1. Remove unnecessary f-string prefix in patch 1/7 line 178
2. Add documentation for the new tool
3. Add release notes entry

The code itself is well-written and the series structure is good. The issues are primarily around the documentation requirements rather than technical problems with the implementation.