You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "senthh (via GitHub)" <gi...@apache.org> on 2026/07/14 05:31:10 UTC
[PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
senthh opened a new pull request, #3758:
URL: https://github.com/apache/celeborn/pull/3758
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### 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:
```properties
*.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Test Output:
<img width="2630" height="1600" alt="image" src="https://github.com/user-attachments/assets/673aff72-dce0-4038-b11f-a66a6a88a4b4" />
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3579035407
##########
common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.metrics.sink
+
+import java.util.Properties
+
+import com.codahale.metrics.MetricRegistry
+import com.codahale.metrics.jmx.JmxReporter
+
+class JmxSink(val property: Properties, val registry: MetricRegistry) extends Sink {
+
+ val reporter: JmxReporter = JmxReporter.forRegistry(registry).build()
+
+ override def start(): Unit = {
+ reporter.start()
+ }
+
+ override def stop(): Unit = {
+ reporter.stop()
+ }
Review Comment:
Add a unit test to ensure JmxSink can be loaded by MetricsSystem from metrics configuration and that start()/stop() correctly register/unregister MBeans in the platform MBeanServer. There is existing sink-level test coverage (e.g., LoggerSinkSuite) that can be extended to cover this new sink and prevent regressions in reflection-based loading and lifecycle handling.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3613038926
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
Review Comment:
> Which version will this sink be enabled?
@SteNicholas We tested this with 0.6.0 and 0.6.2; And hence this sink should be working for other 0.6* too.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-4984111271
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3758?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `80.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 58.22%. Comparing base ([`1e715b2`](https://app.codecov.io/gh/apache/celeborn/commit/1e715b2cf2936a3704c6a44da463628f7e737f35?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`205be0d`](https://app.codecov.io/gh/apache/celeborn/commit/205be0d741f1d04079e61f86c065f0b0eb6a0e3c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/celeborn/pull/3758?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../apache/celeborn/common/metrics/sink/JmxSink.scala](https://app.codecov.io/gh/apache/celeborn/pull/3758?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fmetrics%2Fsink%2FJmxSink.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL21ldHJpY3Mvc2luay9KbXhTaW5rLnNjYWxh) | 80.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/3758?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/celeborn/pull/3758?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3613038926
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
Review Comment:
> Which version will this sink be enabled?
@SteNicholas We tested this with 0.6.2 and 0.7.0-SNAPSHOT one; And hence it is good to add this feature in upcoming release 0.7.0
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3589577295
##########
common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.metrics.sink
+
+import java.util.Properties
+
+import com.codahale.metrics.MetricRegistry
+import com.codahale.metrics.jmx.JmxReporter
+
+class JmxSink(val property: Properties, val registry: MetricRegistry) extends Sink {
+
+ val reporter: JmxReporter = JmxReporter.forRegistry(registry).build()
Review Comment:
Using `JmxReporter` with default settings publishes into the global JMX domain `\"metrics\"`. If multiple Celeborn components/MetricRegistries run in the same JVM (or multiple tests run in-process), this can cause MBean name collisions and cross-component interference. Consider allowing the JMX domain (and optionally the MBeanServer) to be configured via `Properties`, and defaulting to a Celeborn-specific domain (or one derived from the metrics namespace/instance name) using `JmxReporter.Builder.inDomain(...)`.
##########
service/src/test/scala/org/apache/celeborn/server/common/metrics/sink/JmxSinkSuite.scala:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.server.common.metrics.sink
+
+import java.lang.management.ManagementFactory
+import javax.management.ObjectName
+
+import scala.collection.JavaConverters._
+
+import org.apache.celeborn.CelebornFunSuite
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.metrics.MetricsSystem
+import org.apache.celeborn.common.metrics.sink.JmxSink
+import org.apache.celeborn.common.metrics.source.JVMSource
+import org.apache.celeborn.common.network.TestHelper
+
+class JmxSinkSuite extends CelebornFunSuite {
+
+ private def newMetricsSystem(): MetricsSystem = {
+ val celebornConf = new CelebornConf()
+ celebornConf
+ .set(CelebornConf.METRICS_ENABLED.key, "true")
+ .set(
+ CelebornConf.METRICS_CONF.key,
+ TestHelper.getResourceAsAbsolutePath("/metrics-jmx.properties"))
+ val metricsSystem = MetricsSystem.createMetricsSystem("test", celebornConf)
+ metricsSystem.registerSource(new JVMSource(celebornConf, "test"))
+ metricsSystem
+ }
+
+ test("test load jmx sink case") {
+ val metricsSystem = newMetricsSystem()
+ metricsSystem.start(true)
+
+ try {
+ // JmxSink has no dedicated branch in MetricsSystem.registerSinks, so it must be
+ // instantiated reflectively via its (Properties, MetricRegistry) constructor.
+ assert(metricsSystem.sinks.exists(_.isInstanceOf[JmxSink]))
+ } finally {
+ metricsSystem.stop()
+ }
+ }
+
+ test("test jmx sink registers and unregisters MBeans lifecycle case") {
+ val metricsSystem = newMetricsSystem()
+ val mBeanServer = ManagementFactory.getPlatformMBeanServer
+ // JmxReporter publishes metrics under the "metrics" JMX domain by default.
+ val jmxDomainPattern = new ObjectName("metrics:*")
+
+ val beforeStart = mBeanServer.queryNames(jmxDomainPattern, null).asScala
+
+ // start() runs registerSinks() (reflection-based loading) followed by Sink.start(),
+ // which makes the JmxSink's JmxReporter register the registry metrics as MBeans.
+ metricsSystem.start(true)
+ val afterStart = mBeanServer.queryNames(jmxDomainPattern, null).asScala
+ val registeredByJmxSink = afterStart -- beforeStart
+
+ metricsSystem.stop()
+ val afterStop = mBeanServer.queryNames(jmxDomainPattern, null).asScala
Review Comment:
This test does not guarantee `metricsSystem.stop()` runs if `start(true)` or the intermediate JMX queries throw, which can leak a running reporter/MBeans into other tests and cause flakes. Wrap the start/query/assert sequence in a `try` and call `metricsSystem.stop()` in a `finally` to ensure cleanup on failures.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3611771410
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+#*.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
Which version would this sink enable?
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+#*.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
Which version would this sink enable?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3611769237
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+#*.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
```suggestion
# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
```
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-5014113156
@SteNicholas Hi Nicol, I have addressed Co-pilot's review
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #3758: [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans
URL: https://github.com/apache/celeborn/pull/3758
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3623619942
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
Yes @SteNicholas ; Do you want me to un-comment below line?
"# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink"
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-5042949119
@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?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-5065463300
@senthh, thanks for first contribution. Merged to main(v0.7.0).
BTW, if your company uses celeborn on production, please comment in #2140.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-5030806896
> @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.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3586751393
##########
common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.metrics.sink
+
+import java.util.Properties
+
+import com.codahale.metrics.MetricRegistry
+import com.codahale.metrics.jmx.JmxReporter
+
+class JmxSink(val property: Properties, val registry: MetricRegistry) extends Sink {
+
+ val reporter: JmxReporter = JmxReporter.forRegistry(registry).build()
+
+ override def start(): Unit = {
+ reporter.start()
+ }
+
+ override def stop(): Unit = {
+ reporter.stop()
+ }
Review Comment:
Yes added Unit test
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3611772168
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
Review Comment:
Which version will this sink be enabled?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3576457196
##########
common/src/main/scala/org/apache/celeborn/common/metrics/sink/JmxSink.scala:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.metrics.sink
+
+import java.util.Properties
+
+import com.codahale.metrics.MetricRegistry
+import com.codahale.metrics.jmx.JmxReporter
+
+private class JmxSink(val property: Properties, val registry: MetricRegistry) extends Sink {
Review Comment:
`MetricsSystem` instantiates generic sinks via `Utils.classForName(classPath).getConstructor(classOf[Properties], classOf[MetricRegistry])` (i.e., it requires a *public* `(Properties, MetricRegistry)` constructor). Declaring this as a `private class` makes the primary constructor non-public at the JVM level, so enabling `*.sink.jmx.class=...JmxSink` is likely to fail at runtime with `NoSuchMethodException` / `IllegalAccessException` when the sink is reflectively created.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3611769637
##########
conf/metrics.properties.template:
##########
@@ -18,3 +18,8 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+# MBeans are published under the `celeborn` JMX domain by default; override with
+# `*.sink.jmx.domain=<domain>` if needed.
+#*.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
```suggestion
# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
```
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3621566439
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
Therefore, this should not be commented for 0.7.0, right? The version of main branch is 0.7.0.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-4976300918
@senthh, thanks for first contribution. Please fix failure of Integration Test / celeborn_integration_test.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-4995137152
> @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
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3623619942
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
Yes @SteNicholas ; Do you want me to un-comment below line?
# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-5061437662
@SteNicholas @RexXiong Thank you for approving this 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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-4979922458
> @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
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3601395760
##########
service/src/test/scala/org/apache/celeborn/server/common/metrics/sink/JmxSinkSuite.scala:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.server.common.metrics.sink
+
+import java.lang.management.ManagementFactory
+import java.util.Properties
+import javax.management.ObjectName
+
+import scala.collection.JavaConverters._
+
+import com.codahale.metrics.MetricRegistry
+
+import org.apache.celeborn.CelebornFunSuite
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.metrics.MetricsSystem
+import org.apache.celeborn.common.metrics.sink.JmxSink
+import org.apache.celeborn.common.metrics.source.JVMSource
+import org.apache.celeborn.common.network.TestHelper
+
+class JmxSinkSuite extends CelebornFunSuite {
+
+ private def newMetricsSystem(): MetricsSystem = {
+ val celebornConf = new CelebornConf()
+ celebornConf
+ .set(CelebornConf.METRICS_ENABLED.key, "true")
+ .set(
+ CelebornConf.METRICS_CONF.key,
+ TestHelper.getResourceAsAbsolutePath("/metrics-jmx.properties"))
+ val metricsSystem = MetricsSystem.createMetricsSystem("test", celebornConf)
+ metricsSystem.registerSource(new JVMSource(celebornConf, "test"))
+ metricsSystem
+ }
+
+ test("test load jmx sink case") {
+ val metricsSystem = newMetricsSystem()
+ metricsSystem.start(true)
+
+ try {
+ // JmxSink has no dedicated branch in MetricsSystem.registerSinks, so it must be
+ // instantiated reflectively via its (Properties, MetricRegistry) constructor.
+ assert(metricsSystem.sinks.exists(_.isInstanceOf[JmxSink]))
+ } finally {
+ metricsSystem.stop()
+ }
+ }
+
+ test("test jmx sink registers and unregisters MBeans lifecycle case") {
+ val metricsSystem = newMetricsSystem()
+ val mBeanServer = ManagementFactory.getPlatformMBeanServer
+ // JmxSink publishes metrics under the Celeborn-specific default domain.
+ val jmxDomainPattern = new ObjectName(s"${JmxSink.JMX_DEFAULT_DOMAIN}:*")
+
+ val beforeStart = mBeanServer.queryNames(jmxDomainPattern, null).asScala
+ var registeredByJmxSink = Set.empty[ObjectName]
+
+ try {
+ // start() runs registerSinks() (reflection-based loading) followed by Sink.start(),
+ // which makes the JmxSink's JmxReporter register the registry metrics as MBeans.
+ metricsSystem.start(true)
+ // report() is a no-op for JmxSink (JmxReporter reports on registration) but must be safe.
+ metricsSystem.report()
+
+ val afterStart = mBeanServer.queryNames(jmxDomainPattern, null).asScala
+ registeredByJmxSink = (afterStart -- beforeStart).toSet
+
+ assert(metricsSystem.sinks.exists(_.isInstanceOf[JmxSink]))
+ assert(
+ registeredByJmxSink.nonEmpty,
+ s"JmxSink.start() should register metric MBeans under the " +
+ s"'${JmxSink.JMX_DEFAULT_DOMAIN}' domain")
+ } finally {
+ metricsSystem.stop()
+ }
+
+ val afterStop = mBeanServer.queryNames(jmxDomainPattern, null).asScala
+ assert(
+ registeredByJmxSink.forall(name => !afterStop.contains(name)),
+ "JmxSink.stop() should unregister the MBeans it registered")
+ }
+
+ test("test jmx sink honors configured domain case") {
+ val domain = "celeborn-jmx-test-domain"
+ val properties = new Properties()
+ properties.setProperty(JmxSink.JMX_DOMAIN_KEY, domain)
+ val registry = new MetricRegistry()
+ registry.counter("test-counter").inc()
+
+ val sink = new JmxSink(properties, registry)
+ assert(sink.domain == domain)
+
+ val mBeanServer = ManagementFactory.getPlatformMBeanServer
+ val jmxDomainPattern = new ObjectName(s"$domain:*")
+
+ try {
+ sink.start()
+ sink.report()
+ assert(
+ mBeanServer.queryNames(jmxDomainPattern, null).asScala.nonEmpty,
+ s"JmxSink should register MBeans under the configured '$domain' domain")
+ } finally {
+ sink.stop()
+ }
+
+ assert(
+ mBeanServer.queryNames(jmxDomainPattern, null).isEmpty,
+ "JmxSink.stop() should unregister the MBeans under the configured domain")
Review Comment:
The test uses a fixed JMX domain ("celeborn-jmx-test-domain") and asserts `queryNames(...).nonEmpty` after start. Because the platform MBeanServer is global to the JVM, this can become flaky if that domain already has MBeans (false positive) or if a prior failed run left MBeans behind (start may fail due to name collisions). Use a unique domain per test run and assert on the delta between before/after snapshots instead.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#issuecomment-4987658658
@senthh, please rebase the latest main branch and add change in `dev/deps/dependencies-client-spark-4.2` file.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3621566439
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
Therefore, this should not be commented for 0.7.0, right?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2380] Support JmxSink for exposing metrics as JMX MBeans [celeborn]
Posted by "senthh (via GitHub)" <gi...@apache.org>.
senthh commented on code in PR #3758:
URL: https://github.com/apache/celeborn/pull/3758#discussion_r3623619942
##########
charts/celeborn/files/conf/metrics.properties:
##########
@@ -18,3 +18,6 @@
*.sink.prometheusServlet.class=org.apache.celeborn.common.metrics.sink.PrometheusServlet
*.sink.jsonServlet.class=org.apache.celeborn.common.metrics.sink.JsonServlet
*.sink.loggerSink.class=org.apache.celeborn.common.metrics.sink.LoggerSink
+
+# Expose metrics as JMX MBeans. Disabled by default; uncomment to enable.
+# *.sink.jmx.class=org.apache.celeborn.common.metrics.sink.JmxSink
Review Comment:
Yes @SteNicholas ; it's now uncommented for 0.7.0
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org