[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16932: [broker][monitoring] add per-subscription EntryFilter metrics

[GitBox <gi...@apache.org>]

michaeljmarshall commented on code in PR #16932:
URL: https://github.com/apache/pulsar/pull/16932#discussion_r942036529


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java:
##########
@@ -51,6 +52,10 @@ public abstract class AbstractBaseDispatcher extends EntryFilterSupport implemen
 
     protected final ServiceConfiguration serviceConfig;
     protected final boolean dispatchThrottlingOnBatchMessageEnabled;
+    private final LongAdder throughFilterMsgs = new LongAdder();
+    private final LongAdder filterAcceptedMsgs = new LongAdder();

Review Comment:
   This is a good discussion to have. Note that the `filterEntriesForConsumer` method, which we are instrumenting uses `entries` throughout. At first, I thought entries would make more sense. However, I think total messages makes sense here. One interesting point is that the `msgOutCounter` measures the total messages by summing all of the messages within a batched message. Ultimately, I think the final solution should be consistent.
   
   The current solution looks good to me.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1756,6 +1756,13 @@ public void updateRates(NamespaceStats nsStats, NamespaceBundleStats bundleStats
                 topicStatsStream.writePair("totalNonContiguousDeletedMessagesRange",
                         subscription.getTotalNonContiguousDeletedMessagesRange());
                 topicStatsStream.writePair("type", subscription.getTypeString());
+
+                Dispatcher dispatcher0 = subscription.getDispatcher();
+                topicStatsStream.writePair("throughEntryFilterMsgs", dispatcher0.getThroughFilterMsgCount());
+                topicStatsStream.writePair("entryFilterAccepted", dispatcher0.getFilterAcceptedMsgCount());
+                topicStatsStream.writePair("entryFilterRejected", dispatcher0.getFilterRejectedMsgCount());
+                topicStatsStream.writePair("entryFilterRescheduled", dispatcher0.getFilterRescheduledMsgCount());

Review Comment:
   I know this is being discussed elsewhere in the PR, but just want to point out that the methods have `Msg` and the metrics have `entry`. We should make sure that the end result is consistent.



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