KAFKA-20877: Fix incorrect restore-rate and update-rate metrics - #23009
KAFKA-20877: Fix incorrect restore-rate and update-rate metrics#23009mjsax wants to merge 4 commits into
Conversation
Both metrics incorrectly report "batches restored per second" instead of "records restored per seconds".
| final String taskId, | ||
| final StreamsMetricsImpl streamsMetrics) { | ||
| return invocationRateAndTotalSensor( | ||
| return rateAndTotalSensor( |
There was a problem hiding this comment.
This changes dropped-records-rate too: almost all call sites record 1 per record, but InMemoryWindowStore:158 and InMemorySessionStore:163 record a whole batch (expiredRecordSensor.record(expiredRecords, ...))
There was a problem hiding this comment.
Oh. So this metric (dropped-records-rate) is actually broken for InMemoryWindowStore and InMemorySessionStore, too. Great catch.
Guess we just need to update the Jira and PR description, as this PR does fix the issue, too?
There was a problem hiding this comment.
Actually, it seems the issue for InMemoryWindowStore and InMemorySessionStore was only introduced recently on trunk only: d2328ff
So we don't even need to update the Jira description as there is no issue in 4.3 and older releases.
| final StreamsMetricsImpl streamsMetrics, | ||
| final Sensor... parentSensor) { | ||
| return invocationRateAndTotalSensor( | ||
| return rateAndTotalSensor( |
There was a problem hiding this comment.
I assume you changed the code so that it matches with what we have in docs/operations/monitoring.md ? So no need to update the doc? Should we mention this somewhere so that users understand that the change in number is due to an improvement in how we measure it, rather than a change in their application?
There was a problem hiding this comment.
Don't think we need to call it out -- that is what the release changelog is for, and KAFKA-20877 will be listed.
That's actually the main reason, why I did not do this a MINOR PR, but filed a Jira, to highlight the fix.
| // regression test for KAFKA-20877: previously we did incorrectly count batches which would result in 0.03333 | ||
| // using 0.5 as good intermediate to the expected value of 0.83333 | ||
| // -> avoid equalTo(...) on floating point numbers | ||
| 0.5d < ((Number) rateMetric.metricValue()).doubleValue(), |
There was a problem hiding this comment.
Same as in StreamTaskTest
| assertTrue( | ||
| // regression test for KAFKA-20877: previously we did incorrectly count batches which would result in 0.06666 | ||
| // using 2.0 as good intermediate to the expected value of 2.5 | ||
| // -> avoid equalTo(...) on floating point numbers |
There was a problem hiding this comment.
nit: So could we use the JUnit's three-arg assertEquals(expected, actual, delta)?
aliehsaeedii
left a comment
aliehsaeedii
left a comment
There was a problem hiding this comment.
Thanks @mjsax. I left two more nits but it's up to you to address them or not. Approved from my side.
| final S segment = segments.getOrCreateSegmentIfLive(segmentId, internalProcessorContext, observedStreamTime); | ||
| if (segment == null) { | ||
| expiredRecordSensor.record(1.0d, internalProcessorContext.currentSystemTimeMs()); | ||
| expiredRecordSensor.record(); |
There was a problem hiding this comment.
This is the only expired-record drop that logs nothing — the sibling in AbstractDualSchemaRocksDBSegmentedBytesStore follows the same record() with LOG.warn("Skipping record for expired segment."). Worth adding here.
| return store; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
I know this is only a test class but do we need this blank line?
bbejeck
left a comment
bbejeck
left a comment
There was a problem hiding this comment.
Thanks @mjsax - LGMT plus one minor comment @aliehsaeedii pointed out
3 participants
Both metrics incorrectly report "batches restored per second" instead of
"records restored per seconds".
Reviewers: Alieh Saeedi asaeedi@confluent.io, Bill Bejeck
bbejeck@apache.org