-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-14953: Add tiered storage related metrics #13944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b3e3793
925f9cc
fb59242
5663ccb
bd1104a
7fcdb14
bd46e24
5246478
4cc354b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| */ | ||
| package kafka.log.remote; | ||
|
|
||
| import kafka.server.BrokerTopicStats; | ||
| import org.apache.kafka.common.errors.OffsetOutOfRangeException; | ||
| import org.apache.kafka.common.utils.LogContext; | ||
| import org.apache.kafka.storage.internals.log.FetchDataInfo; | ||
|
|
@@ -31,14 +32,19 @@ public class RemoteLogReader implements Callable<Void> { | |
| private final Logger logger; | ||
| private final RemoteStorageFetchInfo fetchInfo; | ||
| private final RemoteLogManager rlm; | ||
| private final BrokerTopicStats brokerTopicStats; | ||
| private final Consumer<RemoteLogReadResult> callback; | ||
|
|
||
| public RemoteLogReader(RemoteStorageFetchInfo fetchInfo, | ||
| RemoteLogManager rlm, | ||
| Consumer<RemoteLogReadResult> callback) { | ||
| Consumer<RemoteLogReadResult> callback, | ||
| BrokerTopicStats brokerTopicStats) { | ||
| this.fetchInfo = fetchInfo; | ||
| this.rlm = rlm; | ||
| this.brokerTopicStats = brokerTopicStats; | ||
| this.callback = callback; | ||
| this.brokerTopicStats.topicStats(fetchInfo.topicPartition.topic()).remoteReadRequestRate().mark(); | ||
| this.brokerTopicStats.allTopicsStats().remoteReadRequestRate().mark(); | ||
| logger = new LogContext() { | ||
| @Override | ||
| public String logPrefix() { | ||
|
|
@@ -54,10 +60,14 @@ public Void call() { | |
| logger.debug("Reading records from remote storage for topic partition {}", fetchInfo.topicPartition); | ||
|
|
||
| FetchDataInfo fetchDataInfo = rlm.read(fetchInfo); | ||
| brokerTopicStats.topicStats(fetchInfo.topicPartition.topic()).remoteBytesInRate().mark(fetchDataInfo.records.sizeInBytes()); | ||
| brokerTopicStats.allTopicsStats().remoteBytesInRate().mark(fetchDataInfo.records.sizeInBytes()); | ||
| result = new RemoteLogReadResult(Optional.of(fetchDataInfo), Optional.empty()); | ||
| } catch (OffsetOutOfRangeException e) { | ||
| result = new RemoteLogReadResult(Optional.empty(), Optional.of(e)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, came across this PR and have a qq, why we don't include the OffsetOutOfRangeException result into the failed remote read request rate metrics? If for some reason we can not, can we have a separate metrics to capture that error in metrics?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not want to include OffsetOutOfRangeException because it is not really a failure in remote reads. We can have a separate discussion on a different metric since that change is unrelated to this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. I will create a JIRA to track that discussion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } catch (Exception e) { | ||
| brokerTopicStats.topicStats(fetchInfo.topicPartition.topic()).failedRemoteReadRequestRate().mark(); | ||
| brokerTopicStats.allTopicsStats().failedRemoteReadRequestRate().mark(); | ||
| logger.error("Error occurred while reading the remote data for {}", fetchInfo.topicPartition, e); | ||
| result = new RemoteLogReadResult(Optional.empty(), Optional.of(e)); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -277,6 +277,12 @@ class BrokerTopicMetrics(name: Option[String]) { | |||||||||||||||||
| BrokerTopicStats.TotalFetchRequestsPerSec -> MeterWrapper(BrokerTopicStats.TotalFetchRequestsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.FetchMessageConversionsPerSec -> MeterWrapper(BrokerTopicStats.FetchMessageConversionsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.ProduceMessageConversionsPerSec -> MeterWrapper(BrokerTopicStats.ProduceMessageConversionsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.RemoteBytesOutPerSec -> MeterWrapper(BrokerTopicStats.RemoteBytesOutPerSec, "bytes"), | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add Remote metrics only when RemoteStorage is enabled on the cluster. Otherwise these these per-topic metrics are useless.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This requires a significant amount of changes. We need to know in this class if remote storage is enabled and for that to happen BrokerTopicStats needs to know that. There are many instances of object creation for BrokerTopicStats all of which will need to supply whether remote storage is enabled. Should we address this in a separate PR? Let me know if you have better ideas to do this.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine. Please open a JIRA ticket for this improvement. Thanks.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't able to find the JIRA, so when ahead created on https://issues.apache.org/jira/browse/KAFKA-15189 . This JIRA is a blocker for 3.6 launch otherwise it will leak to regression for customers not using the Remote Storage feature, |
||||||||||||||||||
| BrokerTopicStats.RemoteBytesInPerSec -> MeterWrapper(BrokerTopicStats.RemoteBytesInPerSec, "bytes"), | ||||||||||||||||||
| BrokerTopicStats.RemoteReadRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteReadRequestsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.RemoteWriteRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteWriteRequestsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.FailedRemoteReadRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteReadRequestsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.FailedRemoteWriteRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteWriteRequestsPerSec, "requests"), | ||||||||||||||||||
|
Comment on lines
+282
to
+285
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. few more naming suggestions:
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the KIP version was voted with the earlier names and now changing the KIP would be equivalent to breaking the agreed contract when the KIP was accepted. I will propose to keep the metric names as they were in the KIP. If we want to introduce new ones, we can start a new KIP which deprecates these metrics and uses new ones.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was suspecting this would be the case. Though as metrics haven't been implemented yet, would it be possible to amend the KIP and ask for feedback?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We at (Amazon MSK) have an implementation against the KIP-405 interfaces [1] and we explicitly tell the customers that the metric such as [1] see: RemoteBytesInPerSec at https://docs.aws.amazon.com/msk/latest/developerguide/metrics-details.html
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should avoid having multiple versions of metrics here. As it is not yet released in Apache, we should have better naming agreeable for the majority. We do not need to have two versions here when it is released in Apache. It avoids confusion for users too. We can discuss whether that can be done by updating KIP-405(I guess I did update d a couple of them) or we can have a minor KIP for them. @divijvaidya This small change can be handled by organizations that adopted KIP-405. |
||||||||||||||||||
| BrokerTopicStats.NoKeyCompactedTopicRecordsPerSec -> MeterWrapper(BrokerTopicStats.NoKeyCompactedTopicRecordsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.InvalidMagicNumberRecordsPerSec -> MeterWrapper(BrokerTopicStats.InvalidMagicNumberRecordsPerSec, "requests"), | ||||||||||||||||||
| BrokerTopicStats.InvalidMessageCrcRecordsPerSec -> MeterWrapper(BrokerTopicStats.InvalidMessageCrcRecordsPerSec, "requests"), | ||||||||||||||||||
|
|
@@ -336,6 +342,18 @@ class BrokerTopicMetrics(name: Option[String]) { | |||||||||||||||||
|
|
||||||||||||||||||
| def invalidOffsetOrSequenceRecordsPerSec: Meter = metricTypeMap.get(BrokerTopicStats.InvalidOffsetOrSequenceRecordsPerSec).meter() | ||||||||||||||||||
|
|
||||||||||||||||||
| def remoteBytesOutRate: Meter = metricTypeMap.get(BrokerTopicStats.RemoteBytesOutPerSec).meter() | ||||||||||||||||||
|
|
||||||||||||||||||
| def remoteBytesInRate: Meter = metricTypeMap.get(BrokerTopicStats.RemoteBytesInPerSec).meter() | ||||||||||||||||||
|
|
||||||||||||||||||
| def remoteReadRequestRate: Meter = metricTypeMap.get(BrokerTopicStats.RemoteReadRequestsPerSec).meter() | ||||||||||||||||||
|
|
||||||||||||||||||
| def remoteWriteRequestRate: Meter = metricTypeMap.get(BrokerTopicStats.RemoteWriteRequestsPerSec).meter() | ||||||||||||||||||
|
|
||||||||||||||||||
| def failedRemoteReadRequestRate: Meter = metricTypeMap.get(BrokerTopicStats.FailedRemoteReadRequestsPerSec).meter() | ||||||||||||||||||
|
|
||||||||||||||||||
| def failedRemoteWriteRequestRate: Meter = metricTypeMap.get(BrokerTopicStats.FailedRemoteWriteRequestsPerSec).meter() | ||||||||||||||||||
|
|
||||||||||||||||||
| def closeMetric(metricType: String): Unit = { | ||||||||||||||||||
| val meter = metricTypeMap.get(metricType) | ||||||||||||||||||
| if (meter != null) | ||||||||||||||||||
|
|
@@ -360,6 +378,12 @@ object BrokerTopicStats { | |||||||||||||||||
| val ProduceMessageConversionsPerSec = "ProduceMessageConversionsPerSec" | ||||||||||||||||||
| val ReassignmentBytesInPerSec = "ReassignmentBytesInPerSec" | ||||||||||||||||||
| val ReassignmentBytesOutPerSec = "ReassignmentBytesOutPerSec" | ||||||||||||||||||
| val RemoteBytesOutPerSec = "RemoteBytesOutPerSec" | ||||||||||||||||||
| val RemoteBytesInPerSec = "RemoteBytesInPerSec" | ||||||||||||||||||
| val RemoteReadRequestsPerSec = "RemoteReadRequestsPerSec" | ||||||||||||||||||
| val RemoteWriteRequestsPerSec = "RemoteWriteRequestsPerSec" | ||||||||||||||||||
| val FailedRemoteReadRequestsPerSec = "RemoteReadErrorsPerSec" | ||||||||||||||||||
| val FailedRemoteWriteRequestsPerSec = "RemoteWriteErrorsPerSec" | ||||||||||||||||||
|
|
||||||||||||||||||
| // These following topics are for LogValidator for better debugging on failed records | ||||||||||||||||||
| val NoKeyCompactedTopicRecordsPerSec = "NoKeyCompactedTopicRecordsPerSec" | ||||||||||||||||||
|
|
@@ -415,6 +439,12 @@ class BrokerTopicStats extends Logging { | |||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.ProduceMessageConversionsPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.ReplicationBytesOutPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.ReassignmentBytesOutPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.RemoteBytesOutPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.RemoteBytesInPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.RemoteReadRequestsPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.RemoteWriteRequestsPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.FailedRemoteReadRequestsPerSec) | ||||||||||||||||||
| topicMetrics.closeMetric(BrokerTopicStats.FailedRemoteWriteRequestsPerSec) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.