KAFKA-5890 records.lag should use tags for topic and partition rather than using metric name. #4362
Conversation
|
@becketqin @junrao @hachikuji pinging you for a review as you were the binding votes of KIP-225. Once this is merged I will open a JIRA against 2.0 to remove the deprecated metrics. |
|
@lahabana Thanks for the patch. It looks good. Can we also update docs/upgrade.html to let the users know about the new metrics and the deprecation schedule of the old metrics so the users may plan ahead? |
|
@becketqin thanks updated I couldn't find how to build the docs locally to check how it looks like. Could you let me know how to do this? Also should I link to the KIP-225 in the upgrade docs? |
|
@lahabana The upgrade doc is an html file, so you can open it in a browser. It would be good to have a pointer to the KIP wiki. In the KIP wiki, do you think it is worth adding a code snippet to show the users how to get the metric values? |
… than using metric name.
|
@becketqin updated the docs and added a before/after to the KIP. |
|
@becketqin what's the next step? |
|
retest this please |
|
@lahabana Sorry for the late response. I usually just comment out the javascript in the html. The updated patch LGTM. |
|
Thanks @becketqin |
| @@ -1327,16 +1327,25 @@ private void recordPartitionLag(TopicPartition tp, long lag) { | |||
| String name = partitionLagMetricName(tp); | |||
| Sensor recordsLag = this.metrics.getSensor(name); | |||
| if (recordsLag == null) { | |||
| Map<String, String> metricTags = new HashMap<>(2); | |||
| metricTags.put("topic", tp.topic().replace('.', '_')); | |||
There was a problem hiding this comment.
Is there a reason you replace '.' by '_' in that case?
There was a problem hiding this comment.
this is always done in metrics. I believe it's because JMX uses . to define hierarchy on entities so using a dot here would cause issues in the jmx reporter.
It's mentioned here: http://kafka.apache.org/documentation.html#upgrade_9_breaking
There was a problem hiding this comment.
Thanks for your reply. I guess this may be true in some cases, but at least for e.g. kafka.server:type=BrokerTopicMetrics,name=MessagesInPerSec it works even if the topic contains dots.
…r than using metric name. This is the implementation of KIP-225. It marks the previous metrics as deprecated in the documentation and adds new metrics using tags. Testing verifies that both the new and the old metric report the same value. Author: cmolter <cmolter@apple.com> Reviewers: Jiangjie (Becket) Qin <becket.qin@gmail.com> Closes apache#4362 from lahabana/kafka-5890 KAFKA-6184; report a metric of the lag between the consumer offset ... Add `records-lead` and partition-level `{topic}-{partition}.records-lead-min|avg` for fetcher metrics. junrao Please kindly review. Thanks. Author: huxihx <huxi_2b@hotmail.com> Reviewers: Jun Rao <junrao@gmail.com> Closes apache#4191 from huxihx/KAFKA-6184 MINOR: Remove deprecated per-partition lag metrics It takes O(n^2) time to instantiate a mbean with n attributes which can be very slow if the number of attributes of this mbean is large. This PR removes metrics whose number of attributes can grow with the number of partitions in the cluster to fix the performance issue. These metrics have already been marked for removal in 2.0 by KIP-225. Author: Dong Lin <lindong28@gmail.com> Reviewers: Ismael Juma <ismael@juma.me.uk> Closes apache#5172 from lindong28/remove-deprecated-metrics (cherry picked from commit 4580d9f) Signed-off-by: Dong Lin <lindong28@gmail.com> CLOUDERA-BUILD: Fix compilation error Use TestUtils.createTopic in PlaintextConsumerTest Change-Id: Ia3296e3093039c11b5c5c264a476ce51211a01f7
This is the implementation of KIP-225.
It marks the previous metrics as deprecated in the documentation and adds new metrics using tags.
Testing verifies that both the new and the old metric report the same value.
Committer Checklist (excluded from commit message)