-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Expose compaction metrics to Prometheus #11739
Expose compaction metrics to Prometheus #11739
Conversation
88b3c8f
to
adcd231
Compare
Thanks for your contribution. Please do not forget to update docs later. And you can ping me to review the docs, thanks. |
adcd231
to
7eab18b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good.
I left one comment about exposing an "Impl" class in PersistentTopic.
PTAL
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/CompactionMetricsTest.java
Outdated
Show resolved
Hide resolved
Thanks for reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/pulsarbot run-failure-checks |
Just took a glance, and this is a good addition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start. I left a couple comments where I think we can improve the data modeling to minimize the number of get
calls required to retrieve metrics from the same CompactionRecord
object.
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/CompactionMetrics.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/CompactionMetrics.java
Outdated
Show resolved
Hide resolved
@Technoboy- it could also be convenient to allow users to aggregate these metrics at a namespace level, but not a topic level. This would especially be helpful for users with many compacted topics. |
Yes, the current CompactorMetrics is based on namespace level. And many thanks for your kind reivew. |
/pulsarbot run-failure-checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Technoboy- thanks for making those changes. It looks great to me. The only change I can see is to update the computeIfAbsent
logic to prevent the same value from being added twice.
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/AbstractMetrics.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/AbstractMetrics.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/AbstractMetrics.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/CompactionMetrics.java
Outdated
Show resolved
Hide resolved
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to check the final prometheus metric output result in PrometheusMetricsTest
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for addressing all of my feedback.
Many thanks for your kind review. |
Yes, done. Thanks for reviewing. |
Hi @sijia-w, in this PR, @Technoboy- has already added docs to
Thanks! |
### Motivation As #11564 has involved compaction metrics in CLI, it's extremely useful to expose relative metrics to Prometheus. - pulsar_compaction_removed_event_count : the removed event count of compaction . - pulsar_compaction_succeed_count : the succeed count of compaction. - pulsar_compaction_failed_count : the failed count of compaction. - pulsar_compaction_duration_time_in_millis : the duration time of compaction. - pulsar_compaction_read_throughput : the read throughput of compaction. - pulsar_compaction_write_throughput : the write throughput of compaction. - pulsar_compaction_compacted_entries_count: the compacted entries count. - pulsar_compaction_compacted_entries_size: the compacted entries size; if users enable the topic level metrics and the topic has been compacted or doing the compaction, we should expose the compaction metrics for the topic level. If users disable the topic level metrics, we should expose the aggregated compaction metrics for the namespace level. (cherry picked from commit 656635e)
### Motivation As apache#11564 has involved compaction metrics in CLI, it's extremely useful to expose relative metrics to Prometheus. - pulsar_compaction_removed_event_count : the removed event count of compaction . - pulsar_compaction_succeed_count : the succeed count of compaction. - pulsar_compaction_failed_count : the failed count of compaction. - pulsar_compaction_duration_time_in_millis : the duration time of compaction. - pulsar_compaction_read_throughput : the read throughput of compaction. - pulsar_compaction_write_throughput : the write throughput of compaction. - pulsar_compaction_compacted_entries_count: the compacted entries count. - pulsar_compaction_compacted_entries_size: the compacted entries size; if users enable the topic level metrics and the topic has been compacted or doing the compaction, we should expose the compaction metrics for the topic level. If users disable the topic level metrics, we should expose the aggregated compaction metrics for the namespace level. (cherry picked from commit 656635e) (cherry picked from commit a80f1e8)
### Motivation As apache#11564 has involved compaction metrics in CLI, it's extremely useful to expose relative metrics to Prometheus. - pulsar_compaction_removed_event_count : the removed event count of compaction . - pulsar_compaction_succeed_count : the succeed count of compaction. - pulsar_compaction_failed_count : the failed count of compaction. - pulsar_compaction_duration_time_in_millis : the duration time of compaction. - pulsar_compaction_read_throughput : the read throughput of compaction. - pulsar_compaction_write_throughput : the write throughput of compaction. - pulsar_compaction_compacted_entries_count: the compacted entries count. - pulsar_compaction_compacted_entries_size: the compacted entries size; if users enable the topic level metrics and the topic has been compacted or doing the compaction, we should expose the compaction metrics for the topic level. If users disable the topic level metrics, we should expose the aggregated compaction metrics for the namespace level.
Motivation
As #11564 has involved compaction metrics in CLI, it's extremely useful to expose relative metrics to Prometheus.
if users enable the topic level metrics and the topic has been compacted or doing the compaction, we should expose the compaction metrics for the topic level. If users disable the topic level metrics, we should expose the aggregated compaction metrics for the namespace level.
Documentation
For contributor
The document is required.