Skip to content

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

Open
senthh wants to merge 6 commits into
apache:mainfrom
senthh:CELEBORN-2380
Open

[CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans#3758
senthh wants to merge 6 commits into
apache:mainfrom
senthh:CELEBORN-2380

Conversation

@senthh

@senthh senthh commented Jul 14, 2026

Copy link
Copy Markdown

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 AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@SteNicholas

Copy link
Copy Markdown
Member

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

@senthh

senthh commented Jul 15, 2026

Copy link
Copy Markdown
Author

@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

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

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

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

senthh and others added 5 commits July 16, 2026 23:34
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

senthh commented Jul 16, 2026

Copy link
Copy Markdown
Author

@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

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

senthh commented Jul 19, 2026

Copy link
Copy Markdown
Author

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants