KAFKA-4982: add listener tags to socket-server-metrics #3004

edoardocomar · 2017-05-09T19:16:12Z

Added tags

asfbot · 2017-05-09T20:26:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3693/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-05-09T20:39:34Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3683/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-09T20:42:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3687/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-05-10T17:57:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3707/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-10T17:57:17Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3717/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-05-10T17:59:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3711/
Test PASSed (JDK 7 and Scala 2.10).

ijuma

Thanks for the PR, seems fine, just left some minor comments. Can we write a simple test?

ijuma · 2017-05-11T11:28:22Z

core/src/main/scala/kafka/network/SocketServer.scala

@@ -400,7 +398,7 @@ private[kafka] class Processor(val id: Int,

  private val newConnections = new ConcurrentLinkedQueue[SocketChannel]()
  private val inflightResponses = mutable.Map[String, RequestChannel.Response]()
-  private val metricTags = Map("networkProcessor" -> id.toString).asJava
+  val metricTags = Map("protocol" -> securityProtocol.name, "listener" -> listenerName.value, "networkProcessor" -> id.toString).asJava


This line is a bit long.

ijuma · 2017-05-11T11:28:41Z

core/src/main/scala/kafka/network/SocketServer.scala

@@ -105,9 +99,13 @@ class SocketServer(val config: KafkaConfig, val metrics: Metrics, val time: Time
      }
    }

+    val allIoWaitRatioMetricNames = processors.map { p =>


We should probably move this to inside the Gauge.

addressing review comments

asfbot · 2017-05-11T15:04:50Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/3766/
Test FAILed (JDK 7 and Scala 2.11).

asfbot · 2017-05-11T15:13:03Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3756/
Test FAILed (JDK 8 and Scala 2.12).

added unit test and fixed test failure

asfbot · 2017-05-11T16:50:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/3771/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-05-11T16:51:32Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3761/
Test PASSed (JDK 8 and Scala 2.12).

ijuma · 2017-05-12T12:21:41Z

core/src/main/scala/kafka/network/SocketServer.scala

-  private val metricTags = Map("networkProcessor" -> id.toString).asJava
+  val metricTags = Map(
+    "protocol" -> securityProtocol.name, "listener" -> listenerName.value, "networkProcessor" -> id.toString
+    ).asJava

  newGauge("IdlePercent",


The KIP simply mentions SelectorMetrics (in Kafka Metrics), but you have also changed the tags for IdlePercent which is a Yammer Metric. Is that intentional?

Yes it was intentional, as "io-wait-ratio" is still a per-network-thread metric and JConsole was grouping it with KafkaMetrics, so I didn't spot it coming from a different registry.
IMHO it would look too odd to leave that just tagged with the processor thread number. I'll clarify the KIP

You need to update the compatibility section too. @junrao, is this OK?

@edoardocomar I had a closer look and I think you misunderstood the issue. io-wait-ratio is a Selector metric. The issue that I'm raising is with regards to IdlePercent:

newGauge("IdlePercent", new Gauge[Double] { def value = { Option(metrics.metric(metrics.metricName("io-wait-ratio", "socket-server-metrics", metricTags))).fold(0.0)(_.value) } }, metricTags.asScala

See the metricTags passed to it?

Thanks, got it !

It's still a per-network-processor-thread metric. That's why the coding was intentional,
though I did not notice the new MBeanName surfacing in a different place.

I prefer the tagging, but I still could live without it here and maintain compatibility.
Please advise, @ijuma @junrao

You didn't do the same for NetworkProcessorAvgIdlePercent though. My preference would be to leave the Yammer metrics alone and just add the tags to the underlying SelectorMetrics, but worth waiting for @junrao's opinion.

Edit: NetworkProcessorAvgIdlePercent is not per network process, so that's OK.

@ijuma On second thought, as the (per-processor) IdlePercent is an alias to the (per-processor) io-wait-ratio we could leave the former unchanged for compatibility. And do the multilevel tagging only in the latter.

Yes, that was my suggestion.

Jun also agreed that we should just add the new tag to the SelectorMetrics.

ijuma · 2017-05-12T12:23:35Z

core/src/test/scala/unit/kafka/network/SocketServerTest.scala

+
+    assertEquals(2, metricsNames.size)
+    assert(metricsNames.head.getMBeanName.endsWith("protocol=PLAINTEXT,listener=PLAINTEXT,networkProcessor=0"))
+    assert(metricsNames.last.getMBeanName.endsWith("protocol=TRACE,listener=TRACE,networkProcessor=1"))


In Kafka, we typically call it a security protocol, not just a protocol. The latter is clearly more concise though. @junrao, do you think protocol is OK?

as below we agreed to remove it, leaving only the listener tag

ijuma · 2017-05-12T14:00:45Z

core/src/main/scala/kafka/network/SocketServer.scala

-  private val metricTags = Map("networkProcessor" -> id.toString).asJava
+  val metricTags = Map(
+    "protocol" -> securityProtocol.name, "listener" -> listenerName.value, "networkProcessor" -> id.toString
+    ).asJava


Looking at the JMX screenshot, the order is important, so you should use a LinkedHashMap. Also, it seems like listener should be the first entry as it uniquely identifies the listener.

I had the protocol as first one, as when using a treeview representation, allows for the listeners to be shown as children.
In fact, protocol/listener/networkProcessor is the order from the most generic to the most specific tag.

That's true. I guess the ideal scenario in my mind would be to just have two levels: listener name and network processor. And the security protocol would be an attribute value. However, I'm not sure how easy it is to do that.

We could omit the securityProtocol altogether. Although that's another deviation from the KIP, as you say the listener is the main differentiator.
I started from the need to distinguish between metrics, but in our setup we have no listener labels, just protocols. That's why we initially came up with a 3-level tagging

Can't find a way to add a string value ... that could be a future enhancement ?

Yes, let's just remove the security protocol algother. Synced with Jun and he agrees that this is fine.

ijuma · 2017-05-12T17:28:15Z

Do you think you'll be able to update the PR today? If so, we can probably get it merged today.

removed protol tag, left yammer metric with older tagging

edoardocomar · 2017-05-12T17:47:38Z

Thanks for the detailed feedback. I've updated the PR

asfbot · 2017-05-12T18:35:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/3828/
Test FAILed (JDK 7 and Scala 2.11).

asfbot · 2017-05-12T18:38:37Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3816/
Test PASSed (JDK 8 and Scala 2.12).

ijuma

LGTM, I included a couple of minor changes as described below before merging.

ijuma · 2017-05-13T01:32:06Z

core/src/main/scala/kafka/network/SocketServer.scala

@@ -400,15 +398,16 @@ private[kafka] class Processor(val id: Int,

  private val newConnections = new ConcurrentLinkedQueue[SocketChannel]()
  private val inflightResponses = mutable.Map[String, RequestChannel.Response]()
-  private val metricTags = Map("networkProcessor" -> id.toString).asJava
+  val metricTags = SortedMap("listener" -> listenerName.value, "networkProcessor" -> id.toString).asJava


A LinkedHashMap is better because it makes it obvious that the order of declaration is important. With a SortedMap, adding a third key could change the order.

ijuma · 2017-05-13T01:32:29Z

core/src/test/scala/unit/kafka/network/SocketServerTest.scala

+    assertFalse(kafkaMetricNames.isEmpty)
+    val expectedListeners = Set("PLAINTEXT","TRACE")
+    kafkaMetricNames.foreach { kafkaMetricName => {
+        System.err.println(kafkaMetricName.tags)


This was probably not intentional.

oops thanks for spotting. Shame syserr/syout are't caught by something akin to a findbugs for Scala

ijuma reviewed May 11, 2017

View reviewed changes

edoardocomar added 3 commits May 11, 2017 15:12

KAFKA-4982: add protocol and listener tags to socket-server-metrics

d30dfea

KAFKA-4982: add protocol and listener tags to socket-server-metrics

e554632

KAFKA-4982: add protocol and listener tags to socket-server-metrics

add67c7

addressing review comments

edoardocomar force-pushed the KAFKA-4982 branch from 2d81841 to add67c7 Compare May 11, 2017 14:51

KAFKA-4982: add protocol and listener tags to socket-server-metrics

f576b9a

added unit test and fixed test failure

ijuma reviewed May 12, 2017

View reviewed changes

KAFKA-4982: add protocol and listener tags to socket-server-metrics

be0a24d

removed protol tag, left yammer metric with older tagging

edoardocomar changed the title ~~KAFKA-4982: add protocol and listener tags to socket-server-metrics~~ KAFKA-4982: add listener tags to socket-server-metrics May 12, 2017

ijuma approved these changes May 13, 2017

View reviewed changes

asfgit closed this in f21f8f2 May 13, 2017

edoardocomar deleted the KAFKA-4982 branch May 15, 2017 10:00

KAFKA-4982: add listener tags to socket-server-metrics #3004

KAFKA-4982: add listener tags to socket-server-metrics #3004

Conversation

edoardocomar commented May 9, 2017

asfbot commented May 9, 2017

asfbot commented May 9, 2017

asfbot commented May 9, 2017

asfbot commented May 10, 2017

asfbot commented May 10, 2017

asfbot commented May 10, 2017

ijuma left a 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

asfbot commented May 11, 2017

asfbot commented May 11, 2017

asfbot commented May 11, 2017

asfbot commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma May 12, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoardocomar May 12, 2017 • 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

ijuma commented May 12, 2017

edoardocomar commented May 12, 2017

asfbot commented May 12, 2017

asfbot commented May 12, 2017

ijuma left a 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

ijuma May 12, 2017 •

edited

Loading

ijuma May 12, 2017 •

edited

Loading

edoardocomar May 12, 2017 •

edited

Loading