On Mon, 5 Jan 2026 17:55:58 +0000 Bruce Richardson <[email protected]> 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 <[email protected]> Automated patch review reports some things that could be added. ## DPDK Patch Review: Telemetry Watcher (v2, 7 patches) **Submitter:** Bruce Richardson <[email protected]> **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.
