[ISSUE #494] support prometheus - #495
Conversation
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR modifies 9 file(s) with 607 lines of diff. No test changes detected — consider adding test coverage.
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [INFO]
metric-exporter/pom.xml:1— Large diff (607 lines). Consider breaking into smaller, focused PRs for easier review. (line outside diff)
| @@ -0,0 +1,234 @@ | |||
| /* | |||
There was a problem hiding this comment.
No test changes detected alongside source modifications. Consider adding tests to cover the changes.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
PR received and logged for review. This PR requires detailed code review by a maintainer.
Diff size: 607 lines
Author: Slideee (CONTRIBUTOR)
Automated review by RockteMQ-AI
| List<String> additionalLabelNames, List<String> additionalLabelValues, double value) { | ||
| String suffix = nameSuffix == null ? "" : nameSuffix; | ||
| List<String> labelValues = sanitizeLabelValues(dropwizardName); | ||
| return new Collector.MetricFamilySamples.Sample(sanitizeMetricName(dropwizardName + suffix), SOURCE_TASK_LABEL_NAMES, labelValues, value); |
There was a problem hiding this comment.
The createSample method completely ignores the additionalLabelNames and additionalLabelValues parameters. These carry the 'quantile' label (e.g., name="quantile", value="0.75") that differentiates histogram percentiles in Prometheus SUMMARY metrics. By always using SOURCE_TASK_LABEL_NAMES and discarding the additional labels, all percentile samples for a histogram end up with an identical metric name and label set, making them indistinguishable and causing Prometheus to reject or arbitrarily deduplicate them. The method should merge additionalLabelNames/additionalLabelValues into the output sample's label names and values.
| List<String> additionalLabelNames, List<String> additionalLabelValues, double value) { | ||
| String suffix = nameSuffix == null ? "" : nameSuffix; | ||
| List<String> labelValues = sanitizeLabelValues(dropwizardName); | ||
| return new Collector.MetricFamilySamples.Sample(sanitizeMetricName(dropwizardName + suffix), SOURCE_TASK_LABEL_NAMES, labelValues, value); |
There was a problem hiding this comment.
sanitizeMetricName(dropwizardName + suffix) appends the suffix to the full dropwizardName string before parsing, but sanitizeMetricName extracts only the 2nd comma-separated field (split(":")[1].split(",")[1]). The suffix (e.g., "_count") lands on the last field and is silently lost. This means the histogram count sample (which uses nameSuffix="_count") gets the same metric name as the percentile samples, producing conflicting samples with the same name and label set. The suffix should be appended to the extracted metric name, not to the raw input string.
| if (!StringUtils.equals(var3, "")) { | ||
| connectorName = var3.substring(var3.indexOf("=") + 1); | ||
| } | ||
| String var4 = split[4]; |
There was a problem hiding this comment.
sanitizeLabelValues performs unchecked array indexing on split(":") and split(",") results, accessing indices 0 through 4 (split[4]) without any bounds validation. If any metric in the registry has a name that does not match the expected 'prefix:group,name,type,connector=X,task=Y' format (e.g., metrics registered by the framework itself or third-party libraries), this will throw ArrayIndexOutOfBoundsException, crashing the entire /metrics endpoint. sanitizeMetricName (line 37) has the same issue. Consider validating the split array length or wrapping in a try-catch that skips malformed metric names.
| this.executor = Executors.newCachedThreadPool(); | ||
| this.connectMetrics = new ConnectMetrics(workerConfig); | ||
| this.stateManagementService = stateManagementService; | ||
| CollectorRegistry.defaultRegistry.register(new DropwizardExports(connectMetrics.registry(), new PrometheusSampleBuilder())); |
There was a problem hiding this comment.
DropwizardExports is registered with the global CollectorRegistry.defaultRegistry but is never unregistered when the Worker is stopped. On Worker restart (e.g., connector reconfiguration), registering the same collector type again will throw IllegalArgumentException('Collector already registered'), breaking the connector lifecycle. The registration should either use a dedicated CollectorRegistry (not the global singleton), or unregister the collector in the Worker's stop/shutdown method.
| this.connectController = connectController; | ||
| pluginsResource = new ConnectorPluginsResource(connectController); | ||
|
|
||
| Javalin embeddedApp = Javalin.create(config -> { |
There was a problem hiding this comment.
The Javalin 'embeddedApp' instance for the metrics server is a local variable in the constructor and is never stored as a field. This means there is no way to stop or shut down the metrics Jetty server when the RestHandler or connect runtime is stopped, causing a port and thread resource leak. The embeddedApp reference should be stored as a field and stopped in the appropriate shutdown method.
| MetricFamilySamples fromSnapshotAndCount(String dropwizardName, Snapshot snapshot, long count, double factor, | ||
| String helpMessage) { | ||
| MetricName metricName = MetricUtils.stringToMetricName(dropwizardName); | ||
| Stat.HistogramType histogramType = Stat.HistogramType.valueOf(metricName.getType()); |
There was a problem hiding this comment.
Stat.HistogramType.valueOf(metricName.getType()) throws IllegalArgumentException if the metric name's type field does not match a known enum constant. Since this is called inside collect() which iterates over ALL metrics in the registry, a single metric with an unrecognized type will crash the entire metrics collection, making the /metrics endpoint return an error for all metrics. Consider wrapping this in a try-catch that skips the individual metric and logs a warning, similar to how fromGauge handles invalid types.
| samples = Arrays.asList(sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.999"), snapshot.get999thPercentile() * factor)); | ||
| break; | ||
| default: | ||
| samples = Arrays.asList(sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.5"), snapshot.getMedian() * factor), sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.5"), snapshot.getMedian() * factor), sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.75"), snapshot.get75thPercentile() * factor), sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.95"), snapshot.get95thPercentile() * factor), sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.98"), snapshot.get98thPercentile() * factor), sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.99"), snapshot.get99thPercentile() * factor), sampleBuilder.createSample(dropwizardName, "", Arrays.asList("quantile"), Arrays.asList("0.999"), snapshot.get999thPercentile() * factor), sampleBuilder.createSample(dropwizardName, "_count", new ArrayList<String>(), new ArrayList<String>(), count)); |
There was a problem hiding this comment.
The default case of the switch in fromSnapshotAndCount creates a duplicate sample: two samples both with quantile="0.5" and snapshot.getMedian(). The original Prometheus DropwizardExports only includes the median sample once. This duplicate should be removed — the second createSample call with Arrays.asList("0.5") and snapshot.getMedian() is redundant.
|
|
||
| } | ||
|
|
||
| private Set<String> parse(HttpServletRequest req) { |
There was a problem hiding this comment.
The parse(HttpServletRequest) method is dead code — it is never called anywhere in the class. It appears to be copied from an upstream Prometheus servlet implementation but was not wired into doGet. Either remove it or use it to support the name[] query parameter for filtering which metrics are returned.
| import java.util.List; | ||
| import org.apache.commons.lang3.StringUtils; | ||
|
|
||
| public class PrometheusSampleBuilder implements SampleBuilder { |
There was a problem hiding this comment.
No test coverage is added for any of the new classes (DropwizardExports, PrometheusSampleBuilder, PrometheusMetricsServlet). Given the complex string-parsing logic in PrometheusSampleBuilder and the metric-type dispatch in DropwizardExports, unit tests are especially important to verify correct behavior with well-formed and malformed metric names, and to prevent regressions in the quantile label and suffix handling.
|
Issue Evaluation Category: This issue references #494 and proposes adding Prometheus monitoring support. Note: This appears to be a PR submission. If you have implementation code ready, please submit it as a pull request directly. Feasibility: Adding Prometheus metrics exposure is a valuable enhancement for observability. Age: This issue is from May 2023. If this feature is still desired, please confirm or submit a PR. Automated evaluation by RockteMQ-AI |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR adds Prometheus metrics export support to the Connect HTTP connector. The overall architecture is sound — using Dropwizard as the metrics registry and exporting via Prometheus text format is a standard approach.
However, there is a critical parsing bug in PrometheusSampleBuilder that will crash on metric names without the expected name:tag format.
Findings
- [Critical]
PrometheusSampleBuilder.java:60—split(":")[1]andsplit(",")[1]lack bounds checking →ArrayIndexOutOfBoundsException - [Warning]
WorkerConfig.java:120— No validation onexporterPortrange
Suggestions
- Add defensive parsing in
PrometheusSampleBuilder.buildMetricName()to handle metric names that don't follow thename:tagconvention - Add port range validation for
exporterPort - Consider adding a unit test for
PrometheusSampleBuilderwith edge cases (no colon, no comma, empty name)
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [CRITICAL]
connectors/rocketmq-connect-http/src/main/java/org/apache/rocketmq/connect/http/metrics/PrometheusSampleBuilder.java:60— [Critical]name.split(":")[1]will throwArrayIndexOutOfBoundsExceptionif the metric name does not contain a colon (e.g., a plain metric name likemy_metric). Add a bounds check:
String[] parts = name.split(":");
String metricName = parts.length > 1 ? parts[1] : parts[0];Similarly, parts[1].split(",")[1] on line 61 can throw if there is no comma. Consider defensive parsing. (line outside diff)
- [WARNING]
connect/connector-runtime/src/main/java/org/apache/rocketmq/connect/runtime/WorkerConfig.java:120— [Warning]exporterPortdefaults to5557with no validation. Consider adding a port range check (1-65535) in the setter or during config initialization to prevent silent failures when an invalid port is configured. (line outside diff)
2 participants
What is the purpose of the change
#494
Brief changelog
XX
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily. Notice,
it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyleto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.