|WARNING| metrics: remove experimental telemetry helper API

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170706

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-07

# DPDK Patch Review: metrics: remove experimental telemetry helper API

## Summary

This patch removes unused experimental API functions from the metrics library and converts them to static helpers. The cleanup is generally sound, but there are a few issues to address.

---

## Errors

### 1. Missing function implementation

**File**: `lib/metrics/rte_metrics_telemetry.c`

The release notes document removal of `rte_metrics_tel_get_global_stats`, but this function is not present in the visible portion of the patch. The `#else` stub for `!RTE_HAS_JANSSON` shows it existed, but the main implementation branch does not show it being converted to static or retained.

**Recommendation**: Verify that `rte_metrics_tel_get_global_stats` is either:
- Converted to static (and the patch should show this)
- Actually unused and removed entirely (and the legacy command list in the release notes should be verified)

If the function is still needed for the legacy `global_stat_values` command mentioned in the release notes, it must be present in the file.

---

## Warnings

### 1. Build file changes may break conditional compilation

**File**: `lib/metrics/meson.build`

The reorganization makes `rte_metrics_telemetry.c` build only when `RTE_HAS_JANSSON` is defined:

```python
if dpdk_conf.has('RTE_HAS_JANSSON'):
    sources += files('rte_metrics_telemetry.c')
```

However, the file contains the telemetry command registration in `RTE_INIT(metrics_ctor)`. If this file is not compiled when jansson is absent, the legacy telemetry commands (`ports_all_stat_values`, etc.) will not be registered at all, not just return `-ENOTSUP`.

**Recommendation**: Verify that the legacy telemetry command registration is either:
- Not needed when jansson is absent (the entire telemetry subsystem doesn't work)
- Moved to a file that always compiles

The old code had stubs that returned `-ENOTSUP`, implying the commands could be registered but would fail at runtime. The new code won't register them at all.

---

### 2. Release notes claim "no users" but functions are called internally

**File**: `doc/guides/rel_notes/release_26_11.rst`

The release notes state:

> These had no users and exposed jansson types in the public header.

However, the patch shows these functions ARE used -- by the telemetry command handlers in the same file (e.g., `handle_ports_all_stat_values`, `handle_global_stat_values`). They had no *external* users outside the metrics library.

**Recommendation**: Clarify the release notes:

```diff
- These had no users and exposed jansson types in the public header.
+ These had no external users and exposed jansson types in the public header.
```

Or more precisely:

```diff
+ These were only used internally by the metrics library's legacy telemetry
+ commands and exposed jansson types in the public header.
```

---

### 3. Static variable declaration style

**File**: `lib/metrics/rte_metrics_telemetry.c`

```c
static struct telemetry_metrics_data {
	int reg_index[RTE_MAX_ETHPORTS];
	int metrics_register_done;
} tel_met_data;
```

While this works, DPDK style typically separates the type definition from the variable declaration for clarity:

```c
struct telemetry_metrics_data {
	int reg_index[RTE_MAX_ETHPORTS];
	int metrics_register_done;
};

static struct telemetry_metrics_data tel_met_data;
```

This is a minor style preference, not an error.

---

## Info

### Positive aspects

1. **Correct removal of dead exports**: The functions were genuinely not used outside the metrics library, so removing them from the public API is appropriate.

2. **Proper cleanup of jansson exposure**: The jansson types (`json_t`) were in a public header, which is an ABI/dependency leak. Moving them to the `.c` file is correct.

3. **Release notes updated**: The patch properly documents the removal in `release_26_11.rst`.

4. **Stub branch removal is correct**: Since the file now only builds when jansson is present, the `!RTE_HAS_JANSSON` stubs are no longer needed.

5. **Function conversion to static is correct**: All the converted functions are only called within the same file, so `static` is appropriate.

---

## Pre-submission Checklist

Before submitting, verify:

- [ ] `rte_metrics_tel_get_global_stats` implementation is present (or confirmed removed entirely if unused)
- [ ] Legacy telemetry commands still register when jansson is absent (or this is intentional and tested)
- [ ] Build tested with and without jansson
- [ ] Functional test that the legacy commands still work (`dpdk-telemetry.py` - `/metrics/ports_all_stat_values`)
- [ ] No compilation errors with `devtools/test-meson-builds.sh`