test: validate metrics compatibility with Grafana scrapper #6150
Conversation
WalkthroughAdds a new Go-based Prometheus metrics validator (with tests and go.mod/.gitignore), CI steps to set up Go and run its tests, appends metrics fetching and validation to the calibnet test script, and documents local usage in tools/README.md. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Script as calibnet_other_check.sh
participant Node as Forest Node (:6116)
participant Validator as prometheus_metrics_validator (go run)
Script->>Node: GET /metrics
Node-->>Script: metrics text
Script->>Validator: go run main.go metrics.log
Validator->>Validator: Validate(metrics)
alt parse ok
Validator-->>Script: exit 0 (valid)
else parse error
Validator-->>Script: exit 1 (invalid)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
hanabi1224
requested review from
akaladarshi and
elmattic
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/README.md (1)
1-10: Clarify the working directory for test commands.The unit test command
go test -v .implies it should be run fromtools/prometheus_metrics_validator/, but this isn't explicitly stated. Consider adding a note about the working directory or using the full path as in the CI workflow.Apply this diff to clarify:
### Unit tests -- run `go test -v .` +Run from the repository root: +```bash +go test -v ./tools/prometheus_metrics_validator +``` + +Or from the `tools/prometheus_metrics_validator/` directory: +```bash +go test -v . +``` ### Validate Forest metricstools/prometheus_metrics_validator/main.go (1)
45-60: Consider documenting the textparse.New boolean parameters.The
textparse.Newcall uses three boolean parameters (false, false, true) whose purpose is unclear without referring to the Prometheus documentation. Adding a brief comment would improve code maintainability.Apply this diff to add clarity:
func Validate(metrics []byte) error { + // textparse.New parameters: content, contentType, timestamp, parseClassicHistograms, skipMetrics, enableUTF8, symbolTable p, err := textparse.New(metrics, "text/plain", "", false, false, true, labels.NewSymbolTable()) if err != nil { return err
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.workis excluded by!**/*.worktools/prometheus_metrics_validator/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.github/workflows/forest.yml(2 hunks)scripts/tests/calibnet_other_check.sh(1 hunks)tools/README.md(1 hunks)tools/prometheus_metrics_validator/.gitignore(1 hunks)tools/prometheus_metrics_validator/go.mod(1 hunks)tools/prometheus_metrics_validator/main.go(1 hunks)tools/prometheus_metrics_validator/main_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/prometheus_metrics_validator/main_test.go (1)
tools/prometheus_metrics_validator/main.go (1)
Validate(45-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Diff snapshot export checks
- GitHub Check: V1 snapshot export checks
🔇 Additional comments (7)
.github/workflows/forest.yml (2)
86-88: LGTM! Go test step is correctly configured.The test command targets the correct path and will run the validator's unit tests during CI.
161-163: LGTM! Go setup is required for metrics validation.The calibnet-check job correctly sets up Go before running the validation script that invokes the metrics validator.
tools/prometheus_metrics_validator/main.go (1)
15-43: LGTM! CLI implementation is clean and functional.The CLI properly handles file reading, validation, and error reporting. The use of
os.Exit(1)on validation failure is appropriate for a CLI tool.tools/prometheus_metrics_validator/main_test.go (1)
9-45: LGTM! Test coverage is appropriate.The tests cover key scenarios:
- Metrics without EOF marker
- Metrics with proper metadata (HELP, TYPE, UNIT) and EOF
- Invalid syntax with unsupported characters
This provides good baseline coverage for the validator's core functionality.
scripts/tests/calibnet_other_check.sh (1)
83-85: LGTM! Metrics validation is properly integrated.The script correctly:
- Downloads metrics from the Forest node
- Invokes the validator to check compatibility
Using
go runis appropriate for the CI environment and keeps the script simple.tools/prometheus_metrics_validator/go.mod (2)
6-6: Prometheus module tag and parser import are correct v0.306.0 maps to Prometheus v3.6.0; import the text parser from "github.com/prometheus/prometheus/model/textparse".
3-3: Go version 1.25.2 is valid Thego versioncommand downloads and runs go1.25.2 without error.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@hanabi1224 CI check is failing |
There was a problem hiding this comment.
@hanabi1224 Could you please verify that this check fails on the Forest version (AFAIR the one that was used for NV27 calibnet) that had invalid characters in the metrics?
|
@LesnyRumcajs I believe the invalid chars have been covered by this unit test: https://github.com/ChainSafe/forest/pull/6150/files#diff-086c82375fc31c5ee660c5cf4ab8b7da381735d9bf6cbed910f53f4333465d16R37 |
Yeah, but the unit test is a synthetic case here. I'd like to confirm that the test fails when Forest reports bad metrics. A one off manual check would suffice. |
@LesnyRumcajs Sure. I just grabbed metrics from https://github.com/ChainSafe/forest/actions/runs/17643806179/job/50137364758#step:8:3557 (CI for the commit before the metrics fix) and run the tool against it. |
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6064
Other information and links
Change checklist
Summary by CodeRabbit