KAFKA-8964: Rename tag client-id for thread-level metrics and below by cadonna · Pull Request #7429 · apache/kafka

cadonna · 2019-10-01T13:41:32Z

Renamed tag client-id to thread-id for thread-level metrics and below
Corrected metrics tag keys for state store that had suffix "-id" instead
of "state-id"

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

cadonna · 2019-10-01T13:41:58Z

Call for review: @guozhangwang @vvcephei @bbejeck

bbejeck

Thanks for the PR @cadonna overall this LGTM, I just have 2 minor comments.

bbejeck · 2019-10-02T15:08:10Z

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java

-    public final Map<String, String> tagMap(final String... tags) {
-        final Map<String, String> tagMap = new LinkedHashMap<>();
-        tagMap.put("client-id", threadName);
-        if (tags != null) {


Nice refactoring to put this code in a separate method!

bbejeck · 2019-10-02T15:52:18Z

...c/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java

+            tagMap.get(builtInMetricsVersion.equals(StreamsConfig.METRICS_LATEST) ? StreamsMetricsImpl.THREAD_ID_TAG
+                : StreamsMetricsImpl.THREAD_ID_TAG_0100_TO_23),


Maybe get the value of this expression on the line above. IMHO it will make it easier to see what the assertThat is doing.

bbejeck · 2019-10-02T15:52:39Z

...c/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java

+            tagMap.get(builtInMetricsVersion.equals(StreamsConfig.METRICS_LATEST) ? StreamsMetricsImpl.THREAD_ID_TAG
+                : StreamsMetricsImpl.THREAD_ID_TAG_0100_TO_23),


Same as above

bbejeck · 2019-10-02T15:56:52Z

Java 11/2.12, Java 11/2.13 and Java 8 all failed. Test results already cleaned up.

retest this please

bbejeck · 2019-10-02T16:27:26Z

ping either of @guozhangwang or @vvcephei for a second review

vvcephei

Looks good! Thanks @cadonna

guozhangwang

Minor comments, otherwise LGTM.

guozhangwang · 2019-10-02T18:58:04Z

streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorNode.java

-            final Map<String, String> tagMap = metrics.tagMap("task-id", context.taskId().toString(), PROCESSOR_NODE_ID_TAG, processorNodeName);
-            final Map<String, String> allTagMap = metrics.tagMap("task-id", context.taskId().toString(), PROCESSOR_NODE_ID_TAG, "all");
+            final Map<String, String> tagMap = metrics.nodeLevelTagMap(context.taskId().toString(), processorNodeName);
+            final Map<String, String> allTagMap =


This is still a node-level metrics technically speaking, but just with node-tag pointing to all right? In that can should we just call nodeLevelTagMap(context.taskId().toString(), ROLLUP_VALUE)?

Good catch! Of course! I even did as you said for the commitOverTask sensor. I totally overlooked this one.

guozhangwang · 2019-10-02T18:58:30Z

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java

        return tagMap;
    }

+    public Map<String, String> taskLevelTagMap(final String taskName, final String... tags) {


If the above comment is valid, then we do not need this overload func here.

Right! Will delete!

guozhangwang · 2019-10-02T19:00:05Z

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java

        updatedTags[tags.length] = scopeName + "-id";
        updatedTags[tags.length + 1] = entityName;
-        return tagMap(updatedTags);
+        return threadLevelTagMap(updatedTags);


This makes me thinking: should we clarify in our javadoc that user customized metrics would be recorded at the thread-level specifically, with additional scope tag?

That would be good idea. Do you want to update the javadocs in KIP-444?

guozhangwang · 2019-10-02T19:04:26Z

...c/main/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStore.java

            expiredRecordSensor,
            "stream-" + metricScope + "-metrics",
-            metrics.tagMap("task-id", taskName, metricScope + "-id", name()),
+            metrics.storeLevelTagMap(taskName, metricScope, name()),


Good catch!

Also since the "incorrect" metrics name is out in our previous releases, but fixing it we are effectively breaking user's compatibility. I think this okay since we are fixing a bug, but would still worth clarifying it in the upgrade guide docs.

guozhangwang

One meta comment: in KIP-444 we are changing the type from stream-metrics to stream-thread-metrics as well, is it going to be included in future PRs? I did not see it in either of the open ones yet.

bbejeck · 2019-10-03T17:15:35Z

All failed, test results cleaned up already

retest this please

cadonna

@guozhangwang Yes, the type is changed from stream-metrics to stream-thread-metrics in the thread-level refactoring thread that I could not open yet since it is based on this one.

cadonna · 2019-10-03T18:43:02Z

streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorNode.java

-            final Map<String, String> tagMap = metrics.tagMap("task-id", context.taskId().toString(), PROCESSOR_NODE_ID_TAG, processorNodeName);
-            final Map<String, String> allTagMap = metrics.tagMap("task-id", context.taskId().toString(), PROCESSOR_NODE_ID_TAG, "all");
+            final Map<String, String> tagMap = metrics.nodeLevelTagMap(context.taskId().toString(), processorNodeName);
+            final Map<String, String> allTagMap =


Good catch! Of course! I even did as you said for the commitOverTask sensor. I totally overlooked this one.

cadonna · 2019-10-03T18:45:12Z

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java

        return tagMap;
    }

+    public Map<String, String> taskLevelTagMap(final String taskName, final String... tags) {


Right! Will delete!

cadonna · 2019-10-03T18:49:08Z

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java

        updatedTags[tags.length] = scopeName + "-id";
        updatedTags[tags.length + 1] = entityName;
-        return tagMap(updatedTags);
+        return threadLevelTagMap(updatedTags);


That would be good idea. Do you want to update the javadocs in KIP-444?

cadonna · 2019-10-03T18:51:09Z

...c/main/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStore.java

            expiredRecordSensor,
            "stream-" + metricScope + "-metrics",
-            metrics.tagMap("task-id", taskName, metricScope + "-id", name()),
+            metrics.storeLevelTagMap(taskName, metricScope, name()),


bbejeck · 2019-10-04T11:46:48Z

Java 11/2.12 failed with org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector
Java 11/2.13 failed with org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSinkConnector
Java 8 failed with
org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector

retest this please

…s and below - Renamed tag client-id to thread-id for thread-level metrics and below - Corrected metrics tag keys for state store that had suffix "-id" instead of "state-id"

cadonna · 2019-10-08T17:55:28Z

In JDK 11/Scala 2.13 the following tests failed:

kafka.admin.ReassignPartitionsClusterTest.shouldTriggerReassignmentWithZnodePrecedenceOnControllerStartup
org.apache.kafka.streams.integration.KTableKTableForeignKeyJoinIntegrationTest.doLeftJoinFromRightThenDeleteRightEntity

Retest this, please

guozhangwang · 2019-10-08T22:11:53Z

Test failures on kafka.api.SslAdminClientIntegrationTest.testAclOperations and org.apache.kafka.streams.integration.KTableKTableForeignKeyJoinIntegrationTest.doLeftJoinFromRightThenDeleteRightEntity respectively.

guozhangwang · 2019-10-08T22:24:42Z

LGTM, merged to trunk (2.5).

Revert "KAFKA-8964: Rename tag client-id for thread-level metrics and below (apache#7429)"

…t-for-generated-requests * apache-github/trunk: KAFKA-8932; Add tag for CreateTopicsResponse.TopicConfigErrorCode (KIP-525) (apache#7464) KAFKA-8944: Fixed KTable compiler warning. (apache#7393) KAFKA-8964: Rename tag client-id for thread-level metrics and below (apache#7429) MINOR: remove unused imports in Streams system tests (apache#7468) KAFKA-7190; Retain producer state until transactionalIdExpiration time passes (apache#7388) KAFKA-8983; AdminClient deleteRecords should not fail all partitions unnecessarily (apache#7449) MINOR: Modified Exception handling for KIP-470 (apache#7461) KAFKA-7245: Deprecate WindowStore#put(key, value) (apache#7105) KAFKA-8179: Part 7, cooperative rebalancing in Streams (apache#7386) KAFKA-8985; Add flexible version support to inter-broker APIs (apache#7453) MINOR: Bump version to 2.5.0-SNAPSHOT (apache#7455)

cadonna changed the title ~~KAFKA-8964: Rename tag client-id to thread-id for thread-level metric…~~ KAFKA-8964: Rename tag client-id for thread-level metrics and below Oct 1, 2019

cadonna mentioned this pull request Oct 2, 2019

KAFKA-8934: Introduce instance-level metrics for streams applications #7416

Merged

3 tasks

bbejeck added the streams label Oct 2, 2019

bbejeck approved these changes Oct 2, 2019

View reviewed changes

vvcephei approved these changes Oct 2, 2019

View reviewed changes

guozhangwang reviewed Oct 2, 2019

View reviewed changes

cadonna commented Oct 3, 2019

View reviewed changes

cadonna added 4 commits October 8, 2019 12:23

KAFKA-8964: Rename tag client-id to thread-id for thread-level metric…

c939fce

…s and below - Renamed tag client-id to thread-id for thread-level metrics and below - Corrected metrics tag keys for state store that had suffix "-id" instead of "state-id"

Include feedback

17127ca

Include feedback

977fd16

Correct errors due to conflicts after rebase

6acde48

cadonna force-pushed the AK8964-rename_client_id_tag_key branch from adbcf55 to 6acde48 Compare October 8, 2019 12:08

guozhangwang merged commit e3c2148 into apache:trunk Oct 8, 2019

soondenana added a commit to soondenana/kafka that referenced this pull request Feb 7, 2020

Merge pull request #1 from vvcephei/upstream_merge_Feb_6

d8a6812

Revert "KAFKA-8964: Rename tag client-id for thread-level metrics and below (apache#7429)"

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

		tagMap.get(builtInMetricsVersion.equals(StreamsConfig.METRICS_LATEST) ? StreamsMetricsImpl.THREAD_ID_TAG
		: StreamsMetricsImpl.THREAD_ID_TAG_0100_TO_23),

Conversation

cadonna commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

cadonna commented Oct 1, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Oct 2, 2019

Uh oh!

bbejeck commented Oct 2, 2019

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Oct 3, 2019

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Oct 4, 2019

Uh oh!

cadonna commented Oct 8, 2019

Uh oh!

guozhangwang commented Oct 8, 2019

Uh oh!

guozhangwang commented Oct 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

cadonna commented Oct 1, 2019 •

edited

Loading