[CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans by senthh · Pull Request #3758 · apache/celeborn

senthh · 2026-07-14T05:31:07Z

What changes were proposed in this pull request?

This PR adds a new JmxSink metrics sink that exposes Celeborn metrics as JMX MBeans.

Add JmxSink (common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala), backed by Dropwizard Metrics' JmxReporter. It follows the existing Sink contract and is loaded reflectively by MetricsSystem via the standard (Properties, MetricRegistry) constructor, so no changes to MetricsSystem are required.
Add the io.dropwizard.metrics:metrics-jmx dependency (which contains JmxReporter in Dropwizard Metrics 4.x) to both the Maven build (pom.xml dependency management + common/pom.xml) and the SBT build (project/CelebornBuild.scala), pinned to the existing ${codahale.metrics.version} (4.2.25).
Add a commented-out example to conf/metrics.properties.template and charts/celeborn/files/conf/metrics.properties showing how to enable the sink.
Document JmxSink in docs/monitoring.md.
Register metrics-jmx-4.2.25.jar in all dev/deps/dependencies-* manifests (server + client profiles) and add io.dropwizard.metrics:metrics-jmx to LICENSE-binary, since the artifact is now bundled in the distributions.

Enabling the sink is opt-in:

*.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink


Test Output:

<img width="1315" height="800" alt="Celeborn_jmx" src="https://github.com/user-attachments/assets/7af729f8-7eb9-4749-af9c-e2af64d48fb8" />

Copilot

Pull request overview

This PR adds a new metrics sink (JmxSink) to expose Celeborn metrics as JMX MBeans using Dropwizard Metrics’ JmxReporter, along with the required build/distribution updates so the sink can be enabled via standard metrics.properties configuration.

Changes:

Add JmxSink implementation backed by Dropwizard JmxReporter.
Add io.dropwizard.metrics:metrics-jmx to Maven + SBT builds and update binary licensing metadata.
Update config templates, Helm chart config, and monitoring docs to document/illustrate enabling the sink.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
project/CelebornBuild.scala	Adds `metrics-jmx` to SBT dependencies for the common module.
pom.xml	Adds `metrics-jmx` to Maven dependency management.
common/pom.xml	Adds `metrics-jmx` as a common-module dependency (version via parent management).
common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala	Introduces the new `JmxSink` implementation using `JmxReporter`.
conf/metrics.properties.template	Adds commented example to enable the JMX sink.
charts/celeborn/files/conf/metrics.properties	Adds commented example to enable the JMX sink in Helm chart config.
docs/monitoring.md	Documents `JmxSink` in the list of supported sinks.
LICENSE-binary	Registers `io.dropwizard.metrics:metrics-jmx` in the binary license list.
dev/deps/dependencies-server	Adds `metrics-jmx-4.2.25.jar` to the server dependency manifest.
dev/deps/dependencies-client-tez	Adds `metrics-jmx-4.2.25.jar` to the Tez client dependency manifest.
dev/deps/dependencies-client-mr	Adds `metrics-jmx-4.2.25.jar` to the MR client dependency manifest.
dev/deps/dependencies-client-spark-4.1	Adds `metrics-jmx-4.2.25.jar` to the Spark 4.1 client dependency manifest.
dev/deps/dependencies-client-spark-4.0	Adds `metrics-jmx-4.2.25.jar` to the Spark 4.0 client dependency manifest.
dev/deps/dependencies-client-spark-3.5	Adds `metrics-jmx-4.2.25.jar` to the Spark 3.5 client dependency manifest.
dev/deps/dependencies-client-spark-3.4	Adds `metrics-jmx-4.2.25.jar` to the Spark 3.4 client dependency manifest.
dev/deps/dependencies-client-spark-3.3	Adds `metrics-jmx-4.2.25.jar` to the Spark 3.3 client dependency manifest.
dev/deps/dependencies-client-spark-3.2	Adds `metrics-jmx-4.2.25.jar` to the Spark 3.2 client dependency manifest.
dev/deps/dependencies-client-spark-3.1	Adds `metrics-jmx-4.2.25.jar` to the Spark 3.1 client dependency manifest.
dev/deps/dependencies-client-spark-3.0	Adds `metrics-jmx-4.2.25.jar` to the Spark 3.0 client dependency manifest.
dev/deps/dependencies-client-flink-2.3	Adds `metrics-jmx-4.2.25.jar` to the Flink 2.3 client dependency manifest.
dev/deps/dependencies-client-flink-2.2	Adds `metrics-jmx-4.2.25.jar` to the Flink 2.2 client dependency manifest.
dev/deps/dependencies-client-flink-2.1	Adds `metrics-jmx-4.2.25.jar` to the Flink 2.1 client dependency manifest.
dev/deps/dependencies-client-flink-2.0	Adds `metrics-jmx-4.2.25.jar` to the Flink 2.0 client dependency manifest.
dev/deps/dependencies-client-flink-1.20	Adds `metrics-jmx-4.2.25.jar` to the Flink 1.20 client dependency manifest.
dev/deps/dependencies-client-flink-1.19	Adds `metrics-jmx-4.2.25.jar` to the Flink 1.19 client dependency manifest.
dev/deps/dependencies-client-flink-1.18	Adds `metrics-jmx-4.2.25.jar` to the Flink 1.18 client dependency manifest.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

SteNicholas · 2026-07-15T02:35:14Z

@senthh, thanks for first contribution. Please fix failure of Integration Test / celeborn_integration_test.

senthh · 2026-07-15T11:17:09Z

@senthh, thanks for first contribution. Please fix failure of Integration Test / celeborn_integration_test.

@SteNicholas Thanks for the response and yes I have fixed the Integration test failures

Copilot

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.

codecov · 2026-07-15T18:40:54Z

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.22%. Comparing base (1e715b2) to head (205be0d).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
.../apache/celeborn/common/metrics/sink/JmxSink.scala	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3758      +/-   ##
============================================
+ Coverage     58.05%   58.22%   +0.18%     
  Complexity      228      228              
============================================
  Files           397      398       +1     
  Lines         27931    27955      +24     
  Branches       2725     2728       +3     
============================================
+ Hits          16212    16275      +63     
+ Misses        10530    10488      -42     
- Partials       1189     1192       +3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

SteNicholas · 2026-07-16T02:47:18Z

@senthh, please rebase the latest main branch and add change in dev/deps/dependencies-client-spark-4.2 file.

Copilot

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…d MBean lifecycle Add a JmxSinkSuite (extending the existing sink-level test coverage) that verifies JmxSink is loaded by MetricsSystem from the metrics configuration via its reflection-based (Properties, MetricRegistry) constructor, and that start()/stop() register and unregister metric MBeans in the platform MBeanServer under the "metrics" JMX domain.

…metrics config Adding the commented JmxSink line to charts/celeborn/files/conf/metrics.properties changes the rendered config, and therefore the celeborn.apache.org/conf-hash checksum annotation on the master and worker StatefulSets. Update the expected hashes in the helm-unittest suites accordingly.

…, spark-4.2 dep - Publish MBeans under a configurable, Celeborn-specific JMX domain (default "celeborn") via `*.sink.jmx.domain`, instead of JmxReporter's global default "metrics" domain, to avoid MBean collisions with other Dropwizard users in the same JVM. - Ensure JmxSinkSuite always stops the MetricsSystem/reporter via try/finally, and add coverage for the configured-domain path and report(). - Add metrics-jmx to dev/deps/dependencies-client-spark-4.2 (new profile on main).

senthh · 2026-07-16T18:11:08Z

@senthh, please rebase the latest main branch and add change in dev/deps/dependencies-client-spark-4.2 file.

@SteNicholas I have rebased and addressed the integration test failure and code coverage issue too now

Copilot

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

…obal MBeanServer Use a unique JMX domain per run and assert on the before/after MBean delta instead of absolute presence, so the test is not affected by MBeans already present in (or leaked by a prior failed run into) the JVM-global MBeanServer.

senthh · 2026-07-19T03:51:04Z

@SteNicholas Hi Nicol, I have addressed Co-pilot's review

SteNicholas

@senthh, thanks for updates. I left some comments for the updates. PTAL.

Copilot

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

… a space Format the disabled JmxSink example as `# *.sink.jmx.class=...` (space after the comment marker) in conf/metrics.properties.template and the Helm chart metrics.properties, per review. Update the master/worker StatefulSet conf-hash test expectations for the changed chart config.

senthh · 2026-07-21T06:23:11Z

@senthh, thanks for updates. I left some comments for the updates. PTAL.

@SteNicholas Thanks for the reviews, I have addressed the points which you mentioned. It will be helpful if you re-review and share me your further input/feedback so that I can work to resolve.

Copilot

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

service/src/test/scala/org/apache/celeborn/server/common/metrics/sink/JmxSinkSuite.scala:53

In this test, metricsSystem.start(true) is called before the try/finally. If start() throws (e.g., due to a bad metrics config or missing dependency), stop() won’t run and can leak sinks/MBeans into subsequent tests in the same JVM.

Since the JmxSink ships in 0.7.0 (the main branch version), enable it by default in the Helm chart's metrics.properties alongside the other sinks instead of leaving it commented out. Update the master/worker StatefulSet conf-hash test expectations accordingly.

senthh · 2026-07-22T07:09:22Z

@SteNicholas Thanks for approving this PR;

I see that the spark4 4.2 failure is an infra issue — 'the hosted runner lost communication with the server' — unrelated to this change; could you re-run the failed job?

github-actions Bot added kind:build kind:deploy kind:documentation kind:infra module:common labels Jul 14, 2026

SteNicholas requested a review from Copilot July 14, 2026 06:09

Copilot started reviewing on behalf of SteNicholas July 14, 2026 06:09 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala Outdated

SteNicholas requested a review from Copilot July 14, 2026 12:38

Copilot started reviewing on behalf of SteNicholas July 14, 2026 12:39 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala

github-actions Bot added the module:service label Jul 15, 2026

SteNicholas requested a review from Copilot July 15, 2026 17:33

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Comment thread common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala Outdated

Comment thread service/src/test/scala/org/apache/celeborn/server/common/metrics/sink/JmxSinkSuite.scala Outdated

Copilot started reviewing on behalf of SteNicholas July 15, 2026 17:58 View session

SteNicholas requested a review from Copilot July 16, 2026 03:03

Copilot started reviewing on behalf of SteNicholas July 16, 2026 03:03 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

senthh and others added 5 commits July 16, 2026 23:34

[CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans

20e9a18

Potential fix for pull request finding

c6392e2

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

senthh force-pushed the CELEBORN-2380 branch from 205be0d to 5ceeadc Compare July 16, 2026 18:08

SteNicholas requested a review from Copilot July 17, 2026 07:53

Copilot started reviewing on behalf of SteNicholas July 17, 2026 07:54 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread service/src/test/scala/org/apache/celeborn/server/common/metrics/sink/JmxSinkSuite.scala Outdated

SteNicholas reviewed Jul 20, 2026

View reviewed changes

Comment thread charts/celeborn/files/conf/metrics.properties Outdated

SteNicholas reviewed Jul 20, 2026

View reviewed changes

Comment thread conf/metrics.properties.template Outdated

SteNicholas reviewed Jul 20, 2026

View reviewed changes

Comment thread charts/celeborn/files/conf/metrics.properties Outdated

SteNicholas reviewed Jul 20, 2026

View reviewed changes

SteNicholas requested a review from Copilot July 20, 2026 02:51

Copilot started reviewing on behalf of SteNicholas July 20, 2026 02:51 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

SteNicholas requested a review from Copilot July 21, 2026 08:05

Copilot started reviewing on behalf of SteNicholas July 21, 2026 08:05 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

SteNicholas reviewed Jul 21, 2026

View reviewed changes

Comment thread charts/celeborn/files/conf/metrics.properties Outdated

SteNicholas approved these changes Jul 22, 2026

View reviewed changes

Uh oh!

Conversation

senthh commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

SteNicholas commented Jul 15, 2026

Uh oh!

senthh commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 15, 2026

Codecov Report

Uh oh!

SteNicholas commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

senthh commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

senthh commented Jul 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SteNicholas left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

senthh commented Jul 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

senthh commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

senthh commented Jul 14, 2026 •

edited

Loading