Skip to content

ci: warn when plugin metric field names are removed or renamed#18957

Open
yaseen-vm wants to merge 6 commits into
influxdata:masterfrom
yaseen-vm:feat/ci-metric-name-change-warning
Open

ci: warn when plugin metric field names are removed or renamed#18957
yaseen-vm wants to merge 6 commits into
influxdata:masterfrom
yaseen-vm:feat/ci-metric-name-change-warning

Conversation

@yaseen-vm

@yaseen-vm yaseen-vm commented May 21, 2026

Copy link
Copy Markdown

Summary

Closes #18955

  • Adds scripts/check-metric-names.sh — extracts backtick-quoted field names from plugin README metric tables and warns when any name present on master disappears in the PR
  • Adds .github/workflows/metric-names.yml — runs the script on every PR targeting master, only when plugin READMEs are modified
  • Always exits 0 (informational warning, not a hard block), consistent with the issue's request for a CI warning

How it works

Each plugin README documents its metrics in markdown tables like:

| `field_name` | include_option | type | description |

The script compares the set of backtick-quoted names in the first column between origin/master and HEAD. If any name is gone, a ::warning annotation is emitted pointing contributors to:

  1. Add a deprecation notice alongside the old name
  2. Keep the old name via a legacy_* include option
  3. Document the migration path in the PR description

Verification

Verified locally by temporarily renaming n_cpusn_cpus_RENAMED in plugins/inputs/system/README.md (replicating the exact change from #18919 that triggered this issue). The check correctly identified n_cpus as removed and emitted the warning with guidance.

  • Script runs cleanly on a branch with no README changes (No plugin READMEs changed)
  • Script fires ::warning when a field name is removed from a metric table
  • Script passes silently when only new fields are added (no false positives)
  • Workflow only triggers when plugins/**/README.md files are modified

Checklist

Related issues

resolves #18955

yaseen-vm added 3 commits May 21, 2026 15:57
Adds scripts/check-metric-names.sh and a GitHub Actions workflow that
runs on every PR targeting master. When a plugin README's metric table
loses a field name that was present on master, the check emits a
::warning annotation and explains the backward-compatibility impact,
helping contributors acknowledge breaking changes before they merge.

Closes influxdata#18955
@telegraf-tiger

Copy link
Copy Markdown
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger Bot added the ci CI pipeline fixes, optimizations, and infrastructure label May 21, 2026
@yaseen-vm

Copy link
Copy Markdown
Author

!signed-cla

@srebhan

srebhan commented May 26, 2026

Copy link
Copy Markdown
Member

Thanks for your contribution @yaseen-vm! Please restore the PR description template, especially the AI section, as we cannot review your PR otherwise.

@srebhan srebhan self-assigned this May 26, 2026
@yaseen-vm

Copy link
Copy Markdown
Author

Thanks for the heads-up @srebhan! I've restored the PR description template and checked the AI-generated code box as this was built with Claude Code following the InfluxData AI policy. Sorry for the oversight.

@srebhan

srebhan commented May 27, 2026

Copy link
Copy Markdown
Member

@yaseen-vm from what I see your approach expects the metrics to be described in the following format

| `field_name` | include_option | type | description |

However, this formatting is neither guaranteed nor does a majority of the REAMEs follow this convention. So how do you expect this to work?

@srebhan srebhan added the waiting for response waiting for response from contributor label May 28, 2026
@yaseen-edstem

Copy link
Copy Markdown

Thank you for the feedback, @srebhan.

You're right — the original implementation only matched the pipe-table format (| field_name | ... |), which is not the dominant convention across the plugin READMEs.

After sampling several plugins (e.g. cpu, activemq, aerospike), the overwhelming majority use the bullet-list format under a ## Metrics section:

- fields:
    - field_name (type)

I've updated the extract_fields logic to support both conventions:

  • Bullet-list (primary): matches indented - field_name lines under a fields: block — covers the vast majority of plugins.
  • Pipe-table (secondary): retains the original backtick-column match for READMEs that use that format.

This should give the check meaningful coverage across the plugin ecosystem. Let me know if you see any other edge cases worth handling.

@telegraf-tiger telegraf-tiger Bot removed the waiting for response waiting for response from contributor label May 29, 2026
The original parser only matched pipe-table formatted field names
(`| \`field_name\` | ... |`), which is rarely used across Telegraf
plugin READMEs. The dominant convention is the bullet-list format
under a Metrics section (`    - field_name (type)`).

Updated extract_fields to handle both formats so the check has
meaningful coverage across the plugin ecosystem.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@telegraf-tiger

Copy link
Copy Markdown
Contributor

@srebhan

srebhan commented Jun 1, 2026

Copy link
Copy Markdown
Member

Thanks for driving this @yaseen-vm! Instead of handling edge-cases I would prefer enforcing a certain structure for the metric description. How about the following

  1. Define a description structure in a Telegraf spec
  2. Extend the README linter to enforce the spec above
  3. Fix all issues the linter brings up
  4. Enable the linter in CI
  5. Add a CI job to warn on changed metrics

While this is way more than you target here it is certainly the most sustainable approach in the long run. What do you think? Are you willing to work on this?

@yaseen-vm

Copy link
Copy Markdown
Author

Thanks for the thoughtful feedback @srebhan! Absolutely agree — patching edge cases in the CI script is the wrong direction, and a spec-first approach is the right long-term investment.

I'm happy to take this on. I've opened issue #19003 to track the full scope of work:

👉 Enforce structured metric descriptions via spec, linter, and CI #19003

The plan there mirrors exactly what you outlined:

  1. Define a metric description spec under docs/specs/
  2. Extend tools/readme_linter to enforce it
  3. Fix all existing violations across plugins
  4. Enable the linter as a required CI check
  5. Land this PR's metric rename/removal warning cleanly on top of that foundation

I'll start with the spec and linter extension and keep that issue updated as I progress. Let me know if you have any preferences on the spec format or linter structure before I dive in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci CI pipeline fixes, optimizations, and infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Request - CI warning when changing existing metrics names

3 participants