imbajin commented on issue #3142:
URL: https://github.com/apache/hugegraph/issues/3142#issuecomment-5218468945
Hi @cui2022, thank you for the detailed investigation and for testing the
fix in a real deployment. This is a valuable finding, and we would be very
happy to welcome your PR.
We checked the current `master` source code and would like to clarify the
two code paths first:
- `GET /metrics` converts gauges, histograms, meters, and timers through
`replaceDotDashInKey()`. The Cache metrics registered by
`GraphManager.registerCacheMetrics()` also use this path.
- `GET /metrics/statistics` converts REST-path metric names through
`replaceSlashInKey()`.
Therefore, for the approximately 20 Cache metrics containing `~` that you
reported from `/metrics`, the direct fix is in `replaceDotDashInKey()`. On
current `master`, changing only `replaceSlashInKey()` cannot change those Cache
gauge names. The different result in your deployment may be related to the
endpoint scraped or to which rebuilt Server artifact was actually deployed, so
please verify the exact URL and JAR/image once more. Your production
observation is still very useful; this clarification is only about the current
source call chain.
### Recommended fix
For a small, low-risk PR, it is acceptable to add `.replace("~", "_")` to
both methods:
```java
public static String replaceDotDashInKey(String orgKey) {
return orgKey.replace(".", "_")
.replace("-", "_")
.replace("/", "_")
.replace("$", "_")
.replace("~", "_");
}
public static String replaceSlashInKey(String orgKey) {
return orgKey.replace("/", "_").replace("~", "_");
}
```
The first change fixes the reported `/metrics` Cache metrics. The second
protects `/metrics/statistics` consistently.
A slightly cleaner alternative is to replace the two partial helpers with
one clearly named helper, such as `sanitizePrometheusMetricName()`, and use it
everywhere a metric name is emitted. Since HugeGraph currently writes unquoted
Prometheus text identifiers, the output should remain compatible with the
legacy-safe pattern `[a-zA-Z_:][a-zA-Z0-9_:]*`. Please keep the sanitization in
the Prometheus exposition layer; changing the internal Cache names is
unnecessary and could affect other behavior.
For this issue, either the focused `~` replacement or a small unified helper
is reasonable. Please avoid mixing unrelated metrics-format refactoring into
the same PR.
### Tests to include
Please add a regression test, rather than relying only on a deployment check:
1. Create a fresh `MetricRegistry` and register a gauge whose name contains
`~` (preferably a Cache-like name).
2. Call `MetricsUtil.writePrometheusFormat()`.
3. Assert that the `# HELP`, `# TYPE`, and sample lines all use the
sanitized name and that no unquoted `~` remains.
4. Also cover `/` and `~` for the statistics-name path, or test the unified
sanitizer directly.
5. Keep your Prometheus/Grafana scrape result in the PR description as
end-to-end evidence.
You can run the focused test and basic checks with:
```bash
mvn test -pl hugegraph-server/hugegraph-test -am \
-P unit-test -Dtest=MetricsUtilTest
mvn editorconfig:format
mvn clean compile -Dmaven.javadoc.skip=true
```
If you extend `MetricsApiTest`, also run the relevant API-test profile.
### How to submit the contribution
The full guide is in
[CONTRIBUTING.md](https://github.com/apache/hugegraph/blob/master/CONTRIBUTING.md).
A concise workflow is:
```bash
# 1. Fork apache/hugegraph on GitHub, then clone your fork
git clone https://github.com/<your-github-name>/hugegraph.git
cd hugegraph
# 2. Add the official repository and start from the latest master
git remote add upstream https://github.com/apache/hugegraph.git
git fetch upstream
git switch -c fix/server-prometheus-metric-name upstream/master
# 3. Make the code and test changes, then run the checks above
git add <changed-files>
git commit -m "fix(server): sanitize Prometheus metric names"
git push -u origin fix/server-prometheus-metric-name
```
Then open a PR from your fork to `apache/hugegraph:master` and use:
- PR title: `fix(server): sanitize Prometheus metric names`
- `Purpose of the PR`: `close #3142`
- `Main Changes`: explain the two endpoint paths and why `~` must be
sanitized
- `Verifying these changes`: list the exact test commands and your
successful Prometheus scrape result
- Mark `Doc - No Need` unless you also change user-facing
behavior/documentation
GitHub will run CI and the contributor/CLA checks. If CI fails or reviewers
request changes, update the same branch and push again; the PR will update
automatically. Multiple review rounds are normal, and maintainers will help you
refine the patch.
Your plan to keep documenting production findings, reproduction steps, and
fixes is exactly the kind of contribution that helps the project. Issues,
tests, documentation, and code fixes are all welcome. Thank you again, and we
look forward to your PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
