[jira] [Commented] (KAFKA-6514) Add API version as a tag for the RequestsPerSec metric

2018-04-16 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/KAFKA-6514?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16439737#comment-16439737
 ] 

ASF GitHub Bot commented on KAFKA-6514:
---

hachikuji closed pull request #4506: KAFKA-6514: Add API version as a tag for 
the RequestsPerSec metric
URL: https://github.com/apache/kafka/pull/4506
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/core/src/main/scala/kafka/network/RequestChannel.scala 
b/core/src/main/scala/kafka/network/RequestChannel.scala
index 8a17528bfb7..f03bdeb9dd9 100644
--- a/core/src/main/scala/kafka/network/RequestChannel.scala
+++ b/core/src/main/scala/kafka/network/RequestChannel.scala
@@ -158,7 +158,7 @@ object RequestChannel extends Logging {
   val metricNames = fetchMetricNames :+ header.apiKey.name
   metricNames.foreach { metricName =>
 val m = metrics(metricName)
-m.requestRate.mark()
+m.requestRate(header.apiVersion).mark()
 m.requestQueueTimeHist.update(Math.round(requestQueueTimeMs))
 m.localTimeHist.update(Math.round(apiLocalTimeMs))
 m.remoteTimeHist.update(Math.round(apiRemoteTimeMs))
@@ -350,10 +350,11 @@ object RequestMetrics {
 }
 
 class RequestMetrics(name: String) extends KafkaMetricsGroup {
+
   import RequestMetrics._
 
   val tags = Map("request" -> name)
-  val requestRate = newMeter(RequestsPerSec, "requests", TimeUnit.SECONDS, 
tags)
+  val requestRateInternal = new mutable.HashMap[Short, Meter]
   // time a request spent in a request queue
   val requestQueueTimeHist = newHistogram(RequestQueueTimeMs, biased = true, 
tags)
   // time a request takes to be processed at the local broker
@@ -386,6 +387,10 @@ class RequestMetrics(name: String) extends 
KafkaMetricsGroup {
   private val errorMeters = mutable.Map[Errors, ErrorMeter]()
   Errors.values.foreach(error => errorMeters.put(error, new ErrorMeter(name, 
error)))
 
+  def requestRate(version: Short): Meter = {
+  requestRateInternal.getOrElseUpdate(version, newMeter("RequestsPerSec", 
"requests", TimeUnit.SECONDS, tags + ("version" -> version.toString)))
+  }
+
   class ErrorMeter(name: String, error: Errors) {
 private val tags = Map("request" -> name, "error" -> error.name)
 
@@ -418,7 +423,7 @@ class RequestMetrics(name: String) extends 
KafkaMetricsGroup {
   }
 
   def removeMetrics(): Unit = {
-removeMetric(RequestsPerSec, tags)
+for (version <- requestRateInternal.keySet) removeMetric(RequestsPerSec, 
tags + ("version" -> version.toString))
 removeMetric(RequestQueueTimeMs, tags)
 removeMetric(LocalTimeMs, tags)
 removeMetric(RemoteTimeMs, tags)
diff --git a/core/src/test/scala/unit/kafka/network/SocketServerTest.scala 
b/core/src/test/scala/unit/kafka/network/SocketServerTest.scala
index e6dadbb4253..7d3b42897cc 100644
--- a/core/src/test/scala/unit/kafka/network/SocketServerTest.scala
+++ b/core/src/test/scala/unit/kafka/network/SocketServerTest.scala
@@ -688,10 +688,14 @@ class SocketServerTest extends JUnitSuite {
   @Test
   def testRequestMetricsAfterStop(): Unit = {
 server.stopProcessingRequests()
-
-server.requestChannel.metrics(ApiKeys.PRODUCE.name).requestRate.mark()
+val version = ApiKeys.PRODUCE.latestVersion
+val version2 = (version - 1).toShort
+for (_ <- 0 to 1) 
server.requestChannel.metrics(ApiKeys.PRODUCE.name).requestRate(version).mark()
+
server.requestChannel.metrics(ApiKeys.PRODUCE.name).requestRate(version2).mark()
+assertEquals(2, 
server.requestChannel.metrics(ApiKeys.PRODUCE.name).requestRate(version).count())
 server.requestChannel.updateErrorMetrics(ApiKeys.PRODUCE, Map(Errors.NONE 
-> 1))
-val nonZeroMeters = 
Map("kafka.network:type=RequestMetrics,name=RequestsPerSec,request=Produce" -> 
1,
+val nonZeroMeters = 
Map(s"kafka.network:type=RequestMetrics,name=RequestsPerSec,request=Produce,version=$version"
 -> 2,
+
s"kafka.network:type=RequestMetrics,name=RequestsPerSec,request=Produce,version=$version2"
 -> 1,
 
"kafka.network:type=RequestMetrics,name=ErrorsPerSec,request=Produce,error=NONE"
 -> 1)
 
 def requestMetricMeters = YammerMetrics
diff --git a/docs/upgrade.html b/docs/upgrade.html
index 95f2c418c3b..d369d1de79d 100644
--- a/docs/upgrade.html
+++ b/docs/upgrade.html
@@ -68,6 +68,12 @@ Notable changes in 1
 
 https://cwiki.apache.org/confluence/x/oYtjB;>KIP-186 
increases the default offset retention time from 1 day to 7 days. This makes it 
less likely to "lose" offsets in an application that commits infrequently. It 
also increases the active set of offsets and therefore can increase memory 
usage on the broker. Note that the console consumer currently enables offset 
commit by

[jira] [Commented] (KAFKA-6514) Add API version as a tag for the RequestsPerSec metric

2018-02-07 Thread Ismael Juma (JIRA) 

[ 
https://issues.apache.org/jira/browse/KAFKA-6514?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16356321#comment-16356321
 ] 

Ismael Juma commented on KAFKA-6514:


[~allenxwang], the tags are part of the name in JMX and hence it would be a 
breaking change for anyone using the JmxReporter (which is enabled by default). 
If there are good arguments to break such uses, it can be considered as part of 
the KIP discussion.

If search for "metric", you can find a few KIPs that were posted for adding or 
updating existing metrics:

[https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals]

> Add API version as a tag for the RequestsPerSec metric
> --
>
> Key: KAFKA-6514
> URL: https://issues.apache.org/jira/browse/KAFKA-6514
> Project: Kafka
>  Issue Type: Improvement
>  Components: core
>Affects Versions: 1.0.0
>Reporter: Allen Wang
>Priority: Major
>
> After we upgrade broker to a new version, one important insight is to see how 
> many clients have been upgraded so that we can switch the message format when 
> most of the clients have also been updated to the new version to minimize the 
> performance penalty. 
> RequestsPerSec with the version tag will give us that insight.
>  
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (KAFKA-6514) Add API version as a tag for the RequestsPerSec metric

2018-02-06 Thread Allen Wang (JIRA) 

[ 
https://issues.apache.org/jira/browse/KAFKA-6514?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16354831#comment-16354831
 ] 

Allen Wang commented on KAFKA-6514:
---

[~ijuma] Since we don't change the name of the metric, but just adding a tag, 
it shouldn't break existing monitoring system. So I am wondering why a KIP is 
still required in this case. 

For example, I found that when upgrading to Kafka 0.10, "records-consumed-rate" 
metric has an added tag "topic", which makes sense to me. 

I would prefer keeping the same metric name since people are already familiar 
with it and the name makes sense. It would also have minimal impact on the 
storage required on a typical metric aggregation system. 

 

> Add API version as a tag for the RequestsPerSec metric
> --
>
> Key: KAFKA-6514
> URL: https://issues.apache.org/jira/browse/KAFKA-6514
> Project: Kafka
>  Issue Type: Improvement
>  Components: core
>Affects Versions: 1.0.0
>Reporter: Allen Wang
>Priority: Major
>
> After we upgrade broker to a new version, one important insight is to see how 
> many clients have been upgraded so that we can switch the message format when 
> most of the clients have also been updated to the new version to minimize the 
> performance penalty. 
> RequestsPerSec with the version tag will give us that insight.
>  
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (KAFKA-6514) Add API version as a tag for the RequestsPerSec metric

2018-02-06 Thread Ismael Juma (JIRA) 

[ 
https://issues.apache.org/jira/browse/KAFKA-6514?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16354625#comment-16354625
 ] 

Ismael Juma commented on KAFKA-6514:


Thanks for the JIRA [~allenxwang]. Agreed that this would be useful. A couple 
of comments/questions:
 # For compatibility reasons, we may consider keeping the metric without a 
version and add new metrics with the version tag.
 # We need a simple KIP since this affects metrics, which are public interfaces.
 # Given that the KIP freeze for 1.1.0 was on 23 January 
([https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=75957546),] 
this would have to target the next release.

> Add API version as a tag for the RequestsPerSec metric
> --
>
> Key: KAFKA-6514
> URL: https://issues.apache.org/jira/browse/KAFKA-6514
> Project: Kafka
>  Issue Type: Improvement
>  Components: core
>Affects Versions: 1.0.0
>Reporter: Allen Wang
>Priority: Major
>
> After we upgrade broker to a new version, one important insight is to see how 
> many clients have been upgraded so that we can switch the message format when 
> most of the clients have also been updated to the new version to minimize the 
> performance penalty. 
> RequestsPerSec with the version tag will give us that insight.
>  
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (KAFKA-6514) Add API version as a tag for the RequestsPerSec metric

2018-02-01 Thread ASF GitHub Bot (JIRA) 

[ 
https://issues.apache.org/jira/browse/KAFKA-6514?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16349012#comment-16349012
 ] 

ASF GitHub Bot commented on KAFKA-6514:
---

allenxwang opened a new pull request #4506: KAFKA-6514: Add API version as a 
tag for the RequestsPerSec metric
URL: https://github.com/apache/kafka/pull/4506
 
 
   Updated `RequestChannel` to include `version` as a tag for all 
RequestsPerSec metrics. Updated tests to verify that the extra tag exists.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> Add API version as a tag for the RequestsPerSec metric
> --
>
> Key: KAFKA-6514
> URL: https://issues.apache.org/jira/browse/KAFKA-6514
> Project: Kafka
>  Issue Type: Improvement
>  Components: core
>Affects Versions: 1.0.0
>Reporter: Allen Wang
>Priority: Major
>
> After we upgrade broker to a new version, one important insight is to see how 
> many clients have been upgraded so that we can switch the message format when 
> most of the clients have also been updated to the new version to minimize the 
> performance penalty. 
> RequestsPerSec with the version tag will give us that insight.
>  
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)