KAFKA-14953: Add tiered storage related metrics by abhijeetk88 · Pull Request #13944 · apache/kafka

abhijeetk88 · 2023-07-01T05:26:59Z

In this PR, I have added the following metrics that are related to tiered storage mentioned in KIP-405.

Metric	Description
RemoteReadRequestsPerSec	Number of remote storage read requests per second
RemoteWriteRequestsPerSec	Number of remote storage write requests per second
RemoteBytesInPerSec	Number of bytes read from remote storage per second
RemoteReadErrorsPerSec	Number of remote storage read errors per second
RemoteBytesOutPerSec	Number of bytes copied to remote storage per second
RemoteWriteErrorsPerSec	Number of remote storage write errors per second
RemoteLogReaderTaskQueueSize	Number of remote storage read tasks pending for execution
RemoteLogReaderAvgIdlePercent	Average idle percent of the remote storage reader thread pool
RemoteLogManagerTasksAvgIdlePercent	Average idle percent of RemoteLogManager thread pool

Added unit tests for all the rate metrics.

I will have a follow-up PR to add these metrics to Kafka documentation.

Committer Checklist (excluded from commit message)

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

showuon · 2023-07-01T06:29:44Z

@abhijeetk88 , please let me know when PR is ready for review. Thanks for the work on weekend! :)

abhijeetk88 · 2023-07-03T11:18:54Z

@showuon The PR is ready for review. Please take a look.

showuon · 2023-07-03T11:24:02Z

Thanks @abhijeetk88 , will review it later this week.

divijvaidya

Thank you for making this change! I left a couple of comments. Let me know what you think about them.

divijvaidya · 2023-07-03T16:36:22Z

We should probably add Remote metrics only when RemoteStorage is enabled on the cluster. Otherwise these these per-topic metrics are useless.

Good point!

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.

Fine. Please open a JIRA ticket for this improvement. Thanks.

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,

showuon

@abhijeetk88 , thanks for the PR. Made a pass, overall LGTM!
But I didn't see the tests for RemoteLogReaderTaskQueueSize, RemoteLogReaderAvgIdlePercent, and RemoteLogManagerTasksAvgIdlePercent. Could you help add them? Thanks.

showuon · 2023-07-05T08:18:27Z

Good point!

abhijeetk88 · 2023-07-08T19:27:09Z

@showuon adding tests for those gauge metrics requires significant changes. We will need to obtain a handle for the underlying threadpool, submit tasks to the threadpool, make sure they don't progress, and then verify the gauges for non-zero values.

Shall we add those tests in a follow-up PR? Let me know if you have better ideas for these tests.

satishd · 2023-07-10T06:34:57Z

@abhijeetk88 @showuon Let us have a followup JIRA/PR on adding more test for the metrics.

showuon · 2023-07-10T06:36:44Z

@abhijeetk88 @showuon Let us have a followup JIRA/PR on adding more test for the metrics.

Sure, please open a JIRA ticket to track it. Thanks.

jeqo · 2023-07-10T21:31:47Z

+    BrokerTopicStats.RemoteReadRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteReadRequestsPerSec, "requests"),
+    BrokerTopicStats.RemoteWriteRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteWriteRequestsPerSec, "requests"),
+    BrokerTopicStats.FailedRemoteReadRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteReadRequestsPerSec, "requests"),
+    BrokerTopicStats.FailedRemoteWriteRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteWriteRequestsPerSec, "requests"),


few more naming suggestions:

Suggested change

BrokerTopicStats.RemoteReadRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteReadRequestsPerSec, "requests"),

BrokerTopicStats.RemoteWriteRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteWriteRequestsPerSec, "requests"),

BrokerTopicStats.FailedRemoteReadRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteReadRequestsPerSec, "requests"),

BrokerTopicStats.FailedRemoteWriteRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteWriteRequestsPerSec, "requests"),

BrokerTopicStats.RemoteFetchRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteFetchRequestsPerSec, "requests"),

BrokerTopicStats.RemoteCopyRequestsPerSec -> MeterWrapper(BrokerTopicStats.RemoteCopyRequestsPerSec, "requests"),

BrokerTopicStats.FailedRemoteCopyRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteFetchRequestsPerSec, "requests"),

BrokerTopicStats.FailedRemoteCopyRequestsPerSec -> MeterWrapper(BrokerTopicStats.FailedRemoteCopyRequestsPerSec, "requests"),

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.

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?

We at (Amazon MSK) have an implementation against the KIP-405 interfaces [1] and we explicitly tell the customers that the metric such as RemoteBytesInPerSec are as per KIP-405 contract. If we change the KIP now, it would be a break in contract for our customers. Hence, I would not be in favour of amending the accepted KIP at this time.

[1] see: RemoteBytesInPerSec at https://docs.aws.amazon.com/msk/latest/developerguide/metrics-details.html

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.

showuon · 2023-07-11T09:52:05Z

@abhijeetk88 @showuon Let us have a followup JIRA/PR on adding more test for the metrics.

Sure, please open a JIRA ticket to track it. Thanks.

KAFKA-15176 is opened.

showuon

LGTM! @abhijeetk88 , could you resolve the conflict?
@divijvaidya @jeqo , any other comments to this PR?

satishd

Thanks @abhijeetk88 for the PR. Overall LGTM. But we can have better naming for these metrics. That can be done in a followup PR once we decide on the names.

jeqo

LGTM, thanks @abhijeetk88 !

divijvaidya

Thank you for you patience so far Abhijeet. I have one last round of questions/comments. I promise to provide rapid review replies (on weekdays) to wrap up this PR soon.

divijvaidya · 2023-07-14T11:36:51Z

                Utils.closeQuietly(indexCache, "RemoteIndexCache");

                rlmScheduledThreadPool.close();
+                removeMetrics();


This should probably be done in a try/finally after the thread pool (in the next line) has been shutdown.

divijvaidya · 2023-07-14T11:47:32Z

+            KafkaMetricsGroup mockThreadPoolMetricsGroup = mockMetricsGroupCtor.constructed().get(1);
+
+            List<String> remoteLogManagerMetricNames = Collections.singletonList(REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT);
+            List<String> remoteStorageThreadPoolMetricNames = Arrays.asList(


When we add a metric to the RemoteLogManager or to the thread pool, we will have to update this test. Alternatively, may I suggest, the pattern we have used at other places, i.e. create a list of metrics in the class where metric group is present, and verify invocations in the test for all members of that class. With this pattern, you won't have to modify the test at all when adding new metrics.

As an example, you can see

kafka/core/src/main/scala/kafka/log/LogCleaner.scala

Line 529 in b3ce2e5

private[log] val MetricNames = Set(

Separately, I also like the pattern of metric decoupling introduced used by QuorumControllerMetrics. It neatly encapsulates all QuorumController metrics at one place and we can potentially do similar for RemoteLogManager. This is a suggestion and feel free to ignore this.

divijvaidya · 2023-07-14T12:14:39Z

    BrokerTopicStats.TotalFetchRequestsPerSec -> MeterWrapper(BrokerTopicStats.TotalFetchRequestsPerSec, "requests"),
    BrokerTopicStats.FetchMessageConversionsPerSec -> MeterWrapper(BrokerTopicStats.FetchMessageConversionsPerSec, "requests"),
    BrokerTopicStats.ProduceMessageConversionsPerSec -> MeterWrapper(BrokerTopicStats.ProduceMessageConversionsPerSec, "requests"),
+    BrokerTopicStats.RemoteCopyBytesPerSec -> MeterWrapper(BrokerTopicStats.RemoteCopyBytesPerSec, "bytes"),


Please note that for some other metrics, we store aggregated topics stat using allTopicsStats. Are we intentionally not add remote* metrics to it?

I have added those now.

hudeqi · 2023-07-14T10:52:13Z


    }
+
+    public void removeMetrics() {


Hi, I am planning to clean up all the leaked metrics in the current base code recently, so I will pay close attention to the "metric", Is it possible to follow this template here? It is better to put the metric name in a collection to prevent others from forgetting to remove when adding a new metric. Otherwise, my work in metric cleaning this area may be endless. @abhijeetk88

abhijeetk88 · 2023-07-16T17:44:25Z

Addressed all the comments. Please take a look.

showuon · 2023-07-17T02:01:35Z

@abhijeetk88 , there are errors from spotbugs. Could you take a look?
https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-13944/7/pipeline

divijvaidya

Thank you for your patience through this PR @abhijeetk88 (and congratulations of your first contribution to Apache Kafka!)

The changes look good to me. I am waiting for CI to reach an acceptable state now.

kamalcph

LGTM. Thanks for the PR!

Staniel · 2023-07-17T23:01:36Z

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?

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.

Thank you. I will create a JIRA to track that discussion.

https://issues.apache.org/jira/browse/KAFKA-15214

divijvaidya · 2023-07-18T10:07:42Z

This has 33 failures, we are currently have 8-9 flaky test failures in other PRs on trunk. i am re-running the tests.

satishd · 2023-07-18T14:38:37Z

After the jenkins rebuild again, there are ~7 test failures that are unrelated to this PR. Merging it to trunk.

* ak/trunk: (110 commits) MINOR: Update docs to include ZK deprecation notice and information (apache#14031) KAFKA-15091: Fix misleading Javadoc for SourceTask::commit (apache#13948) KAFKA-14669: Use the generated docs for MirrorMaker configs in the doc (apache#13658) KAFKA-14953: Add tiered storage related metrics (apache#13944) KAFKA-15121: Implement the alterOffsets method in the FileStreamSourceConnector and the FileStreamSinkConnector (apache#13945) Revert "MINOR: Update .asf.yaml file with refreshed github_whitelist, and collaborators" (apache#14037) MINOR: Update .asf.yaml file with refreshed github_whitelist, and collaborators KAFKA-14737: Move kafka.utils.json to server-common (apache#13585) KAFKA-14647: Move TopicFilter to server-common/utils (apache#13158) MINOR: remove unused variable in examples (apache#14021) ...

* KAFKA-14953: Adding RemoteLogManager metrics In this PR, I have added the following metrics that are related to tiered storage mentioned in[ KIP-405](https://cwiki.apache.org/confluence/display/KAFKA/KIP-405%3A+Kafka+Tiered+Storage). |Metric|Description| |-----------------------------------------|--------------------------------------------------------------| | RemoteReadRequestsPerSec | Number of remote storage read requests per second | | RemoteWriteRequestsPerSec | Number of remote storage write requests per second | | RemoteBytesInPerSec | Number of bytes read from remote storage per second | | RemoteReadErrorsPerSec | Number of remote storage read errors per second | | RemoteBytesOutPerSec | Number of bytes copied to remote storage per second | | RemoteWriteErrorsPerSec | Number of remote storage write errors per second | | RemoteLogReaderTaskQueueSize | Number of remote storage read tasks pending for execution. | | RemoteLogReaderAvgIdlePercent | Average idle percent of the remote storage reader thread pool| | RemoteLogManagerTasksAvgIdlePercent | Average idle percent of RemoteLogManager thread pool | Added unit tests for all the rate metrics. Reviewers: Luke Chen <showuon@gmail.com>, Divij Vaidya <diviv@amazon.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Jorge Esteban Quilcate Otoya <quilcate.jorge@gmail.com>, Staniel Yao<yaolixinylx@gmail.com>, hudeqi<1217150961@qq.com>, Satish Duggana <satishd@apache.org>

abhijeetk88 changed the title ~~[DRAFT PR - WIP] KAFKA-14953: Adding RemoteLogManager metrics~~ [DRAFT] KAFKA-14953: Adding RemoteLogManager metrics Jul 1, 2023

abhijeetk88 changed the title ~~[DRAFT] KAFKA-14953: Adding RemoteLogManager metrics~~ [DRAFT] KAFKA-14953: Add tiered storage related metrics Jul 1, 2023

abhijeetk88 marked this pull request as draft July 1, 2023 05:36

abhijeetk88 force-pushed the KAFKA-14953 branch from 11e0a7b to 0d1830f Compare July 2, 2023 16:29

abhijeetk88 changed the title ~~[DRAFT] KAFKA-14953: Add tiered storage related metrics~~ KAFKA-14953: Add tiered storage related metrics Jul 3, 2023

abhijeetk88 marked this pull request as ready for review July 3, 2023 11:16

satishd self-requested a review July 3, 2023 11:18

showuon added the tiered-storage Related to the Tiered Storage feature label Jul 3, 2023

divijvaidya reviewed Jul 3, 2023

View reviewed changes

divijvaidya reviewed Jul 4, 2023

View reviewed changes

Comment thread core/src/test/java/kafka/log/remote/RemoteLogReaderTest.java Outdated

showuon reviewed Jul 5, 2023

View reviewed changes

Comment thread core/src/main/scala/kafka/server/KafkaRequestHandler.scala Outdated

Copy link
Copy Markdown

Member

showuon Jul 5, 2023

Choose a reason for hiding this comment

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

Good point!

abhijeetk88 force-pushed the KAFKA-14953 branch from 32d11d2 to e24f7b7 Compare July 8, 2023 19:53

jeqo reviewed Jul 10, 2023

View reviewed changes

showuon approved these changes Jul 13, 2023

View reviewed changes

satishd reviewed Jul 13, 2023

View reviewed changes

jeqo approved these changes Jul 13, 2023

View reviewed changes

satishd approved these changes Jul 13, 2023

View reviewed changes

divijvaidya reviewed Jul 14, 2023

View reviewed changes

hudeqi reviewed Jul 14, 2023

View reviewed changes

abhijeetk88 force-pushed the KAFKA-14953 branch from e24f7b7 to fd3039e Compare July 16, 2023 17:39

divijvaidya approved these changes Jul 17, 2023

View reviewed changes

kamalcph approved these changes Jul 17, 2023

View reviewed changes

Staniel reviewed Jul 17, 2023

View reviewed changes

abhijeetk88 added 9 commits July 18, 2023 10:58

KAFKA-14953: Adding RemoteLogManager metrics

b3e3793

Added more tests

925f9cc

Address review comments

fb59242

Undo renaming metrics

5663ccb

Capture all metrics in single place so we wont forget to remove them

bd1104a

Remove metrics in try/catch

7fcdb14

Aggregate metrics

bd46e24

Fix spotbugs error

5246478

Empty commit

4cc354b

abhijeetk88 force-pushed the KAFKA-14953 branch from 0d82d46 to 4cc354b Compare July 18, 2023 05:29

satishd merged commit fd3b113 into apache:trunk Jul 18, 2023

Conversation

abhijeetk88 commented Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

showuon commented Jul 1, 2023

Uh oh!

abhijeetk88 commented Jul 3, 2023

Uh oh!

showuon commented Jul 3, 2023

Uh oh!

divijvaidya 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!

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!

showuon left a comment • 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!

abhijeetk88 commented Jul 8, 2023

Uh oh!

satishd commented Jul 10, 2023

Uh oh!

showuon commented Jul 10, 2023

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!

showuon commented Jul 11, 2023

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

satishd left a comment

Choose a reason for hiding this comment

Uh oh!

jeqo left a comment

Choose a reason for hiding this comment

Uh oh!

divijvaidya 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!

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

abhijeetk88 commented Jul 1, 2023 •

edited

Loading

showuon left a comment •

edited

Loading

showuon commented Jul 17, 2023 •

edited

Loading

satishd commented Jul 18, 2023 •

edited

Loading