Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
afd325b
add bytes/records consumed/produced metrics
ableegoldman May 30, 2022
6ab8c54
checkstyle
ableegoldman Jun 1, 2022
6a4eb80
undo backed out changes
ableegoldman Jun 1, 2022
f69fa86
reapply sensor recordings
ableegoldman Jun 1, 2022
70da1e2
more checkstyle
ableegoldman Jun 1, 2022
c89d21b
review feedback
ableegoldman Jun 1, 2022
edba70f
include all producer bytes
ableegoldman Jun 2, 2022
7a4dad3
null check for headers
ableegoldman Jun 2, 2022
0761f0d
fix name
ableegoldman Jun 2, 2022
5ed377d
fix up tests
ableegoldman Jun 2, 2022
a03f926
review feedback: consolidate to one sensor
ableegoldman Jun 2, 2022
b8d8f0c
checkstyle & value null check
ableegoldman Jun 2, 2022
9a0cfd1
extra ;
ableegoldman Jun 2, 2022
0df6c10
use util method in kvstore test driver
ableegoldman Jun 2, 2022
a8681d1
use topic name extractor
ableegoldman Jun 2, 2022
31d10f8
also nullcheck key
ableegoldman Jun 2, 2022
7dea1ae
nullcheck for test driver
ableegoldman Jun 2, 2022
5745fe9
fixin up some last tests
ableegoldman Jun 2, 2022
511b5bc
use local topic
ableegoldman Jun 2, 2022
a5756c1
add TopicMetrics
ableegoldman Jun 3, 2022
314be89
checkstyle
ableegoldman Jun 3, 2022
65a7ce4
back out formatting changes
ableegoldman Jun 3, 2022
3e09f01
fix StreamsMetricTest
ableegoldman Jun 3, 2022
d39b249
topology mock
ableegoldman Jun 3, 2022
6814000
close partition group in closeCleanAndRecycleState
ableegoldman Jun 3, 2022
7e67567
fix StreamsMetricsImplTest
ableegoldman Jun 3, 2022
9823764
fixin up TestTopicsTest
ableegoldman Jun 3, 2022
49fa512
toSet
ableegoldman Jun 3, 2022
a408b95
fixin scala tests
ableegoldman Jun 4, 2022
a0e2f08
FINALLY fix the broken tests...
ableegoldman Jun 7, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion checkstyle/suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@

<!-- Streams tests -->
<suppress checks="ClassFanOutComplexity"
files="(StreamsPartitionAssignorTest|StreamThreadTest|StreamTaskTest|TaskManagerTest|TopologyTestDriverTest).java"/>
files="(RecordCollectorTest|StreamsPartitionAssignorTest|StreamThreadTest|StreamTaskTest|TaskManagerTest|TopologyTestDriverTest).java"/>

<suppress checks="MethodLength"
files="(EosIntegrationTest|EosV2UpgradeIntegrationTest|KStreamKStreamJoinTest|RocksDBWindowStoreTest).java"/>
Expand Down
4 changes: 2 additions & 2 deletions docs/ops.html
Original file line number Diff line number Diff line change
Expand Up @@ -2308,7 +2308,7 @@ <h4 class="anchor-heading"><a id="connect_monitoring" class="anchor-link"></a><a
<h4 class="anchor-heading"><a id="kafka_streams_monitoring" class="anchor-link"></a><a href="#kafka_streams_monitoring">Streams Monitoring</a></h4>

A Kafka Streams instance contains all the producer and consumer metrics as well as additional metrics specific to Streams.
By default Kafka Streams has metrics with three recording levels: <code>info</code>, <code>debug</code>, and <code>trace</code>.
The metrics have three recording levels: <code>info</code>, <code>debug</code>, and <code>trace</code>.

Copy link
Copy Markdown
Member Author

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


<p>
Note that the metrics have a 4-layer hierarchy. At the top level there are client-level metrics for each started
Expand Down Expand Up @@ -2617,7 +2617,7 @@ <h5 class="anchor-heading"><a id="kafka_streams_node_monitoring" class="anchor-l
</table>

<h5 class="anchor-heading"><a id="kafka_streams_store_monitoring" class="anchor-link"></a><a href="#kafka_streams_store_monitoring">State Store Metrics</a></h5>
All of the following metrics have a recording level of <code>debug</code>, except for the record-e2e-latency-* metrics which have a recording level <code>trace></code>.
All of the following metrics have a recording level of <code>debug</code>, except for the record-e2e-latency-* metrics which have a recording level <code>trace</code>.
Note that the <code>store-scope</code> value is specified in <code>StoreSupplier#metricsScope()</code> for user's customized state stores;
for built-in state stores, currently we have:
<ul>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,8 @@ private StreamTask createActiveTask(final TaskId taskId,
taskId,
streamsProducer,
applicationConfig.defaultProductionExceptionHandler(),
streamsMetrics
streamsMetrics,
topology
);

final StreamTask task = new StreamTask(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,17 @@
import org.apache.kafka.clients.admin.OffsetSpec;
import org.apache.kafka.clients.consumer.Consumer;
import org.apache.kafka.clients.consumer.ConsumerConfig;
import org.apache.kafka.clients.consumer.ConsumerRecord;
import org.apache.kafka.clients.producer.ProducerRecord;
import org.apache.kafka.common.KafkaException;
import org.apache.kafka.common.KafkaFuture;
import org.apache.kafka.common.Metric;
import org.apache.kafka.common.MetricName;
import org.apache.kafka.common.TopicPartition;
import org.apache.kafka.common.errors.TimeoutException;
import org.apache.kafka.common.header.Header;
import org.apache.kafka.common.header.Headers;
import org.apache.kafka.common.utils.Utils;
import org.apache.kafka.streams.StreamsConfig;
import org.apache.kafka.streams.errors.StreamsException;
import org.apache.kafka.streams.processor.TaskId;
Expand Down Expand Up @@ -166,4 +171,46 @@ public static String extractThreadId(final String fullThreadName) {
final int index = fullThreadName.indexOf("StreamThread-");
return fullThreadName.substring(index);
}

public static long producerRecordSizeInBytes(final ProducerRecord<byte[], byte[]> record) {
return recordSizeInBytes(
record.key().length,
record.value() == null ? 0 : record.value().length,
record.topic(),
record.headers()
);
}

public static long consumerRecordSizeInBytes(final ConsumerRecord<byte[], byte[]> record) {
return recordSizeInBytes(
record.serializedKeySize(),
record.serializedValueSize(),
record.topic(),
record.headers()
);
}

public static long recordSizeInBytes(final long keyBytes,
Comment thread
ableegoldman marked this conversation as resolved.
Outdated
Comment thread
ableegoldman marked this conversation as resolved.
Outdated
Comment thread
ableegoldman marked this conversation as resolved.
final long valueBytes,
final String topic,
final Headers headers) {
long headerSizeInBytes = 0L;

if (headers != null) {
for (final Header header : headers.toArray()) {
headerSizeInBytes += Utils.utf8(header.key()).length;
if (header.value() != null) {
headerSizeInBytes += header.value().length;
}
}
}

return keyBytes +
valueBytes +
8L + // timestamp
8L + // offset
Utils.utf8(topic).length +
4L + // partition
headerSizeInBytes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ StampedRecord nextRecord(final RecordInfo info, final long wallClockTime) {
// get the buffer size of queue before poll
final long oldBufferSize = queue.getTotalBytesBuffered();
// get the first record from this queue.
record = queue.poll();
record = queue.poll(wallClockTime);
// After polling, the buffer size would have reduced.
final long newBufferSize = queue.getTotalBytesBuffered();

Expand Down Expand Up @@ -392,6 +392,12 @@ void clear() {
streamTime = RecordQueue.UNKNOWN;
}

void close() {
for (final RecordQueue queue : partitionQueues.values()) {
queue.close();
}
}

// Below methods are for only testing.

boolean allPartitionsBufferedLocally() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,9 @@ public void logChange(final String storeName,
changelogPartition.partition(),
timestamp,
BYTES_KEY_SERIALIZER,
BYTEARRAY_VALUE_SERIALIZER
);
BYTEARRAY_VALUE_SERIALIZER,
null,
null);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ <K, V> void send(final String topic,
final Integer partition,
final Long timestamp,
final Serializer<K> keySerializer,
final Serializer<V> valueSerializer);
final Serializer<V> valueSerializer,
final String processorNodeId,
final InternalProcessorContext<Void, Void> context);

<K, V> void send(final String topic,
final K key,
Expand All @@ -42,6 +44,8 @@ <K, V> void send(final String topic,
final Long timestamp,
final Serializer<K> keySerializer,
final Serializer<V> valueSerializer,
final String processorNodeId,
final InternalProcessorContext<Void, Void> context,
final StreamPartitioner<? super K, ? super V> partitioner);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
import org.apache.kafka.streams.processor.TaskId;
import org.apache.kafka.streams.processor.internals.metrics.StreamsMetricsImpl;
import org.apache.kafka.streams.processor.internals.metrics.TaskMetrics;
import org.apache.kafka.streams.processor.internals.metrics.TopicMetrics;

import org.slf4j.Logger;

import java.util.Collections;
Expand All @@ -54,17 +56,22 @@
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;

import static org.apache.kafka.streams.processor.internals.ClientUtils.producerRecordSizeInBytes;

public class RecordCollectorImpl implements RecordCollector {
private final static String SEND_EXCEPTION_MESSAGE = "Error encountered sending record to topic %s for task %s due to:%n%s";

private final Logger log;
private final TaskId taskId;
private final StreamsProducer streamsProducer;
private final ProductionExceptionHandler productionExceptionHandler;
private final Sensor droppedRecordsSensor;
private final boolean eosEnabled;
private final Map<TopicPartition, Long> offsets;

private final StreamsMetricsImpl streamsMetrics;
private final Sensor droppedRecordsSensor;
private final Map<String, Map<String, Sensor>> sinkNodeToProducedSensorByTopic = new HashMap<>();

private final AtomicReference<KafkaException> sendException = new AtomicReference<>(null);

/**
Expand All @@ -74,15 +81,29 @@ public RecordCollectorImpl(final LogContext logContext,
final TaskId taskId,
final StreamsProducer streamsProducer,
final ProductionExceptionHandler productionExceptionHandler,
final StreamsMetricsImpl streamsMetrics) {
final StreamsMetricsImpl streamsMetrics,
final ProcessorTopology topology) {
this.log = logContext.logger(getClass());
this.taskId = taskId;
this.streamsProducer = streamsProducer;
this.productionExceptionHandler = productionExceptionHandler;
this.eosEnabled = streamsProducer.eosEnabled();
this.streamsMetrics = streamsMetrics;

final String threadId = Thread.currentThread().getName();
this.droppedRecordsSensor = TaskMetrics.droppedRecordsSensor(threadId, taskId.toString(), streamsMetrics);
for (final String topic : topology.sinkTopics()) {
final String processorNodeId = topology.sink(topic).name();
sinkNodeToProducedSensorByTopic.computeIfAbsent(processorNodeId, t -> new HashMap<>()).put(
topic,
TopicMetrics.producedSensor(
threadId,
taskId.toString(),
processorNodeId,
topic,
streamsMetrics
));
}

this.offsets = new HashMap<>();
}
Expand All @@ -106,6 +127,8 @@ public <K, V> void send(final String topic,
final Long timestamp,
final Serializer<K> keySerializer,
final Serializer<V> valueSerializer,
final String processorNodeId,
final InternalProcessorContext<Void, Void> context,
final StreamPartitioner<? super K, ? super V> partitioner) {
final Integer partition;

Expand All @@ -122,7 +145,7 @@ public <K, V> void send(final String topic,
// here we cannot drop the message on the floor even if it is a transient timeout exception,
// so we treat everything the same as a fatal exception
throw new StreamsException("Could not determine the number of partitions for topic '" + topic +
"' for task " + taskId + " due to " + fatal.toString(),
"' for task " + taskId + " due to " + fatal,
fatal
);
}
Expand All @@ -136,7 +159,7 @@ public <K, V> void send(final String topic,
partition = null;
}

send(topic, key, value, headers, partition, timestamp, keySerializer, valueSerializer);
send(topic, key, value, headers, partition, timestamp, keySerializer, valueSerializer, processorNodeId, context);
}

@Override
Expand All @@ -147,7 +170,9 @@ public <K, V> void send(final String topic,
final Integer partition,
final Long timestamp,
final Serializer<K> keySerializer,
final Serializer<V> valueSerializer) {
final Serializer<V> valueSerializer,
final String processorNodeId,
final InternalProcessorContext<Void, Void> context) {
checkForException();

final byte[] keyBytes;
Expand All @@ -173,7 +198,7 @@ public <K, V> void send(final String topic,
valueClass),
exception);
} catch (final RuntimeException exception) {
final String errorMessage = String.format(SEND_EXCEPTION_MESSAGE, topic, taskId, exception.toString());
final String errorMessage = String.format(SEND_EXCEPTION_MESSAGE, topic, taskId, exception);
throw new StreamsException(errorMessage, exception);
}

Expand All @@ -192,6 +217,28 @@ public <K, V> void send(final String topic,
} else {
log.warn("Received offset={} in produce response for {}", metadata.offset(), tp);
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 🙂.

@ableegoldman ableegoldman Jun 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

final Map<String, Sensor> producedSensorByTopic = sinkNodeToProducedSensorByTopic.get(processorNodeId);
if (producedSensorByTopic == null) {
log.error("Unable to records bytes produced to topic {} by sink node {} as the node is not recognized.\n"
+ "Known sink nodes are {}.", topic, processorNodeId, sinkNodeToProducedSensorByTopic.keySet());
} else {
final Sensor topicProducedSensor = producedSensorByTopic.computeIfAbsent(
topic,
t -> TopicMetrics.producedSensor(
Thread.currentThread().getName(),
taskId.toString(),
processorNodeId,
topic,
context.metrics()
)
);
final long bytesProduced = producerRecordSizeInBytes(serializedRecord);
topicProducedSensor.record(bytesProduced, context.currentSystemTimeMs());
}
}
} else {
recordSendError(topic, exception, serializedRecord);

Expand Down Expand Up @@ -267,6 +314,8 @@ public void flush() {
public void closeClean() {
log.info("Closing record collector clean");

removeAllProducedSensors();

// No need to abort transaction during a clean close: either we have successfully committed the ongoing
// transaction during handleRevocation and thus there is no transaction in flight, or else none of the revoked
// tasks had any data in the current transaction and therefore there is no need to commit or abort it.
Expand All @@ -290,6 +339,14 @@ public void closeDirty() {
checkForException();
}

private void removeAllProducedSensors() {
for (final Map<String, Sensor> nodeMap : sinkNodeToProducedSensorByTopic.values()) {
for (final Sensor sensor : nodeMap.values()) {
streamsMetrics.removeSensor(sensor);
}
}
}

@Override
public Map<TopicPartition, Long> offsets() {
return Collections.unmodifiableMap(new HashMap<>(offsets));
Expand Down
Loading