KAFKA-13945: add bytes/records consumed and produced metrics by ableegoldman · Pull Request #12235 · apache/kafka

ableegoldman · 2022-06-01T12:23:08Z

Implementation of KIP-846: Source/sink node metrics for Consumed/Produced throughput in Streams

Adds the following INFO node-level metrics for the total bytes/records consumed and produced:

bytes-consumed-total
records-consumed-total
bytes-produced-total
records-produced-total

ableegoldman · 2022-06-01T12:24:56Z

I'll follow up with a docs PR separately

ableegoldman · 2022-06-01T12:44:19Z

Optimization to hopefully bypass any potential concern or risk of performance regression -- it may be paranoid or an over-optimization at this point but imo it doesn't hurt to report these metrics at a slightly more course-grained time interval. Happy to go either way on this however

I think it is fine as you do it since we do it similarly in other places like processAtSourceSensor.record(1.0d, context.currentSystemTimeMs()); in SourceNode.

You can also remove the comment.

ableegoldman · 2022-06-01T12:45:02Z

Just clearing up an overly-generic method name (the metric name itself is not changed)

cadonna

@ableegoldman Thank you for the PR!

Here my feedback:

cadonna · 2022-06-01T17:49:43Z

I actually do not understand this comment. Backwards compatibility of what? What does "empty sensor without parent" mean? recordsProducedSensor() and bytesProducedSensor() do not create sensors with parents.

Nice, I just realized that you copied that comment from code that I wrote! 🙂
I guess the backwards compatibility was the one with the old metrics structure that we changed in KIP-444. We removed the old structure in 3.0, so I guess that this is an instance of comments that started lying.
Moreover, from where you copied the comment (I guess it was SourceNode) the sensor processAtSourceSensor has indeed a parent. I think the comment does not make sense in this class and we need to verify if it still makes sense in the other class. That does not need to be verifed in this PR.

Ah yeah I was wondering about it but I hoped you'd see this and remember the context. I'll remove the comment from my PR and we can follow up separately if it needs to be removed from the SourceNode class (where I copied it from, yes)

cadonna · 2022-06-01T18:09:10Z

I think it is fine as you do it since we do it similarly in other places like processAtSourceSensor.record(1.0d, context.currentSystemTimeMs()); in SourceNode.

cadonna · 2022-06-01T18:40:04Z

You can also remove the comment.

guozhangwang

Made a pass, overall LGTM modulo one minor suggestion to use one sensor for both metrics.

Please feel free to merge once KIP is accepted!

guozhangwang · 2022-06-02T00:36:26Z

guozhangwang · 2022-06-02T00:39:41Z

A minor suggestion: I think we can use one sensor that contains two metrics, one as "sum" and one as "count", and then call record here once on that sensor which would update both metrics. Similar for consumed.

Examples can be found in those addInvocationRateAndCountToSensor etc functions.

guozhangwang · 2022-06-02T00:40:43Z

Since we always record 1 for this sensor, I think we can combine this and the bytes metrics in a single sensor with sum and count (see my other comment above). Ditto for produced.

cadonna

@ableegoldman Thank you for the updates!

Here my feedback:

ableegoldman · 2022-06-03T09:38:24Z

@cadonna ok this is ready for another pass -- besides addressing your comments I made two changes worth noting:

I moved the produced/consumed metrics to a new TopicMetrics class as they didn't exactly fit into ProcessorNodeMetrics since I realized that due to dynamic topic routing you could have more than one topic produced to by a given sink node.
To address your (imo valid) concern about over-counting bytes that were sent to the producer but never made it to the topic, I moved the "-produced" sensor into RecordCollectorImpl and have it record inside the send callback after we've checked for errors

The PR has definitely ballooned in size since you last reviewed it but that's almost all due to the new test coverage for the topic-level metrics. The actual logical changes are still relatively minor, so I'm hoping you can give this a +1 by now (and again, if so, please go ahead and merge it yourself)

Thanks for the thorough reviews! :P

vvcephei · 2022-06-03T14:53:22Z

The prior build was hung. I cancelled it so the latest commit could be built.

vvcephei · 2022-06-03T14:55:22Z

@ableegoldman , you might want to take a look at the build I cancelled. Before it got stuck, there were a large number of failures: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12235/9/#showFailuresLink

cadonna

@ableegoldman Thank you for the updates!

I had just a couple of nits that can also be done in a follow-up PR.

I did not quite get all the changes in the scala tests and the test-utils tests. Are those intended or just debugging side-effects?

cadonna · 2022-06-07T13:17:57Z

                }
+
+                if (!topic.endsWith("-changelog")) {
+                    // we may not have created a sensor yet if the node uses dynamic topic routing


This comment is a bit misleading here. AFAIU it refers to the else-branch. Please move it or remove it. I think you know my preference 🙂.

~~No, it does refer to the if branch -- if sinkNodeToProducedSensorByTopic.get returns null, that means we haven't seen this topic yet/constructed a sensor for it during initialization.~~
edit: this is wrong, see following comment

I do see how it might be confusing if it's interpreted as referring to the line above, and I do know your preference, but I stand by this particular comment -- it follows the rule of explaining WHY we do something rather than WHAT, and I believe in this case the why is very much not obvious* and worry someone reading this code in the future might think this branch would actually indicate a bug and decide to throw an exception or something. So I'll adjust the spacing to clarify which line it's referring to but I would like to keep it.

I believe in this case the why is very much not obvious

* Source: I myself did not remember about dynamic topics and only realized this was possible when looking at some TTD tests

Oh wait sorry I'm being dumb, yes, it does refer to the else case -- I've moved it accordingly.

cadonna · 2022-06-07T13:54:50Z

The test failures are unrelated.

* KAFKA-13152: Replace "buffered.records.per.partition" with "input.buffer.max.bytes" (apache#11796) Implements KIP-770 Reviewers: Guozhang Wang <wangguoz@gmail.com> (cherry picked from commit 0924fd3) * KAFKA-13945: add bytes/records consumed and produced metrics (apache#12235) Implementation of KIP-846: Source/sink node metrics for Consumed/Produced throughput in Streams Adds the following INFO topic-level metrics for the total bytes/records consumed and produced: bytes-consumed-total records-consumed-total bytes-produced-total records-produced-total Reviewers: Kvicii <Karonazaba@gmail.com>, Guozhang Wang <guozhang@apache.org>, Bruno Cadonna <cadonna@apache.org> * Update CODEOWNERS (#721) (cherry picked from commit ad18b43) Co-authored-by: vamossagar12 <sagarmeansocean@gmail.com> Co-authored-by: A. Sophie Blee-Goldman <sophie@confluent.io> Co-authored-by: Chris Johnson <53450846+cjohnson-confluent@users.noreply.github.com>

…12288) Minor followup to #12235 that adds a null check on the record key in the new ClientUtils#producerRecordSizeInBytes utility method, as there are valid cases in which we might be sending records with null keys to the Producer, such as a simple builder.stream("non-keyed-input-topic").filter(...).to("output-topic") Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>

ableegoldman requested review from cadonna, guozhangwang and vvcephei June 1, 2022 12:23

ableegoldman commented Jun 1, 2022

View reviewed changes

Comment thread docs/ops.html Outdated

Copy link
Copy Markdown

Member Author

ableegoldman Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow up with a docs PR separately

ableegoldman commented Jun 1, 2022

View reviewed changes

Kvicii approved these changes Jun 1, 2022

View reviewed changes

cadonna reviewed Jun 1, 2022

View reviewed changes

guozhangwang reviewed Jun 2, 2022

View reviewed changes

ableegoldman force-pushed the 13945-add-bytes/records-produced-metric branch from 3879617 to a2228f7 Compare June 2, 2022 10:03

cadonna reviewed Jun 2, 2022

View reviewed changes

guozhangwang reviewed Jun 3, 2022

View reviewed changes

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

ableegoldman force-pushed the 13945-add-bytes/records-produced-metric branch 2 times, most recently from c555982 to 61d5a51 Compare June 3, 2022 20:35

ableegoldman added 13 commits June 5, 2022 23:05

add bytes/records consumed/produced metrics

afd325b

checkstyle

6ab8c54

undo backed out changes

6a4eb80

reapply sensor recordings

f69fa86

more checkstyle

70da1e2

review feedback

c89d21b

include all producer bytes

edba70f

null check for headers

7a4dad3

fix name

0761f0d

fix up tests

5ed377d

review feedback: consolidate to one sensor

a03f926

checkstyle & value null check

b8d8f0c

extra ;

9a0cfd1

ableegoldman added 16 commits June 5, 2022 23:06

use util method in kvstore test driver

0df6c10

use topic name extractor

a8681d1

also nullcheck key

31d10f8

nullcheck for test driver

7dea1ae

fixin up some last tests

5745fe9

use local topic

511b5bc

add TopicMetrics

a5756c1

checkstyle

314be89

back out formatting changes

65a7ce4

fix StreamsMetricTest

3e09f01

topology mock

d39b249

close partition group in closeCleanAndRecycleState

6814000

fix StreamsMetricsImplTest

7e67567

fixin up TestTopicsTest

9823764

toSet

49fa512

fixin scala tests

a408b95

This was referenced Jun 6, 2022

[DO NOT MERGE] test branch for debugging #12235 (KAFKA-13945) #12256

Closed

[DO NOT MERGE] test branch for debugging #12235 (KAFKA-13945) #12257

Closed

ableegoldman force-pushed the 13945-add-bytes/records-produced-metric branch 2 times, most recently from 8811616 to a408b95 Compare June 6, 2022 08:37

FINALLY fix the broken tests...

a0e2f08

cadonna approved these changes Jun 7, 2022

View reviewed changes

cadonna merged commit a6c5a74 into apache:trunk Jun 7, 2022

ableegoldman mentioned this pull request Jun 13, 2022

HOTFIX: null check for ProducerRecord when computing sizeInBytes #12288

Merged

ableegoldman mentioned this pull request Jun 19, 2022

MINOR: change Streams topic-level metrics tag from 'topic-name' to 'topic' #12310

Merged

Conversation

ableegoldman commented Jun 1, 2022

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!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

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!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ableegoldman commented Jun 3, 2022

Uh oh!

vvcephei commented Jun 3, 2022

Uh oh!

vvcephei commented Jun 3, 2022

Uh oh!

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ableegoldman Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cadonna commented Jun 7, 2022

Uh oh!

Reviewers

cadonna Jun 1, 2022 •

edited

Loading

ableegoldman Jun 19, 2022 •

edited

Loading