You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2023/01/15 10:55:43 UTC

[GitHub] [pulsar] tjiuming opened a new pull request, #19236: [improve][monitoring]PIP-231: Add `topic_load_failed` metric

tjiuming opened a new pull request, #19236:
URL: https://github.com/apache/pulsar/pull/19236

   PIP: https://github.com/apache/pulsar/issues/18979
   
   ### Motivation
   
   Add `topic_load_failed` metric
   
   ### Does this pull request potentially affect one of the following parts:
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   *If the box was checked, please highlight the changes*
   
   - [ ] Dependencies (add or upgrade a dependency)
   - [ ] The public API
   - [ ] The schema
   - [ ] The default values of configurations
   - [ ] The threading model
   - [ ] The binary protocol
   - [ ] The REST endpoints
   - [ ] The admin CLI options
   - [x] The metrics
   - [ ] Anything that affects deployment
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [x] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] tjiuming commented on pull request #19236: [improve]PIP-231: Add `topic_load_failed` metric

Posted by "tjiuming (via GitHub)" <gi...@apache.org>.
tjiuming commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1431105127

   > @tjiuming You should associate issue with pr. Closes #18963 #18979 And some checks were not successful
   
   I'll close them after the PR merged


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] github-actions[bot] commented on pull request #19236: [improve]PIP-231: Add `topic_load_failed` metric

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1493197255

   The pr had no activity for 30 days, mark with Stale label.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] congbobo184 commented on a diff in pull request #19236: [improve][monitoring]PIP-231: Add `topic_load_failed` metric

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#discussion_r1070568329


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1176,6 +1176,10 @@ public void deleteTopicAuthenticationWithRetry(String topic, CompletableFuture<V
 
     private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic) {
         CompletableFuture<Optional<Topic>> topicFuture = new CompletableFuture<>();
+        topicFuture.exceptionally(t -> {
+            pulsarStats.recordTopicLoadFailed(topic);
+            return Optional.empty();

Review Comment:
   ```suggestion
               return null;
   ```



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarStats.java:
##########
@@ -239,6 +239,14 @@ public void recordTopicLoadTimeValue(String topic, long topicLoadLatencyMs) {
         }
     }
 
+    public void recordTopicLoadFailed(String topic) {
+        try {
+            brokerOperabilityMetrics.recordTopicLoadFailed();
+        } catch (Exception ex) {

Review Comment:
   why catch exception?



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1522,6 +1526,11 @@ private void createPersistentTopic(final String topic, boolean createIfMissing,
         TopicName topicName = TopicName.get(topic);
         final long topicCreateTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
 
+        topicFuture.exceptionally(t -> {
+            pulsarStats.recordTopicLoadFailed(topic);
+            return Optional.empty();

Review Comment:
   ```suggestion
               return null;
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1576802314

   I wonder if this caused #20492 ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Technoboy- commented on pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1573246268

   /pulsarbot run-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Technoboy- commented on a diff in pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#discussion_r1212918817


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/BrokerOperabilityMetrics.java:
##########
@@ -29,6 +30,7 @@
 /**
  */
 public class BrokerOperabilityMetrics {
+    private static final Counter TOPIC_LOAD_FAILED = Counter.build("topic_load_failed", "-").register();

Review Comment:
   removed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Technoboy- merged pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- merged PR #19236:
URL: https://github.com/apache/pulsar/pull/19236


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Anonymitaet commented on pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "Anonymitaet (via GitHub)" <gi...@apache.org>.
Anonymitaet commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1636637724

   @tjiuming this PR is labeled with `doc-required`. Does it need to add the new metric to https://pulsar.apache.org/docs/next/reference-metrics/#topic-metrics?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] shy-share commented on pull request #19236: [improve][monitoring]PIP-231: Add `topic_load_failed` metric

Posted by "shy-share (via GitHub)" <gi...@apache.org>.
shy-share commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1430956245

   @tjiuming You should associate issue with pr. close #18963 #18979 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codecov-commenter commented on pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1573020097

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/19236?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#19236](https://app.codecov.io/gh/apache/pulsar/pull/19236?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4d1fab7) into [master](https://app.codecov.io/gh/apache/pulsar/commit/5e6e6cebcdbeec32ed49729f658f2d5cd0d98347?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5e6e6ce) will **decrease** coverage by `36.07%`.
   > The diff coverage is `85.71%`.
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/19236/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/19236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #19236       +/-   ##
   =============================================
   - Coverage     72.95%   36.89%   -36.07%     
   + Complexity    31914    12091    -19823     
   =============================================
     Files          1867     1690      -177     
     Lines        138485   128927     -9558     
     Branches      15202    14031     -1171     
   =============================================
   - Hits         101037    47565    -53472     
   - Misses        29438    75112    +45674     
   + Partials       8010     6250     -1760     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | inttests | `24.20% <85.71%> (+0.08%)` | :arrow_up: |
   | systests | `24.96% <42.85%> (-0.07%)` | :arrow_down: |
   | unittests | `32.02% <42.85%> (-40.21%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/19236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...rg/apache/pulsar/broker/service/BrokerService.java](https://app.codecov.io/gh/apache/pulsar/pull/19236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Jyb2tlclNlcnZpY2UuamF2YQ==) | `56.78% <66.66%> (-23.87%)` | :arrow_down: |
   | [.../org/apache/pulsar/broker/service/PulsarStats.java](https://app.codecov.io/gh/apache/pulsar/pull/19236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1B1bHNhclN0YXRzLmphdmE=) | `82.57% <100.00%> (-4.35%)` | :arrow_down: |
   | [.../pulsar/broker/stats/BrokerOperabilityMetrics.java](https://app.codecov.io/gh/apache/pulsar/pull/19236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9Ccm9rZXJPcGVyYWJpbGl0eU1ldHJpY3MuamF2YQ==) | `100.00% <100.00%> (ø)` | |
   
   ... and [1431 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/19236/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] tjiuming commented on a diff in pull request #19236: [improve][monitoring]PIP-231: Add `topic_load_failed` metric

Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#discussion_r1070571380


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarStats.java:
##########
@@ -239,6 +239,14 @@ public void recordTopicLoadTimeValue(String topic, long topicLoadLatencyMs) {
         }
     }
 
+    public void recordTopicLoadFailed(String topic) {
+        try {
+            brokerOperabilityMetrics.recordTopicLoadFailed();
+        } catch (Exception ex) {

Review Comment:
   just following the old pattern: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarStats.java#L234



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] hangc0276 commented on a diff in pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#discussion_r1212910838


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/BrokerOperabilityMetrics.java:
##########
@@ -29,6 +30,7 @@
 /**
  */
 public class BrokerOperabilityMetrics {
+    private static final Counter TOPIC_LOAD_FAILED = Counter.build("topic_load_failed", "-").register();

Review Comment:
   Why use `static`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] lhotari commented on pull request #19236: [improve][broker] Add `topic_load_failed` metric

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1576918863

   The test added by this PR causes issues in CI. The test is disabled by #20494. There's a issue about rewriting the test in #20495 . 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] tjiuming commented on pull request #19236: [improve][monitoring]PIP-231: Add `topic_load_failed` metric

Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1383118982

   Tests to be completed


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] congbobo184 commented on a diff in pull request #19236: [improve][monitoring]PIP-231: Add `topic_load_failed` metric

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#discussion_r1070641433


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarStats.java:
##########
@@ -239,6 +239,14 @@ public void recordTopicLoadTimeValue(String topic, long topicLoadLatencyMs) {
         }
     }
 
+    public void recordTopicLoadFailed(String topic) {
+        try {
+            brokerOperabilityMetrics.recordTopicLoadFailed();
+        } catch (Exception ex) {

Review Comment:
   I don't think it needs catch exception, because we don't need to catch any RuntimeException here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] shy-share commented on pull request #19236: [improve]PIP-231: Add `topic_load_failed` metric

Posted by "shy-share (via GitHub)" <gi...@apache.org>.
shy-share commented on PR #19236:
URL: https://github.com/apache/pulsar/pull/19236#issuecomment-1431132772

   > > @tjiuming You should associate issue with pr. Closes #18963 #18979 And some checks were not successful
   > 
   > I'll close them after the PR merged
   
   GitHub provides this function. After you associate the issue with pr, closing the pr will automatically close the issue. 
   refer to https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org