[CELEBORN-1122] Metrics supports json format #2089

RuiQin7 · 2023-11-11T11:41:21Z

What changes were proposed in this pull request?

If the user does not use prometheus to collect monitoring metrics, but rather some other ones. Using metrics in JSON format would be more user-friendly.The PR supports JSON format for metrics.

Why are the changes needed?

Ditto.

Does this PR introduce any user-facing change?

Metrics supports JSON format

How was this patch tested?

Cluster test.

codecov · 2023-11-11T11:55:03Z

Codecov Report

Attention: 308 lines in your changes are missing coverage. Please review.

Comparison is base (1c7cd1b) 47.13% compared to head (af01170) 46.22%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...che/celeborn/common/metrics/sink/JsonServlet.scala	0.00%	252 Missing ⚠️
...apache/celeborn/common/metrics/MetricsSystem.scala	0.00%	25 Missing ⚠️
...leborn/server/common/http/HttpRequestHandler.scala	0.00%	17 Missing ⚠️
...celeborn/common/metrics/sink/AbstractServlet.scala	0.00%	9 Missing ⚠️
...leborn/common/metrics/sink/PrometheusServlet.scala	0.00%	3 Missing ⚠️
...cala/org/apache/celeborn/common/CelebornConf.scala	92.86%	1 Missing ⚠️
...rg/apache/celeborn/server/common/HttpService.scala	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2089      +/-   ##
==========================================
- Coverage   47.13%   46.22%   -0.91%     
==========================================
  Files         173      175       +2     
  Lines       11034    11336     +302     
  Branches     1011     1016       +5     
==========================================
+ Hits         5200     5239      +39     
- Misses       5494     5757     +263     
  Partials      340      340

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

RexXiong · 2023-11-24T02:20:24Z

common/src/main/scala/org/apache/celeborn/common/metrics/MetricsSystem.scala

+                sources.asScala,
+                jsonServletPath).asInstanceOf[JsonServlet])
+            }
+            if (kv._1 == "prometheusServlet") {


nit: else if

nit: else if

@RexXiong Thank you for your suggestion.

common/src/main/scala/org/apache/celeborn/common/metrics/sink/JsonServlet.scala

RexXiong · 2023-11-24T03:42:34Z

common/src/main/scala/org/apache/celeborn/common/metrics/sink/JsonServlet.scala

+
+  private def updateInnerMetrics(absSource: AbstractSource, metricData: MetricData): Unit = {
+    innerMetrics.synchronized {
+      if (innerMetrics.size() >= absSource.metricsCapacity) {


if condition meet, can we directly return? as remove and add new metricData seems useless

if condition meet, can we directly return? as remove and add new metricData seems useless

@RexXiong Thank you for your suggestion. I have resubmitted, please help me review it again.

RexXiong · 2023-11-28T05:41:49Z

common/src/main/scala/org/apache/celeborn/common/metrics/sink/JsonServlet.scala

+
+import com.codahale.metrics.MetricRegistry
+import io.netty.channel.ChannelHandler.Sharable
+import org.apache.hadoop.shaded.com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility


I have no other comments on this pr except this import as it from shaded dependency, WDYT @waitinfuture @pan3793 @FMX @cfmcgrady

I have no other comments on this pr except this import as it from shaded dependency, WDYT @waitinfuture @pan3793 @FMX @cfmcgrady

@RexXiong Thank you for your suggestion. I reimport the jackson package,and the version from hadoop 3.2.4.

pan3793 · 2023-11-28T07:24:02Z

pom.xml

@@ -96,6 +96,8 @@
    <zstd-jni.version>1.5.2-1</zstd-jni.version>
    <kubernetes-client.version>6.7.0</kubernetes-client.version>
    <rocksdbjni.version>8.5.3</rocksdbjni.version>
+    <jackson.version>2.10.5</jackson.version>


why choose such a low version

why choose such a low version

@pan3793
The version of org.apache.hadoop.shaded.*.jackson comes from Hadoop 3.2.4 now. In order to reduce its impact on the original framework. I directly import the jackson 2.10.5 from Hadoop 3.2.4.

pan3793 · 2023-11-28T07:27:16Z

please update the LICENSE and NOTICE since it introduces the new dependencies, also, fix sbt building system

RuiQin7 · 2023-11-30T05:14:24Z

please update the LICENSE and NOTICE since it introduces the new dependencies, also, fix sbt building system

@pan3793 Thank you. I have updated newer version , and fixed sbt building system. Please help me review it again.

pan3793 · 2023-11-30T06:10:29Z

project/CelebornBuild.scala

@@ -56,6 +56,8 @@ object Dependencies {
  val ratisVersion = "2.5.1"
  val roaringBitmapVersion = "0.9.32"
  val rocksdbJniVersion = "8.5.3"
+  val jacksonVersion = "2.15.3"
+  val paranamerVersion = "2.8"


seems we don't need to manage paranamerVersion explicitly, let's simply delegate it to jackson

seems we don't need to manage paranamerVersion explicitly, let's simply delegate it to jackson

Thank you. I remove it.

pan3793 · 2023-11-30T06:11:56Z

common/src/main/scala/org/apache/celeborn/common/metrics/sink/JsonServlet.scala

+import org.apache.celeborn.common.metrics.source.{AbstractSource, NamedCounter, NamedGauge, NamedHistogram, NamedTimer, Source}
+
+object JsonConverter {
+  val mapper = new ObjectMapper() with ClassTagExtensions


I guess user may preview the json using browser, maybe we can print the pretty json string.

I guess user may preview the json using browser, maybe we can print the pretty json string.

@pan3793
Thank you. For users, they may preview the json using browser, but for metrics collection, we prefer to use a compact format. I can add the celeborn.metrics.json.pretty.enabled to control whether to enable pretty and default value true.

configurable SGTM, though I think the overhead is quite nit

pan3793 · 2023-11-30T06:15:42Z

service/src/main/scala/org/apache/celeborn/server/common/http/HttpRequestHandler.scala

+          val servletHttpRequestHandler = servletHttpRequestHandlers.find(servlet =>
+            uri == servlet.getServletPath())
+          if (servletHttpRequestHandler.nonEmpty) {
+            servletHttpRequestHandler.get.handleRequest(uri)


should we update it for JSON?

res.headers().set(HttpHeaderNames.CONTENT_TYPE, "text/plain; charset=UTF-8")

should we update it for JSON?

res.headers().set(HttpHeaderNames.CONTENT_TYPE, "text/plain; charset=UTF-8")

Thank you. You are right. I update it.

common/src/main/scala/org/apache/celeborn/common/metrics/MetricsSystem.scala

pan3793 · 2023-12-01T13:17:47Z

common/src/main/scala/org/apache/celeborn/common/metrics/sink/JsonServlet.scala

+    mapper.writeValueAsString(value)
+  }
+
+  def toJsonWithPretty(value: ArrayBuffer[Any]): String = {


Suggested change

def toJsonWithPretty(value: ArrayBuffer[Any]): String = {

def toPrettyJson(value: Any): String = {

Thank you. I update it.

pan3793 · 2023-12-01T13:17:55Z

common/src/main/scala/org/apache/celeborn/common/metrics/sink/JsonServlet.scala

+  val mapper = new ObjectMapper() with ClassTagExtensions
+  mapper.registerModule(DefaultScalaModule)
+
+  def toJson(value: ArrayBuffer[Any]): String = {


Suggested change

def toJson(value: ArrayBuffer[Any]): String = {

def toJson(value: Any): String = {

Thank you. I update it.

pan3793 · 2023-12-01T13:19:37Z

LICENSE-binary

+com.fasterxml.jackson.module:jackson-module-scala
+com.fasterxml.jackson.core:jackson-databind
+com.fasterxml.jackson.core:jackson-annotations
+com.fasterxml.jackson.core:jackson-core


LICENSE-binary reflects the final content of the binary release tarball, so the paranamer should be retained here.

LICENSE-binary reflects the final content of the binary release tarball, so the parameter should be retained here.

what is the parameter?

sorry, the paranamer

sorry, the paranamer

I have added it. Please review it again.

pan3793 · 2023-12-01T13:20:07Z

dev/deps/dependencies-client-spark-3.2

-hadoop-client-api/3.3.6//hadoop-client-api-3.3.6.jar
-hadoop-client-runtime/3.3.6//hadoop-client-runtime-3.3.6.jar
+guava/14.0.1//guava-14.0.1.jar
+hadoop-client-api/3.2.4//hadoop-client-api-3.2.4.jar


please rebase master and regenerate this files

Thank you. I update it.

RexXiong

LGTM, cc @pan3793 @cfmcgrady

pan3793 · 2023-12-05T02:22:55Z

service/src/main/scala/org/apache/celeborn/server/common/http/HttpRequestHandler.scala

@@ -47,21 +47,34 @@ class HttpRequestHandler(
    val uri = req.uri()
    val (path, parameters) = HttpUtils.parseUri(uri)
    val msg = HttpUtils.handleRequest(service, path, parameters)
-    val response = msg match {
+    val textType = "text/plain; charset=UTF-8"
+    val jsonType = "application/json; charset=UTF-8"


Suggested change

val jsonType = "application/json; charset=UTF-8"

val jsonType = "application/json"

https://www.rfc-editor.org/rfc/rfc8259.html

Note: No "charset" parameter is defined for this registration.
Adding one really has no effect on compliant recipients.

Thanks. I update it.

pan3793

LGTM, except for one minor thing

cfmcgrady

LGTM, except a nit

cfmcgrady · 2023-12-05T03:45:07Z

service/src/main/scala/org/apache/celeborn/server/common/http/HttpRequestHandler.scala

+          val servletHttpRequestHandler = servletHttpRequestHandlers.find(servlet =>
+            uri == servlet.getServletPath())
+          servletHttpRequestHandler match {
+            case Some(handler) => handler match {
+                case jsonHandler: JsonHttpRequestHandler =>
+                  (jsonHandler.handleRequest(uri), jsonType)
+                case _ => (handler.handleRequest(uri), textType)
+              }
+            case _ => (s"Unknown path $uri!", textType)
+          }


nit

Suggested change

val servletHttpRequestHandler = servletHttpRequestHandlers.find(servlet =>

uri == servlet.getServletPath())

servletHttpRequestHandler match {

case Some(handler) => handler match {

case jsonHandler: JsonHttpRequestHandler =>

(jsonHandler.handleRequest(uri), jsonType)

case _ => (handler.handleRequest(uri), textType)

}

case _ => (s"Unknown path $uri!", textType)

}

servletHttpRequestHandlers.find(servlet => uri == servlet.getServletPath())

.map {

case jsonHandler: JsonHttpRequestHandler =>

(jsonHandler.handleRequest(uri), jsonType)

case handler: ServletHttpRequestHandler =>

(handler.handleRequest(uri), textType)

}.getOrElse((s"Unknown path $uri!", textType))

Thanks. I update it.

RexXiong · 2023-12-06T01:22:49Z

Thank you all @suizhe007 @pan3793 @cfmcgrady, merge to main(v0.4.0)

RexXiong reviewed Nov 24, 2023

View reviewed changes

RexXiong reviewed Nov 28, 2023

View reviewed changes

pan3793 reviewed Nov 28, 2023

View reviewed changes

RuiQin7 force-pushed the CELEBORN-1122 branch from 4ce6f37 to bb084cc Compare November 29, 2023 08:45

pan3793 reviewed Nov 30, 2023

View reviewed changes

cfmcgrady reviewed Dec 1, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/metrics/MetricsSystem.scala Outdated Show resolved Hide resolved

RuiQin7 force-pushed the CELEBORN-1122 branch 3 times, most recently from b6b3685 to 381b8ba Compare December 1, 2023 12:30

pan3793 reviewed Dec 1, 2023

View reviewed changes

RuiQin7 force-pushed the CELEBORN-1122 branch 2 times, most recently from 47956ed to 1d9caba Compare December 2, 2023 09:38

RexXiong approved these changes Dec 5, 2023

View reviewed changes

pan3793 reviewed Dec 5, 2023

View reviewed changes

pan3793 approved these changes Dec 5, 2023

View reviewed changes

RuiQin7 force-pushed the CELEBORN-1122 branch from 1d9caba to c4c1a2d Compare December 5, 2023 02:48

cfmcgrady approved these changes Dec 5, 2023

View reviewed changes

[CELEBORN-1122] Metrics supports json format

af01170

RuiQin7 force-pushed the CELEBORN-1122 branch from c4c1a2d to af01170 Compare December 5, 2023 05:24

RexXiong closed this in 04a1e90 Dec 6, 2023

	def toJsonWithPretty(value: ArrayBuffer[Any]): String = {
	def toPrettyJson(value: Any): String = {

	def toJson(value: ArrayBuffer[Any]): String = {
	def toJson(value: Any): String = {

	val jsonType = "application/json; charset=UTF-8"
	val jsonType = "application/json"

[CELEBORN-1122] Metrics supports json format #2089

[CELEBORN-1122] Metrics supports json format #2089

Conversation

RuiQin7 commented Nov 11, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov bot commented Nov 11, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RuiQin7 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

pan3793 commented Nov 28, 2023

RuiQin7 commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RexXiong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 left a comment

Choose a reason for hiding this comment

cfmcgrady left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RexXiong commented Dec 6, 2023

codecov bot commented Nov 11, 2023 •

edited

Loading

RuiQin7 Nov 28, 2023 •

edited

Loading

pan3793 Dec 1, 2023 •

edited

Loading